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

[Petr Mladek]

On Thu 2022-10-20 09:44:06, Petr Mladek wrote:
> 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.

That said, there is definitely a difference between NULL or error code
code and a random pointer address.

The pointers in ERR_RANGE are likely to stay in this range.
It means that this pointer is hardly usable as a security
attack.

On the other hand, "random" pointer has a bigger chance to be
used for a security attack. From this POV, it is more important
to catch and fix random pointer issues. And shoving just -EINVAL
might not be enough to catch attention.

I guess that this was what Andy wanted to explain. And kernel
crash would definitely catch attention. Showing some warning
with KERN_WARNING or even WARN() might be an alternative.


Anyway, I think that this patch is not worth it:

   + kern_addr_valid() always succeeds on all architectures
     except on x86_64. It means that the check would help
     only on x86_64.

   + kern_addr_valid() always fails on x86 when build with SPARSEMEM.
     This is not acceptable for vsprintf().

   + the situation when only vsprintf() would access the wrong pointer
     are rare. In most cases, the pointer is later used and the kernel
     crashes anyway.

Best Regards,
Petr