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

[Ilya Dryomov]

On Wed, Feb 19, 2020 at 8:23 PM Ilya Dryomov <idryomov@xxxxxxxxx> wrote:
>
> On Wed, Feb 19, 2020 at 7:07 PM Ilya Dryomov <idryomov@xxxxxxxxx> wrote:
> >
> > On Wed, Feb 19, 2020 at 6:37 PM Andy Shevchenko
> > <andy.shevchenko@xxxxxxxxx> wrote:
> > >
> > > On Wed, Feb 19, 2020 at 7:13 PM Ilya Dryomov <idryomov@xxxxxxxxx> 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
> > >
> > > ...
> > >
> > > > +/*
> > > > + * NULL pointers aren't hashed.
> > > > + */
> > > >  static void __init
> > > >  null_pointer(void)
> > > >  {
> > > > -       test_hashed("%p", NULL);
> > > > +       test(ZEROS "00000000", "%p", NULL);
> > > >         test(ZEROS "00000000", "%px", NULL);
> > > >         test("(null)", "%pE", NULL);
> > > >  }
> > > >
> > > > +/*
> > > > + * Error pointers aren't hashed.
> > > > + */
> > > > +static void __init
> > > > +error_pointer(void)
> > > > +{
> > > > +       test(ONES "fffffff5", "%p", ERR_PTR(-EAGAIN));
> > > > +       test(ONES "fffffff5", "%px", ERR_PTR(-EAGAIN));
> > >
> > > > +       test("(efault)", "%pE", ERR_PTR(-EAGAIN));
> > >
> > > Hmm... Is capital E on purpose here?
> >
> > Yes.  It shows that for %pE an error pointer is still invalid.
> > %pe is tested separately, in errptr(), and the output would have
> > been "-EAGAIN".
> >
> > > Maybe we may use something else ('%ph'?) for sake of deviation?
> >
> > If you look at the resulting file, you will see that null_pointer(),
> > error_pointer() and invalid_pointer() exercise the same three variants:
> > %p, %px and %pE.
> >
> > This is somewhat confusing, but there seems to be some disagreement
> > between Pavel and Rasmus as to how the test suite should be structured
> > and I didn't want to attempt to restructure anything in this patch.
>
> Sorry, I meant Petr of course.
>
> Rasmus, who had to deal with mips defining EDQUOT to 1133 by special
> casing that in lib/errname.c, reminded me that error codes are a mess:
> EAGAIN is different on alpha.  Rather than picking another error code
> that is the same on all architectures, let's just use explicit -11.
>
> error_pointer() should be:
>
>         test(ONES "fffffff5", "%p", ERR_PTR(-11));
>         test(ONES "fffffff5", "%px", ERR_PTR(-11));
>         test("(efault)", "%pE", ERR_PTR(-11));
>
> I'll wait for more feedback and respin (or perhaps this can be
> fixed up while applying).

Hi Petr,

Bump, as I don't see this in linux-next or other public branches.
The discussion was split between several threads, revolving around
the vision for how lib/test_printf.c should be structured, but the
fix itself wasn't disputed.

Could you please pick it up for 5.7-rc1?  If you want to restructure
the test suite before adding any new test cases, v1 doesn't have them.
Other than the test cases, the only difference between v1 and v2 is
added reviews and acks.

Thanks,

                Ilya