Message-ID: <20171018234512.GE31318@eros>
Date:   Thu, 19 Oct 2017 10:45:12 +1100
From:   "Tobin C. Harding" <me@...in.cc>
To:     Kees Cook <keescook@...omium.org>
Cc:     "kernel-hardening@...ts.openwall.com" 
        <kernel-hardening@...ts.openwall.com>,
        "Jason A. Donenfeld" <Jason@...c4.com>,
        Theodore Ts'o <tytso@....edu>,
        Linus Torvalds <torvalds@...ux-foundation.org>,
        Paolo Bonzini <pbonzini@...hat.com>,
        Tycho Andersen <tycho@...ker.com>,
        "Roberts, William C" <william.c.roberts@...el.com>,
        Tejun Heo <tj@...nel.org>,
        Jordan Glover <Golden_Miller83@...tonmail.ch>,
        Greg KH <gregkh@...uxfoundation.org>,
        Petr Mladek <pmladek@...e.com>, Joe Perches <joe@...ches.com>,
        Ian Campbell <ijc@...lion.org.uk>,
        Sergey Senozhatsky <sergey.senozhatsky@...il.com>,
        Catalin Marinas <catalin.marinas@....com>,
        Will Deacon <wilal.deacon@....com>,
        Steven Rostedt <rostedt@...dmis.org>,
        Chris Fries <cfries@...gle.com>,
        Dave Weinstein <olorin@...gle.com>,
        Daniel Micay <danielmicay@...il.com>,
        Djalal Harouni <tixxdz@...il.com>,
        LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v5] printk: hash addresses printed with %p

On Wed, Oct 18, 2017 at 03:31:16PM -0700, Kees Cook wrote:
> On Wed, Oct 18, 2017 at 2:30 PM, Tobin C. Harding <me@...in.cc> wrote:
> > Currently there are many places in the kernel where addresses are being
> > printed using an unadorned %p. Kernel pointers should be printed using
> > %pK allowing some control via the kptr_restrict sysctl. Exposing addresses
> > gives attackers sensitive information about the kernel layout in memory.
> 
> Is it intended for %pK to be covered by the hash as well? (When a
> disallowed user is looking at %pK output, like kallsyms, the same hash
> is seen for all values, rather than just zero -- I assume since the
> value hashed is zero.)

Good catch, thanks. Have fixed for v6, will wait 24 hours before submitting.

> > We can reduce the attack surface by hashing all addresses printed with
> > %p. This will of course break some users, forcing code printing needed
> > addresses to be updated.
> >
> > For what it's worth, usage of unadorned %p can be broken down as
> > follows (thanks to Joe Perches).
> >
> > $ git grep -E '%p[^A-Za-z0-9]' | cut -f1 -d"/" | sort | uniq -c
> >    1084 arch
> >      20 block
> >      10 crypto
> >      32 Documentation
> >    8121 drivers
> >    1221 fs
> >     143 include
> >     101 kernel
> >      69 lib
> >     100 mm
> >    1510 net
> >      40 samples
> >       7 scripts
> >      11 security
> >     166 sound
> >     152 tools
> >       2 virt
> >
> > Add function ptr_to_id() to map an address to a 32 bit unique identifier.
> >
> > Signed-off-by: Tobin C. Harding <me@...in.cc>
> > ---
> >
> > V5:
> >  - Remove spin lock.
> >  - Add Jason A. Donenfeld to CC list by request.
> >  - Add Theodore Ts'o to CC list due to comment on previous version.
> >
> > V4:
> >  - Remove changes to siphash.{ch}
> >  - Do word size check, and return value cast, directly in ptr_to_id().
> >  - Use add_ready_random_callback() to guard call to get_random_bytes()
> >
> > V3:
> >  - Use atomic_xchg() to guard setting [random] key.
> >  - Remove erroneous white space change.
> >
> > V2:
> >  - Use SipHash to do the hashing.
> >
> > The discussion related to this patch has been fragmented. There are
> > three threads associated with this patch. Email threads by subject:
> >
> > [PATCH] printk: hash addresses printed with %p
> > [PATCH 0/3] add %pX specifier
> > [kernel-hardening] [RFC V2 0/6] add more kernel pointer filter options
> >
> >  lib/vsprintf.c | 66 +++++++++++++++++++++++++++++++++++++++++++++++++++++++---
> >  1 file changed, 63 insertions(+), 3 deletions(-)
> >
> > diff --git a/lib/vsprintf.c b/lib/vsprintf.c
> > index 86c3385b9eb3..14d4c6653384 100644
> > --- a/lib/vsprintf.c
> > +++ b/lib/vsprintf.c
> > @@ -33,6 +33,7 @@
> >  #include <linux/uuid.h>
> >  #include <linux/of.h>
> >  #include <net/addrconf.h>
> > +#include <linux/siphash.h>
> >  #ifdef CONFIG_BLOCK
> >  #include <linux/blkdev.h>
> >  #endif
> > @@ -1591,6 +1592,63 @@ char *device_node_string(char *buf, char *end, struct device_node *dn,
> >         return widen_string(buf, buf - buf_start, end, spec);
> >  }
> >
> > +static siphash_key_t ptr_secret __read_mostly;
> > +static atomic_t have_key = ATOMIC_INIT(0);
> > +
> > +static void initialize_ptr_secret(void)
> > +{
> > +       if (atomic_read(&have_key) == 1)
> > +               return;
> > +
> > +       get_random_bytes(&ptr_secret, sizeof(ptr_secret));
> > +       atomic_set(&have_key, 1);
> > +}
> > +
> > +static void schedule_async_key_init(struct random_ready_callback *rdy)
> > +{
> > +       initialize_ptr_secret();
> > +}
> > +
> > +/* Maps a pointer to a 32 bit unique identifier. */
> > +static char *ptr_to_id(char *buf, char *end, void *ptr, struct printf_spec spec)
> > +{
> > +       static struct random_ready_callback random_ready;
> > +       unsigned int hashval;
> > +       int err;
> > +
> > +       if (atomic_read(&have_key) == 0) {
> > +               random_ready.owner = NULL;
> > +               random_ready.func = schedule_async_key_init;
> > +
> > +               err = add_random_ready_callback(&random_ready);
> > +
> > +               switch (err) {
> > +               case 0:
> > +                       return "(pointer value)";
> > +
> > +               case -EALREADY:
> > +                       initialize_ptr_secret();
> > +                       break;
> > +
> > +               default:
> > +                       /* shouldn't get here */
> > +                       return "(ptr_to_id() error)";
> > +               }
> > +       }
> > +
> > +#ifdef CONFIG_64BIT
> > +       hashval = (unsigned int)siphash_1u64((u64)ptr, &ptr_secret);
> > +#else
> > +       hashval = (unsigned int)siphash_1u32((u32)ptr, &ptr_secret);
> > +#endif
> > +
> > +       spec.field_width = 2 + 2 * sizeof(unsigned int); /* 0x + hex */
> > +       spec.flags = SPECIAL | SMALL | ZEROPAD;
> 
> I don't think this should have SPECIAL. We end up changing things like
> kallsyms (which didn't have 0x before) and printing with double 0x's:

While on the topic, have you an opinion on whether SMALL is good here. My first thought was that
capitals _kind_of_ showed that it was an ID not an address, later contemplation made me think this
may only have meaning to myself from working on the patch so better to leave it SMALL like original.

Any thoughts appreciated.

> 
>         seq_printf(m, " 0x%pK", mod->core_layout.base);
> ...
> # cat /proc/modules
> test_module 16384 0 - Live 0x0xdf81cfb6
> 
> > +       spec.base = 16;
> > +
> > +       return number(buf, end, hashval, spec);
> > +}
> > +
> >  int kptr_restrict __read_mostly;
> >
> >  /*
> > @@ -1703,6 +1761,9 @@ int kptr_restrict __read_mostly;
> >   * Note: The difference between 'S' and 'F' is that on ia64 and ppc64
> >   * function pointers are really function descriptors, which contain a
> >   * pointer to the real address.
> > + *
> > + * Default behaviour (unadorned %p) is to hash the address, rendering it useful
> > + * as a unique identifier.
> >   */
> >  static noinline_for_stack
> >  char *pointer(const char *fmt, char *buf, char *end, void *ptr,
> > @@ -1858,14 +1919,13 @@ char *pointer(const char *fmt, char *buf, char *end, void *ptr,
> >                         return device_node_string(buf, end, ptr, spec, fmt + 1);
> >                 }
> >         }
> > -       spec.flags |= SMALL;
> > +
> >         if (spec.field_width == -1) {
> >                 spec.field_width = default_width;
> >                 spec.flags |= ZEROPAD;
> >         }
> > -       spec.base = 16;
> >
> > -       return number(buf, end, (unsigned long) ptr, spec);
> > +       return ptr_to_id(buf, end, ptr, spec);
> >  }
> >
> >  /*
> > --
> > 2.7.4
> >
> 
> Getting closer! Thanks for continuing to work on it. :)

We are having fun here! Cheers Kees.

Tobin.

Powered by blists - more mailing lists