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

[Petr Mladek]

On Tue 2018-03-06 11:56:25, Andy Shevchenko wrote:
> On Tue, 2018-03-06 at 10:25 +0100, Petr Mladek wrote:
> > On Mon 2018-03-05 16:16:37, Rasmus Villemoes wrote:
> > > On 2 March 2018 at 13:53, Petr Mladek <pmladek@xxxxxxxx> wrote:
> 
> > > > -       if (!ptr && *fmt != 'K' && *fmt != 'x') {
> > > > +       if ((unsigned long)ptr < PAGE_SIZE && *fmt != 'K' && *fmt
> > > > != 'x') {
> > > 
> > > ISTM that accidentally passing an ERR_PTR would be just as likely as
> > > passing a NULL pointer (or some small offset from one), so if we do
> > > this, shouldn't the test also cover IS_ERR values?
> > 
> > It would make perfect sense to catch IS_ERR_PTR(). Derefenrecing
> > such pointer cause crash. But it might be pretty confusing to print
> > "(null)" in this case.
> > 
> > I would handle this in separate patch and print "(err)" or so.
> > Any volunteer to prepare the patch?
> 
> As I pointed out, we have already such check for %s in binary printf().
> And it goes for "(null)". I'm not sure if changing that might break
> something.

I have missed this.

Anyway, I have discussed this with my colleagues. Different people had
different opinions. But I liked the following.

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).

I believe that the error message is not ABI. Otherwise we would
never be able to fix this. Anyway, we would not know if we did
not try it. And I think that this is worth the risk.

Below is my RFC patch. It is rather a concept to see if it would
work. I send it now because others seem to be working on different
approaches. I believe that this is the right direction. I hope
that it does not conflict much with your patches. I deliberately
touched only the locations that are supposed to stay.