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.shevche...@linux.intel.com>
>
> Signed-off-by: Rasmus Villemoes <li...@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.shevche...@linux.intel.com>
Intel Finland Oy
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
