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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

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