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

[Andy Shevchenko]

On Thu, Oct 20, 2022 at 10:52:03AM -0400, Konrad Rzeszutek Wilk wrote:
> On Wed, Oct 19, 2022 at 11:33:47PM +0300, Andy Shevchenko wrote:
> > On Wed, Oct 19, 2022 at 01:41:59PM -0600, Jane Chu wrote:
> > > Having stepped on a local kernel bug where reading sysfs has led to
> > > out-of-bound pointer dereference by vsprintf() which led to GPF panic.
> > > And the reason for GPF is that the OOB pointer was turned to a
> > > non-canonical address such as 0x7665645f63616465.
> > > 
> > > vsprintf() already has this line of defense
> > > 	if ((unsigned long)ptr < PAGE_SIZE || IS_ERR_VALUE(ptr))
> > >                 return "(efault)";
> > > Since a non-canonical pointer can be detected by kern_addr_valid()
> > > on architectures that present VM holes as well as meaningful
> > > implementation of kern_addr_valid() that detects the non-canonical
> > > addresses, this patch adds a check on non-canonical string pointer by
> > > kern_addr_valid() and "(efault)" to alert user that something
> > > is wrong instead of unecessarily panic the server.
> > > 
> > > On the other hand, if the non-canonical string pointer is dereferenced
> > > else where in the kernel, by virtue of being non-canonical, a crash
> > > is expected to be immediate.
> > 
> > What if there is no other dereference except the one happened in printf()?
> > 
> > Just to point out here, that I formally NAKed this on the basis that NULL
> > and error pointers are special, for the bogus pointers we need crash ASAP,
> > no matter what the code issues it. I.o.w. printf() is not special for that
> > kind of pointers (i.e. bogus pointers, but not special).
> 
> Hey Andy,
> 
> Do we want to have user space programs crash the kernel?
> 
> This patch leads to making the kernel more harden so that we do
> not crash when there are bugs but continue on.

Fine, how to push a user to report a bug in the kernel if for them
there is no bug?

OK, let's assume user recognizes this as a bug, what should they do in order
to provide a better description of the bug, so developer can easily debug
and fix it?

> Would we not want that experience for users ?

Yes, if it is a bug in the kernel we want to know it with all possible details.
Hiding bugs is a way to nowhere.

-- 
With Best Regards,
Andy Shevchenko