Re: [PATCH] vsprintf: Make "null" pointer dereference more robust

[Petr Mladek]

On Wed 2018-03-07 10:34:17, Linus Torvalds wrote:
> On Wed, Mar 7, 2018 at 7:52 AM, Petr Mladek <pmladek@xxxxxxxx> wrote:
> > If we are changing things, let's do it properly. The range
> > (-PAGE_SIZE,+PAGE_SIZE) is just a small subset of invalid pointers.
> > Let's try to catch more of them by reading one byte using
> > probe_kernel_read(). It would return -FAULT if we are not able
> > to read the address but it would not crash.
> >
> > Then we clearly need a new message when dereferencing invalid
> > poitners that are not pure NULL. I propose (efault).
> 
> "probe_kernel_read()" is really complicated. It takes a *fault* for chrissake!
> 
> Guess what happens now to any crash report if it uses %p and there is
> anything wrong with the VM?

This patch does _not_ affect plain %p, %px, and %pK!

It affects %s and %p* modifiers that need to read data from the
given address.


> Yes, yes, it disables page faults, but that only means that we won't
> go all the way into the generic VM. We'll still take the fauly, still
> do vmalloc fault filling, still do a *lot* of potentially really
> complicated things.

But the faulty way would happen anyway when vsnprintf() tried to
access the data at the given address.


> Guys, stop this idiocy. printk() needs to be *simple* and *reliable*, not fancy.

This patch is primary about printk() reliability. We want a message
instead of a silent crash.

I am open for better ideas how to get the fault-related messages out
when logbug_lock is taken. At the moment we have them in printk_safe
per-CPU buffer. The only chance to see them is crashdump or
printk_safe_flush_on_panic() called in single CPU mode.


> Plus, you just made %p be an excellent leak of some very sensitive
> information, like "where is the kernel mapped" etc.

We might call panic() after lockbuf_lock is released. This would help
to see the message and prevent crash-free hunting for kernel location.

Best Regards,
Petr