[PATCH] printk: hash addresses printed with %p
Tim Gardner
tim.gardner at canonical.com
Mon Mar 1 19:57:35 UTC 2021
On 2/24/21 1:26 PM, Thadeu Lima de Souza Cascardo wrote:
> 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.
>
Indeed, those include files are not required.
> 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?
>
The %pK format (kptr_restrict) is natively supported in Xenial. One has
to assume upstream thought about when kptr_restrict!=0 could be used
with respect to when /dev/random is initialized. The use of an unadorned
%p format early in boot is a little tougher to predict wrt randomness.
However, even if /dev/random isn't initialized, we're no worse off then
we were before. Right ?
rtg
> 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
--
-----------
Tim Gardner
Canonical, Inc
More information about the kernel-team
mailing list