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

[Andy Shevchenko]

On Wed, Oct 19, 2022 at 06:36:07PM +0000, Jane Chu wrote:
> On 10/18/2022 1:49 PM, 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.
> >>>>
> >>>>>
> >>>>>> the lingering issue
> >>>>>> is that the kernel ought to be able to protect itself, as the pointer
> >>>>>> contains a non-canonical address.
> >>>>>
> >>>>> Was the pointer used only by the vsprintf()?
> >>>>> Or was it accessed also by another code, please?
> >>>>
> >>>> The OOB pointer was used only by vsprintf() for the "cat" sysfs case.
> >>>> No other code uses the OOB pointer, verified both by code examination
> >>>> and test.
> >>>
> >>> So, then the vsprintf() is _the_ point to crash and why should we hide that?
> >>> Because of the crash you found the culprit, right? The efault will hide very
> >>> important details.
> >>>
> >>> So to me it sounds like I like this change less and less...
> >>
> >> What about the existing check
> >>    	if ((unsigned long)ptr < PAGE_SIZE || IS_ERR_VALUE(ptr))
> >>                       return "(efault)";
> >> ?
> > 
> > Because it's _special_. We know that First page is equivalent to a NULL pointer
> > and the last one is dedicated for so called error pointers. There are no more
> > special exceptions to the addresses in the Linux kernel (I don't talk about
> > alignment requirements by the certain architectures).
> > 
> >> In an experiment just to print the raw OOB pointer values, I saw below
> >> (the devX attrY stuff are substitutes of the real attributes, other
> >> values and strings are verbatim copy from "dmesg"):
> >>
> >> [ 3002.772329] devX_attrY[26]: (ffffffff84d60ad3) Dev X AttributeY E
> >> [ 3002.772346] devX_attrY[27]: (ffffffff84d60ae4) Dev X AttributeY F
> >> [ 3002.772347] devX_attrY[28]: (ffffffff84d60aee) Dev X AttributeY G
> >> [ 3002.772349] devX_attrY[29]: (0) (null)
> >> [ 3002.772350] devX_attrY[30]: (0) (null)
> >> [ 3002.772351] devX_attrY[31]: (0) (null)
> >> [ 3002.772352] devX_attrY[32]: (7665645f63616465) (einval)
> >> [ 3002.772354] devX_attrY[33]: (646e61685f656369) (einval)
> >> [ 3002.772355] devX_attrY[34]: (6f635f65755f656c) (einval)
> >> [ 3002.772355] devX_attrY[35]: (746e75) (einval)
> >>
> >> where starting from index 29 are all OOB pointers.
> >>
> >> 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?
> > 
> 
> If the purpose is to see the bug as early as possible, then getting
> "(efault)" from reading sysfs attribute would serve the purpose, right?
> 
> The fact an OOB pointer has already being turned into either NULL or
> non-canonical value implies that *if* kernel code other than
> vsprintf() references the pointer, it'll crash else where;

No, not the case for error pointers and NULL.

> but *if* no
> other code referencing the pointer, why crash?

Because how else you can see the bug?! The trace will give you essential
information about registers, etc that gives you a hint what the _cause_ of the
crash. And we need that cause. The "(efault)" has not even a bit close to what
crash gives us.

So, this is my last message in the discussion.

Here is a formal NAK. Up to maintainers to decide what to do with this.

-- 
With Best Regards,
Andy Shevchenko