ACK: [SRU][X][PATCH 1/1] UBUNTU: SAUCE: Revert "printk: hash addresses printed with %p"
Stefan Bader
stefan.bader at canonical.com
Thu Apr 1 09:20:50 UTC 2021
On 01.04.21 11:13, Kleber Sacilotto de Souza wrote:
> BugLink: https://bugs.launchpad.net/bugs/1922200
>
> This reverts commit 5d742149ceb112c61ee576f371b574da32532c43 (commit
> ad67b74d2469d9b82aaa572d76474c95bc484d57 upstream).
>
> The backport of this upstream commit, applied to fix CVEs
> CVE-2018-5953/CVE-2018-5995/CVE-2018-7754 on xenial/linux, introduced a
> regression on the addresses exported via /proc interfaces (mainly
> /proc/kallsyms). The patch leaks what the address 0x0 hashes to for
> regular users instead of the expected zeroed out values. It also mangles
> the default address for 'startup_64' expected to be 'ffffffff81000000'
> for non-kaslr kernels (<4.15).
>
> Signed-off-by: Kleber Sacilotto de Souza <kleber.souza at canonical.com>
Acked-by: Stefan Bader <stefan.bader at canonical.com>
> ---
> Documentation/printk-formats.txt | 11 ----
> lib/test_printf.c | 108 +++++++++++--------------------
> lib/vsprintf.c | 81 ++---------------------
> 3 files changed, 45 insertions(+), 155 deletions(-)
>
> diff --git a/Documentation/printk-formats.txt b/Documentation/printk-formats.txt
> index fedb13fdb050..ed6f6abaad57 100644
> --- a/Documentation/printk-formats.txt
> +++ b/Documentation/printk-formats.txt
> @@ -31,17 +31,6 @@ return from vsnprintf.
> Raw pointer value SHOULD be printed with %p. The kernel supports
> the following extended format specifiers for pointer types:
>
> -Pointer Types
> -=============
> -
> -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
> -
> Symbols/Function Pointers:
>
> %pF versatile_init+0x0/0x110
> diff --git a/lib/test_printf.c b/lib/test_printf.c
> index e2200f06f168..c5a666af9ba5 100644
> --- a/lib/test_printf.c
> +++ b/lib/test_printf.c
> @@ -18,6 +18,24 @@
> #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;
> @@ -142,79 +160,30 @@ 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)
> {
> - int err;
> -
> - err = plain_hash();
> - if (err) {
> - pr_warn("plain 'p' does not appear to be hashed\n");
> - failed_tests++;
> - return;
> - }
> + 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);
>
> - err = plain_format();
> - if (err) {
> - pr_warn("hashing plain 'p' has unexpected format\n");
> - failed_tests++;
> - }
> + /*
> + * 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);
> }
>
> static void __init
> @@ -225,7 +194,6 @@ 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 698beaccbc37..646009db4198 100644
> --- a/lib/vsprintf.c
> +++ b/lib/vsprintf.c
> @@ -31,8 +31,6 @@
> #include <linux/dcache.h>
> #include <linux/cred.h>
> #include <net/addrconf.h>
> -#include <linux/siphash.h>
> -#include <linux/compiler.h>
>
> #include <asm/page.h> /* for PAGE_SIZE */
> #include <asm/sections.h> /* for dereference_function_descriptor() */
> @@ -1362,73 +1360,6 @@ 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
> @@ -1520,9 +1451,6 @@ static char *ptr_to_id(char *buf, char *end, void *ptr, struct printf_spec spec)
> * 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,
> @@ -1670,9 +1598,14 @@ 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;
>
> - /* default is to _not_ leak addresses, hash before printing */
> - return ptr_to_id(buf, end, ptr, spec);
> + return number(buf, end, (unsigned long) ptr, spec);
> }
>
> /*
>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: OpenPGP_signature
Type: application/pgp-signature
Size: 833 bytes
Desc: OpenPGP digital signature
URL: <https://lists.ubuntu.com/archives/kernel-team/attachments/20210401/a9bc3f25/attachment.sig>
More information about the kernel-team
mailing list