| Message ID | 20210401091351.1690972-2-kleber.souza@canonical.com |
|---|---|
| State | New |
| Headers | show |
| Series | Fix regression on symbols addresses (LP: #1922200) | expand |
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@canonical.com> Acked-by: Stefan Bader <stefan.bader@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); > } > > /* >
On 01/04/2021 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@canonical.com> > --- > Documentation/printk-formats.txt | 11 ---- > lib/test_printf.c | 108 +++++++++++-------------------- > lib/vsprintf.c | 81 ++--------------------- > 3 files changed, 45 insertions(+), 155 deletions(-) Acked-by: Krzysztof Kozlowski <krzysztof.kozlowski@canonical.com> The backport changes user-visible behavior (e.g. logs) so the interface used by some apps might be affected. The impact of CVE is low, so I agree with skipping the backport. Best regards, Krzysztof
On 01/04/2021 10: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@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); > } > > /* > Looks OK to me. Acked-by: Colin Ian King <colin.king@canonical.com>
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); } /*
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@canonical.com> --- Documentation/printk-formats.txt | 11 ---- lib/test_printf.c | 108 +++++++++++-------------------- lib/vsprintf.c | 81 ++--------------------- 3 files changed, 45 insertions(+), 155 deletions(-)