| Message ID | 1508362242-12857-1-git-send-email-me@tobin.cc (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, Oct 18, 2017 at 2:30 PM, 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. 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.) > 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@tobin.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: 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. :) -Kees
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@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. > > 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@tobin.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.
On Thu, 2017-10-19 at 10:45 +1100, Tobin C. Harding wrote: > 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@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. > > > > 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. > > > > + 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. Perhaps using start/stop indicators could highlight this hashing. Perhaps output using #hash_ptr# by adding something like #define HASHED 128 /* Output hashed ptr with # prefix and postfix */ after #define SPECIAL but also perhaps adding to the column width would break hex parsers of seq_ output > Any thoughts appreciated. 2ยข
On Wed, Oct 18, 2017 at 11:30 PM, Tobin C. Harding <me@tobin.cc> wrote: > +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); > +} > + case -EALREADY: > + initialize_ptr_secret(); > + break; Unfortunately the above is racy, and the spinlock you had before was actually correct (though using an atomic inside a spinlock wasn't strictly necessary). The race is that two callers might hit initialize_ptr_secret at the same time, and have_key will be zero at the beginning for both. Then they'll both scribble over ptr_secret, and might wind up using a different value after if one finishes before the other. I see two ways of correcting this: 1) Go back to the spinlock yourself. 2) Use get_random_bytes_once(&ptr_secret, sizeof(ptr_secret)). I don't know lib/once.c especially well, but from cursory look, it appears to be taking a spinlock too, which means you're probably good. + if (atomic_read(&have_key) == 0) { + random_ready.owner = NULL; + random_ready.func = schedule_async_key_init; You can probably take care of this part in the initialization: static struct random_ready_callback random_ready = { .func = schedule_async_key_init }; Alternatively, you could put the actual call to add_random_ready_callback in an init function, but maybe how you have it is easier. Jason
On (10/19/17 03:03), Jason A. Donenfeld wrote:
[..]
> 1) Go back to the spinlock yourself.
so we ruled out NMI deadlocks?
-ss
On Thu, Oct 19, 2017 at 3:31 AM, Sergey Senozhatsky <sergey.senozhatsky.work@gmail.com> wrote: > On (10/19/17 03:03), Jason A. Donenfeld wrote: > [..] >> 1) Go back to the spinlock yourself. > > so we ruled out NMI deadlocks? Oh, right. No, I haven't thought through this enough to rule it out. Indeed if that's an issue, the locks in the _once code will also be an issue. So if locking is totally impossible, then a race-free way of doing this is with a tri-state compare and exchange. Things are either: in state 1: no key, state 2: getting key, state 3: have key. If state 1 or 2, print the placeholder token. If state 3, do the hashing.
On Thu, Oct 19, 2017 at 03:36:20AM +0200, Jason A. Donenfeld wrote: > On Thu, Oct 19, 2017 at 3:31 AM, Sergey Senozhatsky > <sergey.senozhatsky.work@gmail.com> wrote: > > On (10/19/17 03:03), Jason A. Donenfeld wrote: > > [..] > >> 1) Go back to the spinlock yourself. > > > > so we ruled out NMI deadlocks? > > Oh, right. No, I haven't thought through this enough to rule it out. > Indeed if that's an issue, the locks in the _once code will also be an > issue. > > So if locking is totally impossible, then a race-free way of doing > this is with a tri-state compare and exchange. Things are either: in > state 1: no key, state 2: getting key, state 3: have key. If state 1 > or 2, print the placeholder token. If state 3, do the hashing. Cool! That's the solution I've been looking for since day 1. You the man. thanks, Tobin.
A small detail carried over from the other thread: > > but a bigger problem might the following thing: > > vscnprintf() > pointer() > ptr_to_id() > initialize_ptr_secret() > get_random_bytes() > _get_random_bytes() > extract_crng() > _extract_crng() > spin_lock_irqsave(&crng->lock, flags); <<<<< > > > this, once again, can deadlock. can it? just like before: So, actually, then, we need to do this as an initcall. Fortunately, that simplifies things greatly. Here's a rough sketch of what that looks like, which you'll probably need to debug and refine: static siphash_key_t ptr_secret __ro_after_init; static DEFINE_STATIC_KEY_TRUE(no_ptr_secret); static char *ptr_to_id(char *buf, char *end, void *ptr, struct printf_spec spec) { if (static_branch_unlikely(&no_ptr_secret)) return "(pointer value)"; hashval = .... } static void fill_random_ptr_key(struct random_ready_callback *rdy) { get_random_bytes(&ptr_secret, sizeof(ptr_secret)); static_branch_disable(&no_ptr_secret); } static struct random_ready_callback random_ready = { .func = fill_random_ptr_key }; static int __init initialize_ptr_random(void) { int ret = add_random_ready_callback(&random_ready); if (!ret) return 0; else if (ret == -EALREADY) { fill_random_ptr_key(&random_ready); return 0; } return ret; } early_initcall(initialize_ptr_random);
On Wed, Oct 18, 2017 at 10:49 PM, Jason A. Donenfeld <Jason@zx2c4.com> wrote: > static void fill_random_ptr_key(struct random_ready_callback *rdy) > { > get_random_bytes(&ptr_secret, sizeof(ptr_secret)); > static_branch_disable(&no_ptr_secret); > } > > static struct random_ready_callback random_ready = { > .func = fill_random_ptr_key > }; > > static int __init initialize_ptr_random(void) > { > int ret = add_random_ready_callback(&random_ready); > > if (!ret) > return 0; > else if (ret == -EALREADY) { > fill_random_ptr_key(&random_ready); > return 0; > } > > return ret; > } > early_initcall(initialize_ptr_random); Tangent: why is the random_ready API designed with -EALREADY? Couldn't add_random_ready_callback() just perform the call itself and avoid needing all the callers to check for -EALREADY? -Kees
> Tangent: why is the random_ready API designed with -EALREADY? Couldn't > add_random_ready_callback() just perform the call itself and avoid > needing all the callers to check for -EALREADY? Absolutely. I can submit a patch for this soon, though to avoid merge dependencies, and given the usual delays in getting random.c things merged, maybe I'll wait until after this patch hits Linus' tree. This could even be renamed and made into an LD section, as random_initcall, though that needs some more spec'ing out.
On Thu, Oct 19, 2017 at 07:49:06AM +0200, Jason A. Donenfeld wrote: > A small detail carried over from the other thread: > > > > > but a bigger problem might the following thing: > > > > vscnprintf() > > pointer() > > ptr_to_id() > > initialize_ptr_secret() > > get_random_bytes() > > _get_random_bytes() > > extract_crng() > > _extract_crng() > > spin_lock_irqsave(&crng->lock, flags); <<<<< > > > > > > this, once again, can deadlock. can it? just like before: > > So, actually, then, we need to do this as an initcall. Fortunately, > that simplifies things greatly. Here's a rough sketch of what that > looks like, which you'll probably need to debug and refine: > > > > static siphash_key_t ptr_secret __ro_after_init; > static DEFINE_STATIC_KEY_TRUE(no_ptr_secret); > > static char *ptr_to_id(char *buf, char *end, void *ptr, struct printf_spec spec) > { > if (static_branch_unlikely(&no_ptr_secret)) > return "(pointer value)"; > > hashval = .... > > } > > static void fill_random_ptr_key(struct random_ready_callback *rdy) > { > get_random_bytes(&ptr_secret, sizeof(ptr_secret)); > static_branch_disable(&no_ptr_secret); > } > > static struct random_ready_callback random_ready = { > .func = fill_random_ptr_key > }; > > static int __init initialize_ptr_random(void) > { > int ret = add_random_ready_callback(&random_ready); > > if (!ret) > return 0; > else if (ret == -EALREADY) { > fill_random_ptr_key(&random_ready); > return 0; > } > > return ret; > } > early_initcall(initialize_ptr_random); Thanks for this Jason. This is _conceptually_ what I wanted since before v1, I obviously did not ask the right questions. Not to worry, we got there in the end. The process works, thanks to every bodies patience :) Implemented for v6 as suggested (including __read_mostly), you even got it fast for the usual case. Thanks, I learned a whole bunch from this email. Tobin.
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; + 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); } /*
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 (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@tobin.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(-)