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

From: Jane Chu <jane.chu@oracle.com>
To: Petr Mladek <pmladek@suse.com>
Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.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>,
	"linux-mm@kvack.org" <linux-mm@kvack.org>,
	Haakon Bugge <haakon.bugge@oracle.com>,
	John Haxby <john.haxby@oracle.com>,
	Jane Chu <jane.chu@oracle.com>
Subject: Re: [PATCH v2] vsprintf: protect kernel from panic due to non-canonical pointer dereference
Date: Wed, 19 Oct 2022 20:02:36 +0000	[thread overview]
Message-ID: <860872bd-127f-36ee-f803-6553a6f03826@oracle.com> (raw)
In-Reply-To: <Y0/EfVB7fRFXrz2c@alley>

Hi, Petr,

Sorry, I didn't catch this email prior to sending out v3.

[..]
>>
>> Yes, kern_addr_valid() is used by read_kcore() which is architecturally
>> independent and applies everywhere, so does that imply that it is
>> defined in all architectures?
> 
> It is more complicated. fs/proc/kcore.c is built when
> CONFIG_PROC_KCORE is set. It is defined in fs/proc/Kconfig as:
> 
> config PROC_KCORE
> 	bool "/proc/kcore support" if !ARM
> 	depends on PROC_FS && MMU
> 
> So, it is not built on ARM.

Indeed, it's defined on ARM though.

> 
> More importantly, kern_addr_valid() seems to be implemented only for x86_64.
> It is always true (1) on all other architectures, see
> 
> $> git grep kern_addr_valid
> arch/alpha/include/asm/pgtable.h:#define kern_addr_valid(addr)  (1)
> arch/arc/include/asm/pgtable-bits-arcv2.h:#define kern_addr_valid(addr) (1)
> arch/arm/include/asm/pgtable-nommu.h:#define kern_addr_valid(addr)      (1)
> arch/arm/include/asm/pgtable.h:#define kern_addr_valid(addr)    (1)
> [...]
> 
> Wait, it is actually always false (0) on x86 when SPARSEMEM is used,
> see arch/x86/include/asm/pgtable_32.h:
> 
> #ifdef CONFIG_FLATMEM
> #define kern_addr_valid(addr)	(1)
> #else
> #define kern_addr_valid(kaddr)	(0)
> #endif
> 

Thanks for pointing this out.  Let me do some digging ...

> 
>> I guess the early boot scenario is different in that, potentially unkind
>> users aren't involved, hence a broken kernel is broken and need a fix.
> 
> The important thing is that kern_addr_valid() must return valid
> results even during early boot. Otherwise, vsprintf() would not work
> during the early boot which is not expected.

Yes, agreed.

> 
>> The scenario concerned here is with users could potentially exploit a
>> kernel issue with DOS attack.  Then we have the scenario that the kernel
>> bug itself is confined, in that, had the sysfs not been accessed, the
>> OOB pointer won't be produced.  So in this case, "(efault)" is a lot
>> more desirable than panic.
> 
> Please, provide more details about the bug when invalid pointer was
> passed. As Andy wrote, even if we catch the bad pointer in vsprintf(),
> the kernel would most likely kernel crash anyway.

Hopefully the comment in v3 clarifies the bug, please let me know.

thanks!
-jane

> 
> Best Regards,
> Petr