| Message ID | 1508986436-31966-3-git-send-email-me@tobin.cc (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, Oct 26, 2017 at 01:53:56PM +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 (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> > --- > lib/vsprintf.c | 157 +++++++++++++++++++++++++++++++++++++++------------------ > 1 file changed, 107 insertions(+), 50 deletions(-) > > diff --git a/lib/vsprintf.c b/lib/vsprintf.c > index 16a587aed40e..8f4aebd10c7e 100644 > --- a/lib/vsprintf.c > +++ b/lib/vsprintf.c > @@ -33,6 +33,8 @@ > #include <linux/uuid.h> > #include <linux/of.h> > #include <net/addrconf.h> > +#include <linux/siphash.h> > +#include <linux/compiler.h> > #ifdef CONFIG_BLOCK > #include <linux/blkdev.h> > #endif > @@ -1344,6 +1346,57 @@ char *uuid_string(char *buf, char *end, const u8 *addr, > } > > static noinline_for_stack > +char *kernel_pointer(char *buf, char *end, const void *ptr, > + struct printf_spec spec) > +{ > + spec.base = 16; > + spec.flags |= SMALL; > + if (spec.field_width == -1) { > + spec.field_width = 2 * sizeof(void *); > + spec.flags |= ZEROPAD; > + } > + > + switch (kptr_restrict) { > + case 0: > + /* Always print %pK values */ > + break; > + case 1: { > + const struct cred *cred; > + > + /* > + * kptr_restrict==1 cannot be used in IRQ context > + * because its test for CAP_SYSLOG would be meaningless. > + */ > + if (in_irq() || in_serving_softirq() || in_nmi()) > + return string(buf, end, "pK-error", spec); > + > + /* > + * Only print the real pointer value if the current > + * process has CAP_SYSLOG and is running with the > + * same credentials it started with. This is because > + * access to files is checked at open() time, but %pK > + * checks permission at read() time. We don't want to > + * leak pointer values if a binary opens a file using > + * %pK and then elevates privileges before reading it. > + */ > + cred = current_cred(); > + if (!has_capability_noaudit(current, CAP_SYSLOG) || > + !uid_eq(cred->euid, cred->uid) || > + !gid_eq(cred->egid, cred->gid)) > + ptr = NULL; > + break; > + } > + case 2: > + default: > + /* Always print 0's for %pK */ > + ptr = NULL; > + break; > + } > + > + return number(buf, end, (unsigned long)ptr, spec); > +} > + > +static noinline_for_stack > char *netdev_bits(char *buf, char *end, const void *addr, const char *fmt) > { > unsigned long long num; > @@ -1591,6 +1644,54 @@ char *device_node_string(char *buf, char *end, struct device_node *dn, > return widen_string(buf, buf - buf_start, end, spec); > } > > +static bool have_filled_random_ptr_key; > +static siphash_key_t ptr_key __read_mostly; > + > +static void fill_random_ptr_key(struct random_ready_callback *unused) > +{ > + get_random_bytes(&ptr_key, sizeof(ptr_key)); > + WRITE_ONCE(have_filled_random_ptr_key, true); This usage of WRITE_ONCE was suggested by Jason A. Donenfeld. I read include/linux/compiler.h but was not able to grok it. Is this enough to stop the compiler re-ordering these two statements? Or do I need to read Documentation/memory-barriers.txt [again]? thanks, Tobin.
On Thu, Oct 26, 2017 at 4:53 AM, Tobin C. Harding <me@tobin.cc> wrote:
> +static bool have_filled_random_ptr_key;
__read_mostly
On Thu, 26 Oct 2017 13:58:38 +1100 "Tobin C. Harding" <me@tobin.cc> wrote: > > +static bool have_filled_random_ptr_key; > > +static siphash_key_t ptr_key __read_mostly; > > + > > +static void fill_random_ptr_key(struct random_ready_callback *unused) > > +{ > > + get_random_bytes(&ptr_key, sizeof(ptr_key)); > > + WRITE_ONCE(have_filled_random_ptr_key, true); > > This usage of WRITE_ONCE was suggested by Jason A. Donenfeld. I read > include/linux/compiler.h but was not able to grok it. Is this enough to > stop the compiler re-ordering these two statements? > > Or do I need to read Documentation/memory-barriers.txt [again]? No, the WRITE_ONCE does not stop the compiler from reordering those statements. If you need that, then you need to do: get_random_bytes(&ptr_key, sizeof(ptr_key)); barrier(); WRITE_ONCE(have_filled_random_ptr_key, true); and that only works against interrupts. If you need synchronization across CPUs, then you need smp_mb(). -- Steve
On Mon, Oct 30, 2017 at 05:33:22PM -0400, Steven Rostedt wrote: > On Thu, 26 Oct 2017 13:58:38 +1100 > "Tobin C. Harding" <me@tobin.cc> wrote: > > > > +static bool have_filled_random_ptr_key; > > > +static siphash_key_t ptr_key __read_mostly; > > > + > > > +static void fill_random_ptr_key(struct random_ready_callback *unused) > > > +{ > > > + get_random_bytes(&ptr_key, sizeof(ptr_key)); > > > + WRITE_ONCE(have_filled_random_ptr_key, true); > > > > This usage of WRITE_ONCE was suggested by Jason A. Donenfeld. I read > > include/linux/compiler.h but was not able to grok it. Is this enough to > > stop the compiler re-ordering these two statements? > > > > Or do I need to read Documentation/memory-barriers.txt [again]? > > No, the WRITE_ONCE does not stop the compiler from reordering those > statements. If you need that, then you need to do: > > get_random_bytes(&ptr_key, sizeof(ptr_key)); > barrier(); > WRITE_ONCE(have_filled_random_ptr_key, true); > > and that only works against interrupts. If you need synchronization > across CPUs, then you need smp_mb(). Cool. So I think we need get_random_bytes(&ptr_key, sizeof(ptr_key)); smp_mb(); WRITE_ONCE(have_filled_random_ptr_key, true); V10 to include this unless I have it wrong. thanks, Tobin.
On Tue, 31 Oct 2017 09:41:02 +1100 "Tobin C. Harding" <me@tobin.cc> wrote: > Cool. So I think we need > > get_random_bytes(&ptr_key, sizeof(ptr_key)); You'll need to add a comment here to describe what ordering the memory barrier is used against. That is, somewhere else there's something that needs to see the "true" after the "get_random_bytes" update. Describe it here. -- Steve > smp_mb(); > WRITE_ONCE(have_filled_random_ptr_key, true); > > V10 to include this unless I have it wrong. > > thanks, > Tobin.
On Mon, Oct 30, 2017 at 08:00:46PM -0400, Steven Rostedt wrote: > On Tue, 31 Oct 2017 09:41:02 +1100 > "Tobin C. Harding" <me@tobin.cc> wrote: > > > > Cool. So I think we need > > > > get_random_bytes(&ptr_key, sizeof(ptr_key)); > > You'll need to add a comment here to describe what ordering the memory > barrier is used against. That is, somewhere else there's something that > needs to see the "true" after the "get_random_bytes" update. Describe > it here. Got it, thanks Tobin.
diff --git a/lib/vsprintf.c b/lib/vsprintf.c index 16a587aed40e..8f4aebd10c7e 100644 --- a/lib/vsprintf.c +++ b/lib/vsprintf.c @@ -33,6 +33,8 @@ #include <linux/uuid.h> #include <linux/of.h> #include <net/addrconf.h> +#include <linux/siphash.h> +#include <linux/compiler.h> #ifdef CONFIG_BLOCK #include <linux/blkdev.h> #endif @@ -1344,6 +1346,57 @@ char *uuid_string(char *buf, char *end, const u8 *addr, } static noinline_for_stack +char *kernel_pointer(char *buf, char *end, const void *ptr, + struct printf_spec spec) +{ + spec.base = 16; + spec.flags |= SMALL; + if (spec.field_width == -1) { + spec.field_width = 2 * sizeof(void *); + spec.flags |= ZEROPAD; + } + + switch (kptr_restrict) { + case 0: + /* Always print %pK values */ + break; + case 1: { + const struct cred *cred; + + /* + * kptr_restrict==1 cannot be used in IRQ context + * because its test for CAP_SYSLOG would be meaningless. + */ + if (in_irq() || in_serving_softirq() || in_nmi()) + return string(buf, end, "pK-error", spec); + + /* + * Only print the real pointer value if the current + * process has CAP_SYSLOG and is running with the + * same credentials it started with. This is because + * access to files is checked at open() time, but %pK + * checks permission at read() time. We don't want to + * leak pointer values if a binary opens a file using + * %pK and then elevates privileges before reading it. + */ + cred = current_cred(); + if (!has_capability_noaudit(current, CAP_SYSLOG) || + !uid_eq(cred->euid, cred->uid) || + !gid_eq(cred->egid, cred->gid)) + ptr = NULL; + break; + } + case 2: + default: + /* Always print 0's for %pK */ + ptr = NULL; + break; + } + + return number(buf, end, (unsigned long)ptr, spec); +} + +static noinline_for_stack char *netdev_bits(char *buf, char *end, const void *addr, const char *fmt) { unsigned long long num; @@ -1591,6 +1644,54 @@ char *device_node_string(char *buf, char *end, struct device_node *dn, return widen_string(buf, buf - buf_start, end, spec); } +static bool have_filled_random_ptr_key; +static siphash_key_t ptr_key __read_mostly; + +static void fill_random_ptr_key(struct random_ready_callback *unused) +{ + get_random_bytes(&ptr_key, sizeof(ptr_key)); + WRITE_ONCE(have_filled_random_ptr_key, true); +} + +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); + +/* Maps a pointer to a 32 bit unique identifier. */ +static char *ptr_to_id(char *buf, char *end, void *ptr, struct printf_spec spec) +{ + unsigned int hashval; + int size = sizeof(hashval); + + if (unlikely(!have_filled_random_ptr_key)) { + spec.field_width = 2 + 2 * size; /* 0x + hex */ + return string(buf, end, "(pointer)", spec); + } + +#ifdef CONFIG_64BIT + hashval = (unsigned int)siphash_1u64((u64)ptr, &ptr_key); +#else + hashval = (unsigned int)siphash_1u32((u32)ptr, &ptr_key); +#endif + + return special_hex_number(buf, end, hashval, size); +} + int kptr_restrict __read_mostly; /* @@ -1703,13 +1804,14 @@ 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. + * + * Note: The 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, struct printf_spec spec) { - const int default_width = 2 * sizeof(void *); - if (!ptr && *fmt != 'K') return string(buf, end, "(null)", spec); @@ -1785,47 +1887,7 @@ char *pointer(const char *fmt, char *buf, char *end, void *ptr, return buf; } case 'K': - switch (kptr_restrict) { - case 0: - /* Always print %pK values */ - break; - case 1: { - const struct cred *cred; - - /* - * kptr_restrict==1 cannot be used in IRQ context - * because its test for CAP_SYSLOG would be meaningless. - */ - if (in_irq() || in_serving_softirq() || in_nmi()) { - if (spec.field_width == -1) - spec.field_width = default_width; - return string(buf, end, "pK-error", spec); - } - - /* - * Only print the real pointer value if the current - * process has CAP_SYSLOG and is running with the - * same credentials it started with. This is because - * access to files is checked at open() time, but %pK - * checks permission at read() time. We don't want to - * leak pointer values if a binary opens a file using - * %pK and then elevates privileges before reading it. - */ - cred = current_cred(); - if (!has_capability_noaudit(current, CAP_SYSLOG) || - !uid_eq(cred->euid, cred->uid) || - !gid_eq(cred->egid, cred->gid)) - ptr = NULL; - break; - } - case 2: - default: - /* Always print 0's for %pK */ - ptr = NULL; - break; - } - break; - + return kernel_pointer(buf, end, ptr, spec); case 'N': return netdev_bits(buf, end, ptr, fmt); case 'a': @@ -1851,14 +1913,9 @@ 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); + /* default is to _not_ leak addresses, hash before printing */ + 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> --- lib/vsprintf.c | 157 +++++++++++++++++++++++++++++++++++++++------------------ 1 file changed, 107 insertions(+), 50 deletions(-)