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

[Jane Chu <jane.chu@oracle.com>]

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)";
?

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?

Thanks,
-jane