[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <71c9bce7-cd93-eb2f-5b69-de1a9ffe48b5@oracle.com>
Date: Mon, 17 Oct 2022 21:12:04 +0000
From: Jane Chu <jane.chu@...cle.com>
To: Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
CC: "pmladek@...e.com" <pmladek@...e.com>,
"rostedt@...dmis.org" <rostedt@...dmis.org>,
"senozhatsky@...omium.org" <senozhatsky@...omium.org>,
"linux@...musvillemoes.dk" <linux@...musvillemoes.dk>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v2] vsprintf: protect kernel from panic due to
non-canonical pointer dereference
On 10/17/2022 1:27 PM, Andy Shevchenko wrote:
> On Mon, Oct 17, 2022 at 01:44:47PM -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
>
> non-canical?
Sorry, typo, will fix.
>
>> 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 run one more round of check via kern_addr_valid(). On architectures
>> that provide meaningful implementation, this line of check effectively
>> catches non-canonical pointers, etc.
>
> OK, but I don't see how this is useful in the form of returning efault here.
> Ideally we should inform user that the pointer is wrong and how it's wrong.
> But. It will crash somewhere else at some point, right?
Broadly speaking, yes. It's not a perfect line of defense, but again,
the bug scenario is a "cat" of some sysfs attributes that leads to
panic. Does it make sense for kernel to protect itself against panic
triggered by a "cat" from user if it could?
I mean that there
> is no guarantee that kernel has protection in every single place against
> dangling / invalid pointers. One way or another it will crash.
>
> That said, honestly I have no idea how this patch may be considered
> anything but band-aid. OTOH, I don't see a harm. Perhaps others will
> share their opinions.
>
3+ years ago, commit 3e5903eb9cff7 ("vsprintf: Prevent crash when
dereferencing invalid pointers") provided the similar level of
protection as this patch. But it was soon revised by commit
2ac5a3bf7042a ("vsprintf: Do not break early boot with probing
addresses"), and that's why the string() utility no longer detects
non-canonical string pointer.
I only thought that kern_addr_valid() is less of a heavy hammer, and
could be safely deployed.
thanks,
-jane
Powered by blists - more mailing lists