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

From: Jane Chu <jane.chu@oracle.com>
To: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Cc: "pmladek@suse.com" <pmladek@suse.com>,
	"rostedt@goodmis.org" <rostedt@goodmis.org>,
	"senozhatsky@chromium.org" <senozhatsky@chromium.org>,
	"linux@rasmusvillemoes.dk" <linux@rasmusvillemoes.dk>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH v2] vsprintf: protect kernel from panic due to non-canonical pointer dereference
Date: Mon, 17 Oct 2022 21:12:04 +0000	[thread overview]
Message-ID: <71c9bce7-cd93-eb2f-5b69-de1a9ffe48b5@oracle.com> (raw)
In-Reply-To: <Y026l2PZgvt+G6p0@smile.fi.intel.com>

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