| Message ID | 1508215972-7769-1-git-send-email-me@tobin.cc (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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.
On Tue, 17 Oct 2017 15:52:51 +1100 "Tobin C. Harding" <me@tobin.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. > > 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<offset> -- 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 <me@tobin.cc> > --- >
> -----Original Message----- > From: Tobin C. Harding [mailto:me@tobin.cc] > Sent: Monday, October 16, 2017 9:53 PM > To: kernel-hardening@lists.openwall.com > Cc: Tobin C. Harding <me@tobin.cc>; Linus Torvalds <torvalds@linux- > foundation.org>; Kees Cook <keescook@chromium.org>; Paolo Bonzini > <pbonzini@redhat.com>; Tycho Andersen <tycho@docker.com>; Roberts, > William C <william.c.roberts@intel.com>; Tejun Heo <tj@kernel.org>; Jordan > Glover <Golden_Miller83@protonmail.ch>; Greg KH > <gregkh@linuxfoundation.org>; Petr Mladek <pmladek@suse.com>; Joe > Perches <joe@perches.com>; Ian Campbell <ijc@hellion.org.uk>; Sergey > Senozhatsky <sergey.senozhatsky@gmail.com>; Catalin Marinas > <catalin.marinas@arm.com>; Will Deacon <will.deacon@arm.com>; Steven > Rostedt <rostedt@goodmis.org>; Chris Fries <cfries@google.com>; Dave > Weinstein <olorin@google.com>; Daniel Micay <danielmicay@gmail.com>; Djalal > Harouni <tixxdz@gmail.com>; 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 <me@tobin.cc> > --- > > 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 <linux/uuid.h> > #include <linux/of.h> > #include <net/addrconf.h> > +#include <linux/siphash.h> > #ifdef CONFIG_BLOCK > #include <linux/blkdev.h> > #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(&ptr_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, &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 __read_mostly; > > /* > @@ -1703,6 +1727,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 > +1885,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
On Tue, Oct 17, 2017 at 05:27:15PM +0000, Roberts, William C wrote: > > > > -----Original Message----- > > From: Tobin C. Harding [mailto:me@tobin.cc] > > Sent: Monday, October 16, 2017 9:53 PM > > To: kernel-hardening@lists.openwall.com > > Cc: Tobin C. Harding <me@tobin.cc>; Linus Torvalds <torvalds@linux- > > foundation.org>; Kees Cook <keescook@chromium.org>; Paolo Bonzini > > <pbonzini@redhat.com>; Tycho Andersen <tycho@docker.com>; Roberts, > > William C <william.c.roberts@intel.com>; Tejun Heo <tj@kernel.org>; Jordan > > Glover <Golden_Miller83@protonmail.ch>; Greg KH > > <gregkh@linuxfoundation.org>; Petr Mladek <pmladek@suse.com>; Joe > > Perches <joe@perches.com>; Ian Campbell <ijc@hellion.org.uk>; Sergey > > Senozhatsky <sergey.senozhatsky@gmail.com>; Catalin Marinas > > <catalin.marinas@arm.com>; Will Deacon <will.deacon@arm.com>; Steven > > Rostedt <rostedt@goodmis.org>; Chris Fries <cfries@google.com>; Dave > > Weinstein <olorin@google.com>; Daniel Micay <danielmicay@gmail.com>; Djalal > > Harouni <tixxdz@gmail.com>; 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 <me@tobin.cc> > > --- > > > > 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 <linux/uuid.h> > > #include <linux/of.h> > > #include <net/addrconf.h> > > +#include <linux/siphash.h> > > #ifdef CONFIG_BLOCK > > #include <linux/blkdev.h> > > #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(&ptr_secret, sizeof(ptr_secret)); > > + have_key = true; > > Wouldn't one want to use an atomic test and swap for this > block? Great, thanks for the pointer. Thanks for the review William. Tobin.
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" <me@tobin.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. > > > > 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<offset> 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.
On Tue, Oct 17, 2017 at 4:15 PM, Tobin C. Harding <me@tobin.cc> 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" <me@tobin.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. >> > >> > 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<offset> > > 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
Hi Tobin, Many comments in line below. On Tue, Oct 17, 2017 at 6:52 AM, Tobin C. Harding <me@tobin.cc> 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(&ptr_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, &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
On Wed, 18 Oct 2017 10:15:59 +1100 "Tobin C. Harding" <me@tobin.cc> wrote: > > Does %p[FfSs] leak addresses? Well, I guess it does if they are not > > found in kallsyms, but otherwise you have: > > > > function+0x<offset> > > 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
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.
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 <me@tobin.cc> 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" <me@tobin.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. > >> > > >> > 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<offset> > > > > 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.
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<offset> > > > > > > > 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<offset>"? If nothing else, it will help correlate the random address with other places where it was printed via %p. - Ted
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 <linux/uuid.h> #include <linux/of.h> #include <net/addrconf.h> +#include <linux/siphash.h> #ifdef CONFIG_BLOCK #include <linux/blkdev.h> #endif @@ -503,6 +504,7 @@ char *number(char *buf, char *end, unsigned long long num, *buf = '0'; ++buf; } + /* 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(&ptr_secret, sizeof(ptr_secret)); + have_key = true; + } + + hashval = siphash_1ulong((unsigned long)ptr, &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 __read_mostly; /* @@ -1703,6 +1727,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 +1885,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); } /*
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 <me@tobin.cc> --- 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(-)