Re: [kernel-hardening] Re: [PATCH v2] printk: hash addresses printed with %p

[Theodore Ts'o]

On Wed, Oct 18, 2017 at 01:28:05PM +1100, Tobin C. Harding wrote:
> > >> Does %p[FfSs] leak addresses? Well, I guess it does if they are not
> > >> found in kallsyms, but otherwise you have:
> > >>
> > >>   function+0x
> > >
> > 
> > They haven't traditionally been a big deal. If they turn out to be
> > problematic, we can deal with it then, IMO.

If it's not in kallsyms, the raw address is probably not going to be
terribly useful --- so even if it's not traditionally been a big deal,
why not just hash them if it's not going to be printed as "function+0x"?

If nothing else, it will help correlate the random address with other
places where it was printed via %p.

- Ted

Re: [kernel-hardening] Re: [PATCH v2] printk: hash addresses printed with %p

[Theodore Ts'o]

On Wed, Oct 18, 2017 at 01:28:05PM +1100, Tobin C. Harding wrote:
> > >> Does %p[FfSs] leak addresses? Well, I guess it does if they are not
> > >> found in kallsyms, but otherwise you have:
> > >>
> > >>   function+0x
> > >
> > 
> > They haven't traditionally been a big deal. If they turn out to be
> > problematic, we can deal with it then, IMO.

If it's not in kallsyms, the raw address is probably not going to be
terribly useful --- so even if it's not traditionally been a big deal,
why not just hash them if it's not going to be printed as "function+0x"?

If nothing else, it will help correlate the random address with other
places where it was printed via %p.

- Ted

Re: [PATCH v2] printk: hash addresses printed with %p

[Tobin C. Harding]

On Tue, Oct 17, 2017 at 05:13:10PM -0700, Kees Cook wrote:
> On Tue, Oct 17, 2017 at 4:15 PM, Tobin C. Harding  wrote:
> > On Tue, Oct 17, 2017 at 09:31:19AM -0400, Steven Rostedt wrote:
> >> On Tue, 17 Oct 2017 15:52:51 +1100
> >> "Tobin C. Harding"  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.
> >> >
> >> > 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
> >> >
> >> > git grep '%p[^KFfSsBRrbMmIiEUVKNhdDgCGO]' | wc -l
> >>
> >> Does %p[FfSs] leak addresses? Well, I guess it does if they are not
> >> found in kallsyms, but otherwise you have:
> >>
> >>   function+0x
> >
> > You are correct %pF and %pS print an offset. Does this provide an attack 
> > vector,
> > I didn't think so but I'm no security expert. If they do then we need to 
> > amend
> > those calls also.
> 
> They haven't traditionally been a big deal. If they turn out to be
> problematic, we can deal with it then, IMO.

Thanks Kees,
Tobin.

Re: [PATCH v2] printk: hash addresses printed with %p

[Tobin C. Harding]

On Tue, Oct 17, 2017 at 05:13:10PM -0700, Kees Cook wrote:
> On Tue, Oct 17, 2017 at 4:15 PM, Tobin C. Harding  wrote:
> > On Tue, Oct 17, 2017 at 09:31:19AM -0400, Steven Rostedt wrote:
> >> On Tue, 17 Oct 2017 15:52:51 +1100
> >> "Tobin C. Harding"  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.
> >> >
> >> > 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
> >> >
> >> > git grep '%p[^KFfSsBRrbMmIiEUVKNhdDgCGO]' | wc -l
> >>
> >> Does %p[FfSs] leak addresses? Well, I guess it does if they are not
> >> found in kallsyms, but otherwise you have:
> >>
> >>   function+0x
> >
> > You are correct %pF and %pS print an offset. Does this provide an attack 
> > vector,
> > I didn't think so but I'm no security expert. If they do then we need to 
> > amend
> > those calls also.
> 
> They haven't traditionally been a big deal. If they turn out to be
> problematic, we can deal with it then, IMO.

Thanks Kees,
Tobin.

Re: [PATCH v2] printk: hash addresses printed with %p

[Tobin C. Harding]

On Wed, Oct 18, 2017 at 02:27:43AM +0200, Jason A. Donenfeld wrote:
[snip]

Thank you for your extensive comments Jason. I had v3 in flight before I 
received your email, please
don't think I ignored your suggestions.

v4 to come!

thanks,
Tobin.

Re: [PATCH v2] printk: hash addresses printed with %p

[Tobin C. Harding]

On Wed, Oct 18, 2017 at 02:27:43AM +0200, Jason A. Donenfeld wrote:
[snip]

Thank you for your extensive comments Jason. I had v3 in flight before I 
received your email, please
don't think I ignored your suggestions.

v4 to come!

thanks,
Tobin.

Re: [PATCH v2] printk: hash addresses printed with %p

[Steven Rostedt]

On Wed, 18 Oct 2017 10:15:59 +1100
"Tobin C. Harding"  wrote:

> > Does %p[FfSs] leak addresses? Well, I guess it does if they are not
> > found in kallsyms, but otherwise you have:
> > 
> >   function+0x  
> 
> You are correct %pF and %pS print an offset. Does this provide an attack 
> vector,
> I didn't think so but I'm no security expert. If they do then we need to amend
> those calls also.

Hopefully not. We changed stack dumps to use them only instead of
showing addresses because of the location leak.

-- Steve

Re: [PATCH v2] printk: hash addresses printed with %p

[Steven Rostedt]

On Wed, 18 Oct 2017 10:15:59 +1100
"Tobin C. Harding"  wrote:

> > Does %p[FfSs] leak addresses? Well, I guess it does if they are not
> > found in kallsyms, but otherwise you have:
> > 
> >   function+0x  
> 
> You are correct %pF and %pS print an offset. Does this provide an attack 
> vector,
> I didn't think so but I'm no security expert. If they do then we need to amend
> those calls also.

Hopefully not. We changed stack dumps to use them only instead of
showing addresses because of the location leak.

-- Steve

Re: [PATCH v2] printk: hash addresses printed with %p

[Jason A. Donenfeld]

Hi Tobin,

Many comments in line below.

On Tue, Oct 17, 2017 at 6:52 AM, Tobin C. Harding  wrote:
>
> diff --git a/include/linux/siphash.h b/include/linux/siphash.h
> index fa7a6b9cedbf..a9392568c8b8 100644
> --- a/include/linux/siphash.h
> +++ b/include/linux/siphash.h
> @@ -26,6 +26,8 @@ u64 __siphash_aligned(const void *data, size_t len, const 
> siphash_key_t *key);
>  u64 __siphash_unaligned(const void *data, size_t len, const siphash_key_t 
> *key);
>  #endif
>
> +unsigned long siphash_1ulong(const unsigned long a, const siphash_key_t 
> *key);

This signature is incorrect, as siphash always returns a u64. The
caller should do the casting, not the actual function itself.
[However, see below. I don't think you should be touching this file.]

>  u64 siphash_1u64(const u64 a, const siphash_key_t *key);
>  u64 siphash_2u64(const u64 a, const u64 b, const siphash_key_t *key);
>  u64 siphash_3u64(const u64 a, const u64 b, const u64 c,
> diff --git a/lib/siphash.c b/lib/siphash.c
> index 3ae58b4edad6..63f4ff57c9ce 100644
> --- a/lib/siphash.c
> +++ b/lib/siphash.c
> @@ -116,6 +116,19 @@ EXPORT_SYMBOL(__siphash_unaligned);
>  #endif
>
>  /**
> + * siphash_1ulong - computes siphash PRF value
> + * @first: value to hash
> + * @key: the siphash key
> + */

Please match the template usage text of every single other function, like so:

* siphash_1ulong - compute 64-bit siphash PRF value of 1 unsigned long
* @first: first unsigned long
* @key: the siphash key

[However, see below. I don't think you should be touching this file.]


> +unsigned long siphash_1ulong(const unsigned long first, const siphash_key_t 
> *key)

Return u64. [However, see below. I don't think you should be touching
this file.]

> +{
> +#ifdef CONFIG_64BIT
> +   return (unsigned long)siphash_1u64((u64)first, key);

Don't cast it here. [However, see below. I don't think you should be
touching this file.]

> +#endif

There's no point in making gcc's life harder. Use an #else for the
32-bit section. [However, see below. I don't think you should be
touching this file.]


> +   return (unsigned long)siphash_1u32((u32)first, key);

Also don't cast. [However, see below. I don't think you should be
touching this file.]

> +/* 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 siphash_key_t ptr_secret __read_mostly;
> +   static bool have_key = false;
> +   unsigned long hashval;
> +
> +   /* Kernel doesn't boot if we use get_random_once() */
> +   if (!have_key) {
> +   get_random_bytes(_secret, sizeof(ptr_secret));
> +   have_key = true;
> +   }

This is wrong. You need to either use get_random_bytes_wait, which you
can't actually do safely here. So, better, use
add_random_ready_callback to get a notification of when this is safe
to use. Before it's safe to use, simply return "(ptr value)" or some
similar stub.

> +
> +   hashval = siphash_1ulong((unsigned long)ptr, _secret);

As mentioned above with the [brackets], don't pollute
siphash.h/siphash.c with the helper, and just put the #ifdef stuff
here. That should make it much more clear what's going on and also
make it easier in the future to swap out the 32-bit function when
we're ready.

So, this looks like instead:

#ifdef CONFIG_64BIT
hashval = (unsigned long)siphash_1u64((u64)ptr, key);
#else
hashval = (unsigned long)siphash_1u32((u32)ptr, key);
#endif

However, in another thread, Linus mentioned that he'd prefer all the
obfuscated values actually be 32-bit. So, this then looks like:

unsigned int hashval;
...
#ifdef CONFIG_64BIT
hashval = (unsigned int)siphash_1u64((u64)ptr, key);
#else
hashval = (unsigned int)siphash_1u32((u32)ptr, key);
#endif


Looking forward to v3!

Thanks,
Jason

Re: [PATCH v2] printk: hash addresses printed with %p

[Jason A. Donenfeld]

Hi Tobin,

Many comments in line below.

On Tue, Oct 17, 2017 at 6:52 AM, Tobin C. Harding  wrote:
>
> diff --git a/include/linux/siphash.h b/include/linux/siphash.h
> index fa7a6b9cedbf..a9392568c8b8 100644
> --- a/include/linux/siphash.h
> +++ b/include/linux/siphash.h
> @@ -26,6 +26,8 @@ u64 __siphash_aligned(const void *data, size_t len, const 
> siphash_key_t *key);
>  u64 __siphash_unaligned(const void *data, size_t len, const siphash_key_t 
> *key);
>  #endif
>
> +unsigned long siphash_1ulong(const unsigned long a, const siphash_key_t 
> *key);

This signature is incorrect, as siphash always returns a u64. The
caller should do the casting, not the actual function itself.
[However, see below. I don't think you should be touching this file.]

>  u64 siphash_1u64(const u64 a, const siphash_key_t *key);
>  u64 siphash_2u64(const u64 a, const u64 b, const siphash_key_t *key);
>  u64 siphash_3u64(const u64 a, const u64 b, const u64 c,
> diff --git a/lib/siphash.c b/lib/siphash.c
> index 3ae58b4edad6..63f4ff57c9ce 100644
> --- a/lib/siphash.c
> +++ b/lib/siphash.c
> @@ -116,6 +116,19 @@ EXPORT_SYMBOL(__siphash_unaligned);
>  #endif
>
>  /**
> + * siphash_1ulong - computes siphash PRF value
> + * @first: value to hash
> + * @key: the siphash key
> + */

Please match the template usage text of every single other function, like so:

* siphash_1ulong - compute 64-bit siphash PRF value of 1 unsigned long
* @first: first unsigned long
* @key: the siphash key

[However, see below. I don't think you should be touching this file.]


> +unsigned long siphash_1ulong(const unsigned long first, const siphash_key_t 
> *key)

Return u64. [However, see below. I don't think you should be touching
this file.]

> +{
> +#ifdef CONFIG_64BIT
> +   return (unsigned long)siphash_1u64((u64)first, key);

Don't cast it here. [However, see below. I don't think you should be
touching this file.]

> +#endif

There's no point in making gcc's life harder. Use an #else for the
32-bit section. [However, see below. I don't think you should be
touching this file.]


> +   return (unsigned long)siphash_1u32((u32)first, key);

Also don't cast. [However, see below. I don't think you should be
touching this file.]

> +/* 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 siphash_key_t ptr_secret __read_mostly;
> +   static bool have_key = false;
> +   unsigned long hashval;
> +
> +   /* Kernel doesn't boot if we use get_random_once() */
> +   if (!have_key) {
> +   get_random_bytes(_secret, sizeof(ptr_secret));
> +   have_key = true;
> +   }

This is wrong. You need to either use get_random_bytes_wait, which you
can't actually do safely here. So, better, use
add_random_ready_callback to get a notification of when this is safe
to use. Before it's safe to use, simply return "(ptr value)" or some
similar stub.

> +
> +   hashval = siphash_1ulong((unsigned long)ptr, _secret);

As mentioned above with the [brackets], don't pollute
siphash.h/siphash.c with the helper, and just put the #ifdef stuff
here. That should make it much more clear what's going on and also
make it easier in the future to swap out the 32-bit function when
we're ready.

So, this looks like instead:

#ifdef CONFIG_64BIT
hashval = (unsigned long)siphash_1u64((u64)ptr, key);
#else
hashval = (unsigned long)siphash_1u32((u32)ptr, key);
#endif

However, in another thread, Linus mentioned that he'd prefer all the
obfuscated values actually be 32-bit. So, this then looks like:

unsigned int hashval;
...
#ifdef CONFIG_64BIT
hashval = (unsigned int)siphash_1u64((u64)ptr, key);
#else
hashval = (unsigned int)siphash_1u32((u32)ptr, key);
#endif


Looking forward to v3!

Thanks,
Jason

Re: [PATCH v2] printk: hash addresses printed with %p

[Kees Cook]

On Tue, Oct 17, 2017 at 4:15 PM, Tobin C. Harding  wrote:
> On Tue, Oct 17, 2017 at 09:31:19AM -0400, Steven Rostedt wrote:
>> On Tue, 17 Oct 2017 15:52:51 +1100
>> "Tobin C. Harding"  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.
>> >
>> > 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
>> >
>> > git grep '%p[^KFfSsBRrbMmIiEUVKNhdDgCGO]' | wc -l
>>
>> Does %p[FfSs] leak addresses? Well, I guess it does if they are not
>> found in kallsyms, but otherwise you have:
>>
>>   function+0x
>
> You are correct %pF and %pS print an offset. Does this provide an attack 
> vector,
> I didn't think so but I'm no security expert. If they do then we need to amend
> those calls also.

They haven't traditionally been a big deal. If they turn out to be
problematic, we can deal with it then, IMO.

-Kees

-- 
Kees Cook
Pixel Security

Re: [PATCH v2] printk: hash addresses printed with %p

[Kees Cook]

On Tue, Oct 17, 2017 at 4:15 PM, Tobin C. Harding  wrote:
> On Tue, Oct 17, 2017 at 09:31:19AM -0400, Steven Rostedt wrote:
>> On Tue, 17 Oct 2017 15:52:51 +1100
>> "Tobin C. Harding"  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.
>> >
>> > 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
>> >
>> > git grep '%p[^KFfSsBRrbMmIiEUVKNhdDgCGO]' | wc -l
>>
>> Does %p[FfSs] leak addresses? Well, I guess it does if they are not
>> found in kallsyms, but otherwise you have:
>>
>>   function+0x
>
> You are correct %pF and %pS print an offset. Does this provide an attack 
> vector,
> I didn't think so but I'm no security expert. If they do then we need to amend
> those calls also.

They haven't traditionally been a big deal. If they turn out to be
problematic, we can deal with it then, IMO.

-Kees

-- 
Kees Cook
Pixel Security

Re: [PATCH v2] printk: hash addresses printed with %p

[Tobin C. Harding]

On Tue, Oct 17, 2017 at 09:31:19AM -0400, Steven Rostedt wrote:
> On Tue, 17 Oct 2017 15:52:51 +1100
> "Tobin C. Harding"  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.
> > 
> > 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
> > 
> > git grep '%p[^KFfSsBRrbMmIiEUVKNhdDgCGO]' | wc -l
> 
> Does %p[FfSs] leak addresses? Well, I guess it does if they are not
> found in kallsyms, but otherwise you have:
> 
>   function+0x

You are correct %pF and %pS print an offset. Does this provide an attack vector,
I didn't think so but I'm no security expert. If they do then we need to amend
those calls also.

We still have %pa[pd] to see to as well obviously.

thanks for the review,
Tobin.

Re: [PATCH v2] printk: hash addresses printed with %p

[Tobin C. Harding]

On Tue, Oct 17, 2017 at 09:31:19AM -0400, Steven Rostedt wrote:
> On Tue, 17 Oct 2017 15:52:51 +1100
> "Tobin C. Harding"  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.
> > 
> > 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
> > 
> > git grep '%p[^KFfSsBRrbMmIiEUVKNhdDgCGO]' | wc -l
> 
> Does %p[FfSs] leak addresses? Well, I guess it does if they are not
> found in kallsyms, but otherwise you have:
> 
>   function+0x

You are correct %pF and %pS print an offset. Does this provide an attack vector,
I didn't think so but I'm no security expert. If they do then we need to amend
those calls also.

We still have %pa[pd] to see to as well obviously.

thanks for the review,
Tobin.

Re: [PATCH v2] printk: hash addresses printed with %p

[Tobin C. Harding]

On Tue, Oct 17, 2017 at 05:27:15PM +, Roberts, William C wrote:
> 
> 
> > -Original Message-
> > From: Tobin C. Harding [mailto:m...@tobin.cc]
> > Sent: Monday, October 16, 2017 9:53 PM
> > To: kernel-harden...@lists.openwall.com
> > Cc: Tobin C. Harding ; Linus Torvalds  > foundation.org>; Kees Cook ; Paolo Bonzini
> > ; Tycho Andersen ; Roberts,
> > William C ; Tejun Heo ; Jordan
> > Glover ; Greg KH
> > ; Petr Mladek ; Joe
> > Perches ; Ian Campbell ; Sergey
> > Senozhatsky ; Catalin Marinas
> > ; Will Deacon ; Steven
> > Rostedt ; Chris Fries ; Dave
> > Weinstein ; Daniel Micay ; Djalal
> > Harouni ; linux-kernel@vger.kernel.org
> > Subject: [PATCH v2] printk: hash addresses printed with %p
> > 
> > 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.
> > 
> > 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
> > 
> > git grep '%p[^KFfSsBRrbMmIiEUVKNhdDgCGO]' | wc -l
> > 
> > arch: 2512
> > block: 20
> > crypto: 12
> > fs: 1221
> > include: 147
> > kernel: 109
> > lib: 77
> > mm: 120
> > net: 1516
> > security: 11
> > sound: 168
> > virt: 2
> > drivers: 8420
> > 
> > Add helper function siphash_1ulong(). Add function ptr_to_id() to map an
> > address to a 32 bit unique identifier.
> > 
> > Signed-off-by: Tobin C. Harding 
> > ---
> > 
> > V2:
> >  - Use SipHash to do the hashing
> > 
> > The discussion related to this patch has been fragmented. There are three 
> > other
> > 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
> > 
> >  include/linux/siphash.h |  2 ++
> >  lib/siphash.c   | 13 +
> >  lib/vsprintf.c  | 32 +---
> >  3 files changed, 44 insertions(+), 3 deletions(-)
> > 
> > diff --git a/include/linux/siphash.h b/include/linux/siphash.h index
> > fa7a6b9cedbf..a9392568c8b8 100644
> > --- a/include/linux/siphash.h
> > +++ b/include/linux/siphash.h
> > @@ -26,6 +26,8 @@ u64 __siphash_aligned(const void *data, size_t len, const
> > siphash_key_t *key);
> >  u64 __siphash_unaligned(const void *data, size_t len, const siphash_key_t
> > *key);  #endif
> > 
> > +unsigned long siphash_1ulong(const unsigned long a, const siphash_key_t
> > +*key);
> > +
> >  u64 siphash_1u64(const u64 a, const siphash_key_t *key);
> >  u64 siphash_2u64(const u64 a, const u64 b, const siphash_key_t *key);
> >  u64 siphash_3u64(const u64 a, const u64 b, const u64 c, diff --git 
> > a/lib/siphash.c
> > b/lib/siphash.c index 3ae58b4edad6..63f4ff57c9ce 100644
> > --- a/lib/siphash.c
> > +++ b/lib/siphash.c
> > @@ -116,6 +116,19 @@ EXPORT_SYMBOL(__siphash_unaligned);
> >  #endif
> > 
> >  /**
> > + * siphash_1ulong - computes siphash PRF value
> > + * @first: value to hash
> > + * @key: the siphash key
> > + */
> > +unsigned long siphash_1ulong(const unsigned long first, const
> > +siphash_key_t *key) { #ifdef CONFIG_64BIT
> > +   return (unsigned long)siphash_1u64((u64)first, key); #endif
> > +   return (unsigned long)siphash_1u32((u32)first, key); }
> > +
> > +/**
> >   * siphash_1u64 - compute 64-bit siphash PRF value of a u64
> >   * @first: first u64
> >   * @key: the siphash key
> > diff --git a/lib/vsprintf.c b/lib/vsprintf.c index 
> > 86c3385b9eb3..afd1c835b0f6 100644
> > --- a/lib/vsprintf.c
> > +++ b/lib/vsprintf.c
> > @@ -33,6 +33,7 @@
> >  #include 
> >  #include 
> >  #include 
> > +#include 
> >  #ifdef CONFIG_BLOCK
> >  #include 
> >  #endif
> > @@ -503,6 +504,7 @@ char *number(char *buf, char *end, unsigned long long
> > num,
> > *buf = '0';
> > ++buf;
> > }
> > +
> 
> Unneeded whitespace change?

:) thanks

> 
> > /* actual digits of result */
> > while (--i >= 0) {
> > if (buf < end)
> > @@ -1591,6 +1593,28 @@ char *device_node_string(char *buf, char *end, struct
> > device_node *dn,
> > return widen_string(buf, buf - buf_start, end, spec);  }
> > 
> > +/* Maps a pointer to a 32 bit unique 

Re: [PATCH v2] printk: hash addresses printed with %p

[Tobin C. Harding]

On Tue, Oct 17, 2017 at 05:27:15PM +, Roberts, William C wrote:
> 
> 
> > -Original Message-
> > From: Tobin C. Harding [mailto:m...@tobin.cc]
> > Sent: Monday, October 16, 2017 9:53 PM
> > To: kernel-harden...@lists.openwall.com
> > Cc: Tobin C. Harding ; Linus Torvalds  > foundation.org>; Kees Cook ; Paolo Bonzini
> > ; Tycho Andersen ; Roberts,
> > William C ; Tejun Heo ; Jordan
> > Glover ; Greg KH
> > ; Petr Mladek ; Joe
> > Perches ; Ian Campbell ; Sergey
> > Senozhatsky ; Catalin Marinas
> > ; Will Deacon ; Steven
> > Rostedt ; Chris Fries ; Dave
> > Weinstein ; Daniel Micay ; Djalal
> > Harouni ; linux-kernel@vger.kernel.org
> > Subject: [PATCH v2] printk: hash addresses printed with %p
> > 
> > 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.
> > 
> > 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
> > 
> > git grep '%p[^KFfSsBRrbMmIiEUVKNhdDgCGO]' | wc -l
> > 
> > arch: 2512
> > block: 20
> > crypto: 12
> > fs: 1221
> > include: 147
> > kernel: 109
> > lib: 77
> > mm: 120
> > net: 1516
> > security: 11
> > sound: 168
> > virt: 2
> > drivers: 8420
> > 
> > Add helper function siphash_1ulong(). Add function ptr_to_id() to map an
> > address to a 32 bit unique identifier.
> > 
> > Signed-off-by: Tobin C. Harding 
> > ---
> > 
> > V2:
> >  - Use SipHash to do the hashing
> > 
> > The discussion related to this patch has been fragmented. There are three 
> > other
> > 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
> > 
> >  include/linux/siphash.h |  2 ++
> >  lib/siphash.c   | 13 +
> >  lib/vsprintf.c  | 32 +---
> >  3 files changed, 44 insertions(+), 3 deletions(-)
> > 
> > diff --git a/include/linux/siphash.h b/include/linux/siphash.h index
> > fa7a6b9cedbf..a9392568c8b8 100644
> > --- a/include/linux/siphash.h
> > +++ b/include/linux/siphash.h
> > @@ -26,6 +26,8 @@ u64 __siphash_aligned(const void *data, size_t len, const
> > siphash_key_t *key);
> >  u64 __siphash_unaligned(const void *data, size_t len, const siphash_key_t
> > *key);  #endif
> > 
> > +unsigned long siphash_1ulong(const unsigned long a, const siphash_key_t
> > +*key);
> > +
> >  u64 siphash_1u64(const u64 a, const siphash_key_t *key);
> >  u64 siphash_2u64(const u64 a, const u64 b, const siphash_key_t *key);
> >  u64 siphash_3u64(const u64 a, const u64 b, const u64 c, diff --git 
> > a/lib/siphash.c
> > b/lib/siphash.c index 3ae58b4edad6..63f4ff57c9ce 100644
> > --- a/lib/siphash.c
> > +++ b/lib/siphash.c
> > @@ -116,6 +116,19 @@ EXPORT_SYMBOL(__siphash_unaligned);
> >  #endif
> > 
> >  /**
> > + * siphash_1ulong - computes siphash PRF value
> > + * @first: value to hash
> > + * @key: the siphash key
> > + */
> > +unsigned long siphash_1ulong(const unsigned long first, const
> > +siphash_key_t *key) { #ifdef CONFIG_64BIT
> > +   return (unsigned long)siphash_1u64((u64)first, key); #endif
> > +   return (unsigned long)siphash_1u32((u32)first, key); }
> > +
> > +/**
> >   * siphash_1u64 - compute 64-bit siphash PRF value of a u64
> >   * @first: first u64
> >   * @key: the siphash key
> > diff --git a/lib/vsprintf.c b/lib/vsprintf.c index 
> > 86c3385b9eb3..afd1c835b0f6 100644
> > --- a/lib/vsprintf.c
> > +++ b/lib/vsprintf.c
> > @@ -33,6 +33,7 @@
> >  #include 
> >  #include 
> >  #include 
> > +#include 
> >  #ifdef CONFIG_BLOCK
> >  #include 
> >  #endif
> > @@ -503,6 +504,7 @@ char *number(char *buf, char *end, unsigned long long
> > num,
> > *buf = '0';
> > ++buf;
> > }
> > +
> 
> Unneeded whitespace change?

:) thanks

> 
> > /* actual digits of result */
> > while (--i >= 0) {
> > if (buf < end)
> > @@ -1591,6 +1593,28 @@ char *device_node_string(char *buf, char *end, struct
> > device_node *dn,
> > return widen_string(buf, buf - buf_start, end, spec);  }
> > 
> > +/* 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 siphash_key_t ptr_secret __read_mostly;
> > +   static bool have_key = false;
> > +   unsigned long hashval;
> > +
> > +   /* Kernel doesn't boot if we use get_random_once() */
> > +   if (!have_key) {
> > +   get_random_bytes(_secret, sizeof(ptr_secret));
> > +   have_key = true;
> 
> Wouldn't one want to use an 

RE: [PATCH v2] printk: hash addresses printed with %p

[Roberts, William C]

> -Original Message-
> From: Tobin C. Harding [mailto:m...@tobin.cc]
> Sent: Monday, October 16, 2017 9:53 PM
> To: kernel-harden...@lists.openwall.com
> Cc: Tobin C. Harding ; Linus Torvalds  foundation.org>; Kees Cook ; Paolo Bonzini
> ; Tycho Andersen ; Roberts,
> William C ; Tejun Heo ; Jordan
> Glover ; Greg KH
> ; Petr Mladek ; Joe
> Perches ; Ian Campbell ; Sergey
> Senozhatsky ; Catalin Marinas
> ; Will Deacon ; Steven
> Rostedt ; Chris Fries ; Dave
> Weinstein ; Daniel Micay ; Djalal
> Harouni ; linux-kernel@vger.kernel.org
> Subject: [PATCH v2] printk: hash addresses printed with %p
> 
> 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.
> 
> 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
> 
> git grep '%p[^KFfSsBRrbMmIiEUVKNhdDgCGO]' | wc -l
> 
> arch: 2512
> block: 20
> crypto: 12
> fs: 1221
> include: 147
> kernel: 109
> lib: 77
> mm: 120
> net: 1516
> security: 11
> sound: 168
> virt: 2
> drivers: 8420
> 
> Add helper function siphash_1ulong(). Add function ptr_to_id() to map an
> address to a 32 bit unique identifier.
> 
> Signed-off-by: Tobin C. Harding 
> ---
> 
> V2:
>  - Use SipHash to do the hashing
> 
> The discussion related to this patch has been fragmented. There are three 
> other
> 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
> 
>  include/linux/siphash.h |  2 ++
>  lib/siphash.c   | 13 +
>  lib/vsprintf.c  | 32 +---
>  3 files changed, 44 insertions(+), 3 deletions(-)
> 
> diff --git a/include/linux/siphash.h b/include/linux/siphash.h index
> fa7a6b9cedbf..a9392568c8b8 100644
> --- a/include/linux/siphash.h
> +++ b/include/linux/siphash.h
> @@ -26,6 +26,8 @@ u64 __siphash_aligned(const void *data, size_t len, const
> siphash_key_t *key);
>  u64 __siphash_unaligned(const void *data, size_t len, const siphash_key_t
> *key);  #endif
> 
> +unsigned long siphash_1ulong(const unsigned long a, const siphash_key_t
> +*key);
> +
>  u64 siphash_1u64(const u64 a, const siphash_key_t *key);
>  u64 siphash_2u64(const u64 a, const u64 b, const siphash_key_t *key);
>  u64 siphash_3u64(const u64 a, const u64 b, const u64 c, diff --git 
> a/lib/siphash.c
> b/lib/siphash.c index 3ae58b4edad6..63f4ff57c9ce 100644
> --- a/lib/siphash.c
> +++ b/lib/siphash.c
> @@ -116,6 +116,19 @@ EXPORT_SYMBOL(__siphash_unaligned);
>  #endif
> 
>  /**
> + * siphash_1ulong - computes siphash PRF value
> + * @first: value to hash
> + * @key: the siphash key
> + */
> +unsigned long siphash_1ulong(const unsigned long first, const
> +siphash_key_t *key) { #ifdef CONFIG_64BIT
> + return (unsigned long)siphash_1u64((u64)first, key); #endif
> + return (unsigned long)siphash_1u32((u32)first, key); }
> +
> +/**
>   * siphash_1u64 - compute 64-bit siphash PRF value of a u64
>   * @first: first u64
>   * @key: the siphash key
> diff --git a/lib/vsprintf.c b/lib/vsprintf.c index 86c3385b9eb3..afd1c835b0f6 
> 100644
> --- a/lib/vsprintf.c
> +++ b/lib/vsprintf.c
> @@ -33,6 +33,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>  #ifdef CONFIG_BLOCK
>  #include 
>  #endif
> @@ -503,6 +504,7 @@ char *number(char *buf, char *end, unsigned long long
> num,
>   *buf = '0';
>   ++buf;
>   }
> +

Unneeded whitespace change?

>   /* actual digits of result */
>   while (--i >= 0) {
>   if (buf < end)
> @@ -1591,6 +1593,28 @@ char *device_node_string(char *buf, char *end, struct
> device_node *dn,
>   return widen_string(buf, buf - buf_start, end, spec);  }
> 
> +/* 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 siphash_key_t ptr_secret __read_mostly;
> + static bool have_key = false;
> + unsigned long hashval;
> +
> + /* Kernel doesn't boot if we use get_random_once() */
> + if (!have_key) {
> + 

RE: [PATCH v2] printk: hash addresses printed with %p

[Roberts, William C]

> -Original Message-
> From: Tobin C. Harding [mailto:m...@tobin.cc]
> Sent: Monday, October 16, 2017 9:53 PM
> To: kernel-harden...@lists.openwall.com
> Cc: Tobin C. Harding ; Linus Torvalds  foundation.org>; Kees Cook ; Paolo Bonzini
> ; Tycho Andersen ; Roberts,
> William C ; Tejun Heo ; Jordan
> Glover ; Greg KH
> ; Petr Mladek ; Joe
> Perches ; Ian Campbell ; Sergey
> Senozhatsky ; Catalin Marinas
> ; Will Deacon ; Steven
> Rostedt ; Chris Fries ; Dave
> Weinstein ; Daniel Micay ; Djalal
> Harouni ; linux-kernel@vger.kernel.org
> Subject: [PATCH v2] printk: hash addresses printed with %p
> 
> 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.
> 
> 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
> 
> git grep '%p[^KFfSsBRrbMmIiEUVKNhdDgCGO]' | wc -l
> 
> arch: 2512
> block: 20
> crypto: 12
> fs: 1221
> include: 147
> kernel: 109
> lib: 77
> mm: 120
> net: 1516
> security: 11
> sound: 168
> virt: 2
> drivers: 8420
> 
> Add helper function siphash_1ulong(). Add function ptr_to_id() to map an
> address to a 32 bit unique identifier.
> 
> Signed-off-by: Tobin C. Harding 
> ---
> 
> V2:
>  - Use SipHash to do the hashing
> 
> The discussion related to this patch has been fragmented. There are three 
> other
> 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
> 
>  include/linux/siphash.h |  2 ++
>  lib/siphash.c   | 13 +
>  lib/vsprintf.c  | 32 +---
>  3 files changed, 44 insertions(+), 3 deletions(-)
> 
> diff --git a/include/linux/siphash.h b/include/linux/siphash.h index
> fa7a6b9cedbf..a9392568c8b8 100644
> --- a/include/linux/siphash.h
> +++ b/include/linux/siphash.h
> @@ -26,6 +26,8 @@ u64 __siphash_aligned(const void *data, size_t len, const
> siphash_key_t *key);
>  u64 __siphash_unaligned(const void *data, size_t len, const siphash_key_t
> *key);  #endif
> 
> +unsigned long siphash_1ulong(const unsigned long a, const siphash_key_t
> +*key);
> +
>  u64 siphash_1u64(const u64 a, const siphash_key_t *key);
>  u64 siphash_2u64(const u64 a, const u64 b, const siphash_key_t *key);
>  u64 siphash_3u64(const u64 a, const u64 b, const u64 c, diff --git 
> a/lib/siphash.c
> b/lib/siphash.c index 3ae58b4edad6..63f4ff57c9ce 100644
> --- a/lib/siphash.c
> +++ b/lib/siphash.c
> @@ -116,6 +116,19 @@ EXPORT_SYMBOL(__siphash_unaligned);
>  #endif
> 
>  /**
> + * siphash_1ulong - computes siphash PRF value
> + * @first: value to hash
> + * @key: the siphash key
> + */
> +unsigned long siphash_1ulong(const unsigned long first, const
> +siphash_key_t *key) { #ifdef CONFIG_64BIT
> + return (unsigned long)siphash_1u64((u64)first, key); #endif
> + return (unsigned long)siphash_1u32((u32)first, key); }
> +
> +/**
>   * siphash_1u64 - compute 64-bit siphash PRF value of a u64
>   * @first: first u64
>   * @key: the siphash key
> diff --git a/lib/vsprintf.c b/lib/vsprintf.c index 86c3385b9eb3..afd1c835b0f6 
> 100644
> --- a/lib/vsprintf.c
> +++ b/lib/vsprintf.c
> @@ -33,6 +33,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>  #ifdef CONFIG_BLOCK
>  #include 
>  #endif
> @@ -503,6 +504,7 @@ char *number(char *buf, char *end, unsigned long long
> num,
>   *buf = '0';
>   ++buf;
>   }
> +

Unneeded whitespace change?

>   /* actual digits of result */
>   while (--i >= 0) {
>   if (buf < end)
> @@ -1591,6 +1593,28 @@ char *device_node_string(char *buf, char *end, struct
> device_node *dn,
>   return widen_string(buf, buf - buf_start, end, spec);  }
> 
> +/* 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 siphash_key_t ptr_secret __read_mostly;
> + static bool have_key = false;
> + unsigned long hashval;
> +
> + /* Kernel doesn't boot if we use get_random_once() */
> + if (!have_key) {
> + get_random_bytes(_secret, sizeof(ptr_secret));
> + have_key = true;

Wouldn't one want to use an atomic test and swap for this
block?

> + }
> +
> + hashval = siphash_1ulong((unsigned long)ptr, _secret);
> +
> + spec.field_width = 2 + 2 * sizeof(unsigned int); /* 0x + hex */
> + spec.flags = SPECIAL | SMALL | ZEROPAD;
> + spec.base = 16;
> +
> + return number(buf, end, (u32)hashval, spec); }
> +
>  int kptr_restrict 

Re: [PATCH v2] printk: hash addresses printed with %p

[Steven Rostedt]

On Tue, 17 Oct 2017 15:52:51 +1100
"Tobin C. Harding"  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.
> 
> 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
> 
> git grep '%p[^KFfSsBRrbMmIiEUVKNhdDgCGO]' | wc -l

Does %p[FfSs] leak addresses? Well, I guess it does if they are not
found in kallsyms, but otherwise you have:

  function+0x

-- Steve


> 
> arch: 2512
> block: 20
> crypto: 12
> fs: 1221
> include: 147
> kernel: 109
> lib: 77
> mm: 120
> net: 1516
> security: 11
> sound: 168
> virt: 2
> drivers: 8420
> 
> Add helper function siphash_1ulong(). Add function ptr_to_id() to map an
> address to a 32 bit unique identifier.
> 
> Signed-off-by: Tobin C. Harding 
> ---
>

Re: [PATCH v2] printk: hash addresses printed with %p

[Steven Rostedt]

On Tue, 17 Oct 2017 15:52:51 +1100
"Tobin C. Harding"  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.
> 
> 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
> 
> git grep '%p[^KFfSsBRrbMmIiEUVKNhdDgCGO]' | wc -l

Does %p[FfSs] leak addresses? Well, I guess it does if they are not
found in kallsyms, but otherwise you have:

  function+0x

-- Steve


> 
> arch: 2512
> block: 20
> crypto: 12
> fs: 1221
> include: 147
> kernel: 109
> lib: 77
> mm: 120
> net: 1516
> security: 11
> sound: 168
> virt: 2
> drivers: 8420
> 
> Add helper function siphash_1ulong(). Add function ptr_to_id() to map an
> address to a 32 bit unique identifier.
> 
> Signed-off-by: Tobin C. Harding 
> ---
>

Re: [PATCH v2] printk: hash addresses printed with %p

[Joe Perches]

On Tue, 2017-10-17 at 15:52 +1100, Tobin C. Harding 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.
> 
> 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
> 
> git grep '%p[^KFfSsBRrbMmIiEUVKNhdDgCGO]' | wc -l

Not really.
There are many asm uses included there

I think a better grep is:

$ 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

> arch: 2512

arch is especially overestimated.

Re: [PATCH v2] printk: hash addresses printed with %p

[Joe Perches]

On Tue, 2017-10-17 at 15:52 +1100, Tobin C. Harding 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.
> 
> 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
> 
> git grep '%p[^KFfSsBRrbMmIiEUVKNhdDgCGO]' | wc -l

Not really.
There are many asm uses included there

I think a better grep is:

$ 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

> arch: 2512

arch is especially overestimated.