Messages in this thread | | | Subject | Re: [PATCH 1/4] lib/vsprintf.c: handle invalid format specifiers more robustly | From | Andy Shevchenko <> | Date | Mon, 28 Sep 2015 11:08:11 +0300 |
| |
On Fri, 2015-09-25 at 19:41 +0200, Rasmus Villemoes wrote: > If we meet any invalid or unsupported format specifier, 'handling' it > by just printing it as a literal string is not safe: Presumably the > format string and the arguments passed gcc's type checking, but that > means something like sprintf(buf, "%n %pd", &intvar, dentry) would > end > up interpreting &intvar as a struct dentry*. > > When the offending specifier was %n it used to be at the end of the > format string, but we can't rely on that always being the case. Also, > gcc doesn't complain about some more or less exotic qualifiers (or > 'length modifiers' in posix-speak) such as 'j' or 'q', but being > unrecognized by the kernel's printf implementation, they'd be > interpreted as unknown specifiers, and the rest of arguments would be > interpreted wrongly. > > So let's complain about anything we don't understand, not just %n, > and > stop pretending that we'd be able to make sense of the rest of the > format/arguments. If the offending specifier is in a printk() call we > unfortunately only get a "BUG: recent printk recursion!", but at > least > direct users of the sprintf family will be caught.
I like the fix (I noticed the same issue after commented out Martin's patch).
Few minor comments below.
Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> > Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk> > --- > lib/vsprintf.c | 31 +++++++++++++++++++++---------- > 1 file changed, 21 insertions(+), 10 deletions(-) > > diff --git a/lib/vsprintf.c b/lib/vsprintf.c > index 95cd63b43b99..f2590a80937f 100644 > --- a/lib/vsprintf.c > +++ b/lib/vsprintf.c > @@ -1769,14 +1769,14 @@ qualifier: > > case 'n': > /* > - * Since %n poses a greater security risk than > utility, treat > - * it as an invalid format specifier. Warn about its > use so > - * that new instances don't get added. > + * Since %n poses a greater security risk than
Any reason to wrap first string?
> + * utility, treat it as any other invalid or > + * unsupported format specifier. > */ > - WARN_ONCE(1, "Please remove ignored %%n in '%s'\n", > fmt); > /* Fall-through */ > > default: > + WARN_ONCE(1, "Please remove unsupported %%%c in > format string\n", *fmt); > spec->type = FORMAT_TYPE_INVALID; > return fmt - start; > } > @@ -1944,10 +1944,15 @@ int vsnprintf(char *buf, size_t size, const > char *fmt, va_list args) > break; > > case FORMAT_TYPE_INVALID: > - if (str < end) > - *str = '%'; > - ++str; > - break; > + /* > + * Presumably the arguments passed gcc's > type > + * checking, but there is no safe or sane > way > + * for us to continue parsing the format and > + * fetching from the va_list; the remaining > + * specifiers and arguments would be out of > + * sync.
Could we use wider strings in the commentary here?
> + */ > + goto out; > > default: > switch (spec.type) { > @@ -1992,6 +1997,7 @@ int vsnprintf(char *buf, size_t size, const > char *fmt, va_list args) > } > } > > +out: > if (size > 0) { > if (str < end) > *str = '\0'; > @@ -2189,9 +2195,10 @@ do { > > > > \ > > > switch (spec.type) { > case FORMAT_TYPE_NONE: > - case FORMAT_TYPE_INVALID: > case FORMAT_TYPE_PERCENT_CHAR: > break; > + case FORMAT_TYPE_INVALID: > + goto out; > > case FORMAT_TYPE_WIDTH: > case FORMAT_TYPE_PRECISION: > @@ -2253,6 +2260,7 @@ do { > > > > \ > > } > } > > +out: > return (u32 *)(PTR_ALIGN(str, sizeof(u32))) - bin_buf; > #undef save_arg > } > @@ -2375,12 +2383,14 @@ int bstr_printf(char *buf, size_t size, const > char *fmt, const u32 *bin_buf) > break; > > case FORMAT_TYPE_PERCENT_CHAR: > - case FORMAT_TYPE_INVALID: > if (str < end) > *str = '%'; > ++str; > break; > > + case FORMAT_TYPE_INVALID: > + goto out; > + > default: { > unsigned long long num; > > @@ -2423,6 +2433,7 @@ int bstr_printf(char *buf, size_t size, const > char *fmt, const u32 *bin_buf) > } /* switch(spec.type) */ > } /* while(*fmt) */ > > +out: > if (size > 0) { > if (str < end) > *str = '\0';
-- Andy Shevchenko <andriy.shevchenko@linux.intel.com> Intel Finland Oy
| |