Re: [PATCH v6 8/9] vsprintf: Prevent crash when dereferencing invalid pointers

[Petr Mladek <pmladek@suse.com>]

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