RE: [kernel-hardening] [RFC V2 4/6] lib: vsprintf: default kptr_restrict to the maximum value

[Roberts, William C]

> -----Original Message-----
> From: keescook@xxxxxxxxxx [mailto:keescook@xxxxxxxxxx] On Behalf Of Kees
> Cook
> Sent: Wednesday, October 4, 2017 9:43 AM
> To: Tobin C. Harding <me@xxxxxxxx>
> Cc: Greg KH <gregkh@xxxxxxxxxxxxxxxxxxx>; Petr Mladek <pmladek@xxxxxxxx>;
> Joe Perches <joe@xxxxxxxxxxx>; Ian Campbell <ijc@xxxxxxxxxxxxxx>; Sergey
> Senozhatsky <sergey.senozhatsky@xxxxxxxxx>; kernel-
> hardening@xxxxxxxxxxxxxxxxxx; LKML <linux-kernel@xxxxxxxxxxxxxxx>; Catalin
> Marinas <catalin.marinas@xxxxxxx>; Will Deacon <will.deacon@xxxxxxx>;
> Steven Rostedt <rostedt@xxxxxxxxxxx>; Roberts, William C
> <william.c.roberts@xxxxxxxxx>; Chris Fries <cfries@xxxxxxxxxx>; Dave Weinstein
> <olorin@xxxxxxxxxx>; Linus Torvalds <torvalds@xxxxxxxxxxxxxxxxxxxx>
> Subject: Re: [kernel-hardening] [RFC V2 4/6] lib: vsprintf: default kptr_restrict to
> the maximum value
> 
> On Sat, Sep 30, 2017 at 5:06 PM, Tobin C. Harding <me@xxxxxxxx> wrote:
> > Set the initial value of kptr_restrict to the maximum setting rather
> > than the minimum setting, to ensure that early boot logging is not
> > leaking information.
> >
> > Signed-off-by: Tobin C. Harding <me@xxxxxxxx>
> > ---
> >  lib/vsprintf.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/lib/vsprintf.c b/lib/vsprintf.c index 0271223..e009325
> > 100644
> > --- a/lib/vsprintf.c
> > +++ b/lib/vsprintf.c
> > @@ -396,7 +396,7 @@ struct printf_spec {  #define FIELD_WIDTH_MAX ((1
> > << 23) - 1)  #define PRECISION_MAX ((1 << 15) - 1)
> >
> > -int kptr_restrict __read_mostly;
> > +int kptr_restrict __read_mostly = 4; /* maximum setting */
> >
> >  /*
> >   * return non-zero if we should cleanse pointers for %p and %pK specifiers.
> 
> I share Linus's concern that making this the unconditional default will break some
> people. The intention here (as stated in the
> changelog) is to cover anything leaked during early boot before sysctl settings can
> change this value. That shouldn't break perf (which should happen after sysctl
> settings), but it _may_ break debugging of early boot. I would hope that nothing
> would be needed there, but if we want to make this more configurable, we may
> want to consider a Kconfig for the default. Perhaps:
> 
> -int kptr_restrict __read_mostly;
> +int kptr_restrict __read_mostly = CONFIG_KPTR_RESTRICT_DEFAULT;
> 
> ...
> 
> +config KPTR_RESTRICT_DEFAULT
> + bool "Avoid leaking kernel pointers to userspace"
> + default 0
> + range 0 4
> + help
> +   This specifies the initial value of the kptr_restrict sysctl, which
> +   controls the level of kernel pointers removed from display
> +   to userspace. 0 = off, 1 = %pK not visible to non-root, 2 = %pK
> +   not visible for any user, 3 = %p also not visible, 4 = physical and
> +   resource addresses also not visible.
> 
> 
> I'd argue that a default of "1" would be a sensible starting place, but that can be a
> separate patch, IMO.
> 

I might be crazy, as often the case, but a  command line option might be useful here
so you can boot the same kernel with Kptr < 4 if you really need to get at any information
hidden by a configure time value of 4.

> -Kees
> 
> --
> Kees Cook
> Pixel Security