[PATCH] printk: hash addresses printed with %p
Thadeu Lima de Souza Cascardo
cascardo at canonical.com
Wed Feb 24 20:26:41 UTC 2021
On Wed, Feb 24, 2021 at 01:01:01PM -0700, Tim Gardner wrote:
> From: "Tobin C. Harding" <me at tobin.cc>
>
> Currently there exist approximately 14 000 places in the kernel where
> addresses are being printed using an unadorned %p. This potentially
> leaks sensitive information regarding the Kernel layout in memory. Many
> of these calls are stale, instead of fixing every call lets hash the
> address by default before printing. This will of course break some
> users, forcing code printing needed addresses to be updated.
>
> Code that _really_ needs the address will soon be able to use the new
> printk specifier %px to print the address.
>
> 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. Hash any unadorned usage of specifier %p and any malformed
> specifiers.
>
> Signed-off-by: Tobin C. Harding <me at tobin.cc>
> (backported from commit ad67b74d2469d9b82aaa572d76474c95bc484d57)
> CVE-2018-7273
> Signed-off-by: Tim Gardner <tim.gardner at canonical.com>
>
> Conflicts:
> Documentation/printk-formats.txt (Added a paragraph under Kernel Pointers)
> lib/vsprintf.c (required additional include files, no code changes)
> ---
> Documentation/printk-formats.txt | 6 ++
> lib/test_printf.c | 108 ++++++++++++++++++++-----------
> lib/vsprintf.c | 86 ++++++++++++++++++++++--
> 3 files changed, 155 insertions(+), 45 deletions(-)
>
> diff --git a/Documentation/printk-formats.txt b/Documentation/printk-formats.txt
> index ed6f6abaad57..2329b1eac0fa 100644
> --- a/Documentation/printk-formats.txt
> +++ b/Documentation/printk-formats.txt
> @@ -64,6 +64,12 @@ Kernel Pointers:
> users. The behaviour of %pK depends on the kptr_restrict sysctl - see
> Documentation/sysctl/kernel.txt for more details.
>
> + Pointers printed without a specifier extension (i.e unadorned %p) are
> + hashed to give a unique identifier without leaking kernel addresses to user
> + space. On 64 bit machines the first 32 bits are zeroed.
> +
> + %p abcdef12 or 00000000abcdef12
> +
> Struct Resources:
>
> %pr [mem 0x60000000-0x6fffffff flags 0x2200] or
> diff --git a/lib/test_printf.c b/lib/test_printf.c
> index c5a666af9ba5..e2200f06f168 100644
> --- a/lib/test_printf.c
> +++ b/lib/test_printf.c
> @@ -18,24 +18,6 @@
> #define BUF_SIZE 256
> #define FILL_CHAR '$'
>
> -#define PTR1 ((void*)0x01234567)
> -#define PTR2 ((void*)(long)(int)0xfedcba98)
> -
> -#if BITS_PER_LONG == 64
> -#define PTR1_ZEROES "000000000"
> -#define PTR1_SPACES " "
> -#define PTR1_STR "1234567"
> -#define PTR2_STR "fffffffffedcba98"
> -#define PTR_WIDTH 16
> -#else
> -#define PTR1_ZEROES "0"
> -#define PTR1_SPACES " "
> -#define PTR1_STR "1234567"
> -#define PTR2_STR "fedcba98"
> -#define PTR_WIDTH 8
> -#endif
> -#define PTR_WIDTH_STR stringify(PTR_WIDTH)
> -
> static unsigned total_tests __initdata;
> static unsigned failed_tests __initdata;
> static char *test_buffer __initdata;
> @@ -160,30 +142,79 @@ test_string(void)
> test("a | | ", "%-3.s|%-3.0s|%-3.*s", "a", "b", 0, "c");
> }
>
> +#define PLAIN_BUF_SIZE 64 /* leave some space so we don't oops */
> +
> +#if BITS_PER_LONG == 64
> +
> +#define PTR_WIDTH 16
> +#define PTR ((void *)0xffff0123456789ab)
> +#define PTR_STR "ffff0123456789ab"
> +#define ZEROS "00000000" /* hex 32 zero bits */
> +
> +static int __init
> +plain_format(void)
> +{
> + char buf[PLAIN_BUF_SIZE];
> + int nchars;
> +
> + nchars = snprintf(buf, PLAIN_BUF_SIZE, "%p", PTR);
> +
> + if (nchars != PTR_WIDTH || strncmp(buf, ZEROS, strlen(ZEROS)) != 0)
> + return -1;
> +
> + return 0;
> +}
> +
> +#else
> +
> +#define PTR_WIDTH 8
> +#define PTR ((void *)0x456789ab)
> +#define PTR_STR "456789ab"
> +
> +static int __init
> +plain_format(void)
> +{
> + /* Format is implicitly tested for 32 bit machines by plain_hash() */
> + return 0;
> +}
> +
> +#endif /* BITS_PER_LONG == 64 */
> +
> +static int __init
> +plain_hash(void)
> +{
> + char buf[PLAIN_BUF_SIZE];
> + int nchars;
> +
> + nchars = snprintf(buf, PLAIN_BUF_SIZE, "%p", PTR);
> +
> + if (nchars != PTR_WIDTH || strncmp(buf, PTR_STR, PTR_WIDTH) == 0)
> + return -1;
> +
> + return 0;
> +}
> +
> +/*
> + * We can't use test() to test %p because we don't know what output to expect
> + * after an address is hashed.
> + */
> static void __init
> plain(void)
> {
> - test(PTR1_ZEROES PTR1_STR " " PTR2_STR, "%p %p", PTR1, PTR2);
> - /*
> - * The field width is overloaded for some %p extensions to
> - * pass another piece of information. For plain pointers, the
> - * behaviour is slightly odd: One cannot pass either the 0
> - * flag nor a precision to %p without gcc complaining, and if
> - * one explicitly gives a field width, the number is no longer
> - * zero-padded.
> - */
> - test("|" PTR1_STR PTR1_SPACES " | " PTR1_SPACES PTR1_STR "|",
> - "|%-*p|%*p|", PTR_WIDTH+2, PTR1, PTR_WIDTH+2, PTR1);
> - test("|" PTR2_STR " | " PTR2_STR "|",
> - "|%-*p|%*p|", PTR_WIDTH+2, PTR2, PTR_WIDTH+2, PTR2);
> + int err;
>
> - /*
> - * Unrecognized %p extensions are treated as plain %p, but the
> - * alphanumeric suffix is ignored (that is, does not occur in
> - * the output.)
> - */
> - test("|"PTR1_ZEROES PTR1_STR"|", "|%p0y|", PTR1);
> - test("|"PTR2_STR"|", "|%p0y|", PTR2);
> + err = plain_hash();
> + if (err) {
> + pr_warn("plain 'p' does not appear to be hashed\n");
> + failed_tests++;
> + return;
> + }
> +
> + err = plain_format();
> + if (err) {
> + pr_warn("hashing plain 'p' has unexpected format\n");
> + failed_tests++;
> + }
> }
>
> static void __init
> @@ -194,6 +225,7 @@ symbol_ptr(void)
> static void __init
> kernel_ptr(void)
> {
> + /* We can't test this without access to kptr_restrict. */
> }
>
> static void __init
> diff --git a/lib/vsprintf.c b/lib/vsprintf.c
> index 646009db4198..3cfeeaf0518d 100644
> --- a/lib/vsprintf.c
> +++ b/lib/vsprintf.c
> @@ -31,6 +31,13 @@
> #include <linux/dcache.h>
> #include <linux/cred.h>
> #include <net/addrconf.h>
> +#include <linux/siphash.h>
> +#include <linux/compiler.h>
> +#ifdef CONFIG_BLOCK
> +#include <linux/blkdev.h>
> +#endif
> +
> +#include "../mm/internal.h" /* For the trace_print_flags arrays */
>
This hunk is not needed from what I could see from history. It's worth build
testing anyway, but they should be left out.
Can you also test that it works as expected in respect to dmesg and
kptr_restrict values, specially for the earliest messages, as they depend on
random to be initialized?
Cascardo.
> #include <asm/page.h> /* for PAGE_SIZE */
> #include <asm/sections.h> /* for dereference_function_descriptor() */
> @@ -1360,6 +1367,73 @@ char *clock(char *buf, char *end, struct clk *clk, struct printf_spec spec,
>
> int kptr_restrict __read_mostly;
>
> +static bool have_filled_random_ptr_key __read_mostly;
> +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));
> + /*
> + * have_filled_random_ptr_key==true is dependent on get_random_bytes().
> + * ptr_to_id() needs to see have_filled_random_ptr_key==true
> + * after get_random_bytes() returns.
> + */
> + smp_mb();
> + 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 long hashval;
> + const int default_width = 2 * sizeof(ptr);
> +
> + if (unlikely(!have_filled_random_ptr_key)) {
> + spec.field_width = default_width;
> + /* string length must be less than default_width */
> + return string(buf, end, "(ptrval)", spec);
> + }
> +
> +#ifdef CONFIG_64BIT
> + hashval = (unsigned long)siphash_1u64((u64)ptr, &ptr_key);
> + /*
> + * Mask off the first 32 bits, this makes explicit that we have
> + * modified the address (and 32 bits is plenty for a unique ID).
> + */
> + hashval = hashval & 0xffffffff;
> +#else
> + hashval = (unsigned long)siphash_1u32((u32)ptr, &ptr_key);
> +#endif
> +
> + spec.flags |= SMALL;
> + if (spec.field_width == -1) {
> + spec.field_width = default_width;
> + spec.flags |= ZEROPAD;
> + }
> + spec.base = 16;
> +
> + return number(buf, end, hashval, spec);
> +}
> +
> /*
> * Show a '%p' thing. A kernel extension is that the '%p' is followed
> * by an extra set of alphanumeric characters that are extended format
> @@ -1451,6 +1525,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.
> + *
> + * 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,
> @@ -1598,14 +1675,9 @@ char *pointer(const char *fmt, char *buf, char *end, void *ptr,
> ((const struct file *)ptr)->f_path.dentry,
> spec, fmt);
> }
> - 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);
> }
>
> /*
> --
> 2.17.1
>
>
> --
> kernel-team mailing list
> kernel-team at lists.ubuntu.com
> https://lists.ubuntu.com/mailman/listinfo/kernel-team
More information about the kernel-team
mailing list