Re: [PATCH] vsprintf: protect kernel from panic due to non-canonical pointer dereference

[Petr Mladek <pmladek@suse.com>]

On Tue 2022-10-18 23:49:27, Andy Shevchenko wrote:
> On Tue, Oct 18, 2022 at 08:30:01PM +0000, Jane Chu wrote:
> > On 10/18/2022 1:07 PM, Andy Shevchenko wrote:
> > > On Tue, Oct 18, 2022 at 06:56:31PM +0000, Jane Chu wrote:
> > >> On 10/18/2022 5:45 AM, Petr Mladek wrote:
> > >>> On Mon 2022-10-17 19:31:53, Jane Chu wrote:
> > >>>> On 10/17/2022 12:25 PM, Andy Shevchenko wrote:
> > >>>>> On Mon, Oct 17, 2022 at 01:16:11PM -0600, Jane Chu wrote:
> > >>>>>> While debugging a separate issue, it was found that an invalid string
> > >>>>>> pointer could very well contain a non-canical address, such as
> > >>>>>> 0x7665645f63616465. In that case, this line of defense isn't enough
> > >>>>>> to protect the kernel from crashing due to general protection fault
> > >>>>>>
> > >>>>>> 	if ((unsigned long)ptr < PAGE_SIZE || IS_ERR_VALUE(ptr))
> > >>>>>>                    return "(efault)";
> > >>>>>>
> > >>>>>> So instead, use kern_addr_valid() to validate the string pointer.
> > >>>>>
> > >>>>> How did you check that value of the (invalid string) pointer?
> > >>>>>
> > >>>>
> > >>>> In the bug scenario, the invalid string pointer was an out-of-bound
> > >>>> string pointer. While the OOB referencing is fixed,
> > >>>
> > >>> Could you please provide more details about the fixed OOB?
> > >>> What exact vsprintf()/printk() call was broken and eventually
> > >>> how it was fixed, please?
> > >>
> > >> For sensitive reason, I'd like to avoid mentioning the specific name of
> > >> the sysfs attribute in the bug, instead, just call it "devX_attrY[]",
> > >> and describe the precise nature of the issue.
> > >>
> > >> devX_attrY[] is a string array, declared and filled at compile time,
> > >> like
> > >>     const char const devX_attrY[] = {
> > >> 	[ATTRY_A] = "Dev X AttributeY A",
> > >> 	[ATTRY_B] = "Dev X AttributeY B",
> > >> 	...
> > >> 	[ATTRY_G] = "Dev X AttributeY G",
> > >>     }
> > >> such that, when user "cat /sys/devices/systems/.../attry_1",
> > >> "Dev X AttributeY B" will show up in the terminal.
> > >> That's it, no more reference to the pointer devX_attrY[ATTRY_B] after that.
> > >>
> > >> The bug was that the index to the array was wrongfully produced,
> > >> leading up to OOB, e.g. devX_attrY[11].  The fix was to fix the
> > >> calculation and that is not an upstream fix.

I see. printk()/vsprintf() is the only code that accesses this pointer.
If vsprintf() survives than the system survives.

> > As you can see, if the OOBs are NULL, "(null)" was printed due to the 
> > existing checking, but when the OOBs are turned to non-canonical which
> > is detectable, the fact the pointer value deviates from
> >    (ffffffff84d60aee + 4 * sizeof(void *))
> > evidently shown that the OOBs are detectable.
> > 
> > The question then is why should the non-canonical OOBs be treated 
> > differently from NULL and ERR_VALUE?
> 
> Obviously, to see the crash. And let kernel _to crash_. Isn't it what we need
> to see a bug as early as possible?

I do not agree here. Kernel tries to survive many situations when
thighs does not work as expected. It prints a warning so that
users/developers are aware of the problem and could fix it.

In our case, the crash happened when reading a sysfs file.
IMHO, it is much better to show (-EINVAL) than crash. The bug
when accessing devX_attrY[] does not affect the stability of
the system at all.

And the broken string might be passed in a very rare case,
e.g. in an error path. So that it might be hard to catch
when testing.

Best Regards,
Petr