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
