Re: [PATCH v6 8/9] vsprintf: Prevent crash when dereferencing invalid pointers
From: Petr Mladek
Date: Wed Feb 20 2019 - 05:24:18 EST
On Tue 2019-02-19 23:15:16, Sergey Senozhatsky wrote:
> On (02/19/19 15:49), Andy Shevchenko wrote:
> > > On (02/19/19 13:02), Andy Shevchenko wrote:
> > > [..]
> > > > And if it's not? You will get in either case incomplete information,
> > > > but at least with "(e" (or even "(") you might get a clue that it
> > > > errornous conditions.
> > >
> > > The thing I'm signaling here is that in some cases we still can
> > > crash the kernel; with the difference that invalid dereference
> > > can now be a memory corruption. Just saying.
> >
> > Wouldn't that mean that the culprit in the caller, not in the callee?
> >
> > (As far as I got your another example with badly called sprintf() which may
> > overwrite stack, etc).
>
> ipv4 printout case does not look like a caller bug to me: we expect a 15
> bytes ipv4 address, allocate a 16 bytes buffer, sprintf() voluntarily
> writes 18 bytes. This error reporting is a bit of a dangerous practice;
> next year someone might add another specifier and another
>
> return string(buf, end, "(this data does not look right)", spec);
I hope that this would never pass a review.
> We probably would want to do something about it. For instance, mandating
> that "(error)" string cannot be larger than 8 bytes can be a good starting
> point.
All the warnings are printed by string_nocheck(). We could add one
more variant for printing the warnings, for example:
static int error_string(char **buf, char *end, const char *err_msg,
struct printf_spec spec)
{
/*
* Limit the error message when the buffer size is unknown.
* It is not full proof but it looks like a reasonable
* compromise. The (null) error string never caused a problem.
*/
if ((end - *buf > INT_MAX / 2) && !spec.precision) {
if (spec.field_width)
spec.precision = spec.field_width;
else
spec.precision = 8;
}
*buf = string_nocheck(*buf, end, err_msg, spec);
return -EFAULT;
}
return 0;
}
Well, I am not sure that it is worth it. I guess that "(null)"
never caused a problem. And all the new error messages have
8 characters at maximum.
Best Regards,
Petr