lkml.org 
[lkml]   [2019]   [Feb]   [20]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH v6 8/9] vsprintf: Prevent crash when dereferencing invalid pointers
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

\
 
 \ /
  Last update: 2019-02-20 11:24    [W:0.312 / U:0.024 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site