Messages in this thread | | | Date | Wed, 20 Feb 2019 11:24:14 +0100 | From | Petr Mladek <> | Subject | Re: [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
| |