Re: [PATCH] vsprintf: don't obfuscate NULL and error pointers

[Ilya Dryomov]

On Tue, Feb 18, 2020 at 11:33 AM Petr Mladek <pmladek@xxxxxxxx> wrote:
>
> On Tue 2020-02-18 01:07:53, Ilya Dryomov wrote:
> > On Tue, Feb 18, 2020 at 12:47 AM Kees Cook <keescook@xxxxxxxxxxxx> wrote:
> > >
> > > On Mon, Feb 17, 2020 at 11:28:03PM +0100, Ilya Dryomov wrote:
> > > > I don't see what security concern is addressed by obfuscating NULL
> > > > and IS_ERR() error pointers, printed with %p/%pK.  Given the number
> > > > of sites where %p is used (over 10000) and the fact that NULL pointers
> > > > aren't uncommon, it probably wouldn't take long for an attacker to
> > > > find the hash that corresponds to 0.  Although harder, the same goes
> > > > for most common error values, such as -1, -2, -11, -14, etc.
> > > >
> > > > The NULL part actually fixes a regression: NULL pointers weren't
> > > > obfuscated until commit 3e5903eb9cff ("vsprintf: Prevent crash when
> > > > dereferencing invalid pointers") which went into 5.2.  I'm tacking
> > > > the IS_ERR() part on here because error pointers won't leak kernel
> > > > addresses and printing them as pointers shouldn't be any different
> > > > from e.g. %d with PTR_ERR_OR_ZERO().  Obfuscating them just makes
> > > > debugging based on existing pr_debug and friends excruciating.
> > > >
> > > > Note that the "always print 0's for %pK when kptr_restrict == 2"
> > > > behaviour which goes way back is left as is.
> > > >
> > > > Example output with the patch applied:
> > > >
> > > >                             ptr         error-ptr              NULL
> > > > %p:            0000000001f8cc5b  fffffffffffffff2  0000000000000000
> > > > %pK, kptr = 0: 0000000001f8cc5b  fffffffffffffff2  0000000000000000
> > > > %px:           ffff888048c04020  fffffffffffffff2  0000000000000000
> > > > %pK, kptr = 1: ffff888048c04020  fffffffffffffff2  0000000000000000
> > > > %pK, kptr = 2: 0000000000000000  0000000000000000  0000000000000000
> > >
> > > This seems reasonable. Though I wonder -- since the efault string is
> > > exposed now -- should this instead print all the error-ptr strings
> > > instead of the unsigned negative pointer value?
>
> It would make sense to distinguish it from a hashed value that might
> be in the NULL or ERR range as well.
>
> The chance is small. But it might safe people from spending time
> on false paths.

Yeah, when the obfuscation patch went in, NULL was "(null)", but
that got dropped later in an argument that "(null)" should only
be printed on dereference attempts and also in an effort to make it
consistent with %pK.  I don't agree with that because "(null)" dated
back to 2009 and %pK has always been a special case, but I decided
to go with the minimal fix to avoid bike shedding.

For error pointers, at least on 64-bit they are easy to distinguish
because the upper 32 bits of the hash are cleared.

Thanks,

                Ilya