From 0e1d5f67253e63a71da7a6a3efdc9536ed2d2539 Mon Sep 17 00:00:00 2001 From: Ilya Dryomov Date: Tue, 19 May 2020 13:26:57 +0200 Subject: [PATCH] vsprintf: don't obfuscate NULL and error pointers commit 7bd57fbc4a4ddedc664cad0bbced1b469e24e921 upstream. I don't see what security concern is addressed by obfuscating NULL and IS_ERR() error pointers, printed with %p/%pK. Given the number of sites where %p is used (over 10000) and the fact that NULL pointers aren't uncommon, it probably wouldn't take long for an attacker to find the hash that corresponds to 0. Although harder, the same goes for most common error values, such as -1, -2, -11, -14, etc. The NULL part actually fixes a regression: NULL pointers weren't obfuscated until commit 3e5903eb9cff ("vsprintf: Prevent crash when dereferencing invalid pointers") which went into 5.2. I'm tacking the IS_ERR() part on here because error pointers won't leak kernel addresses and printing them as pointers shouldn't be any different from e.g. %d with PTR_ERR_OR_ZERO(). Obfuscating them just makes debugging based on existing pr_debug and friends excruciating. Note that the "always print 0's for %pK when kptr_restrict == 2" behaviour which goes way back is left as is. Example output with the patch applied: ptr error-ptr NULL %p: 0000000001f8cc5b fffffffffffffff2 0000000000000000 %pK, kptr = 0: 0000000001f8cc5b fffffffffffffff2 0000000000000000 %px: ffff888048c04020 fffffffffffffff2 0000000000000000 %pK, kptr = 1: ffff888048c04020 fffffffffffffff2 0000000000000000 %pK, kptr = 2: 0000000000000000 0000000000000000 0000000000000000 Fixes: 3e5903eb9cff ("vsprintf: Prevent crash when dereferencing invalid pointers") Signed-off-by: Ilya Dryomov Reviewed-by: Petr Mladek Reviewed-by: Sergey Senozhatsky Reviewed-by: Andy Shevchenko Acked-by: Steven Rostedt (VMware) Signed-off-by: Linus Torvalds Signed-off-by: Greg Kroah-Hartman --- lib/test_printf.c | 19 ++++++++++++++++++- lib/vsprintf.c | 7 +++++++ 2 files changed, 25 insertions(+), 1 deletion(-) diff --git a/lib/test_printf.c b/lib/test_printf.c index 5d94cbff2120..d4b711b53942 100644 --- a/lib/test_printf.c +++ b/lib/test_printf.c @@ -212,6 +212,7 @@ test_string(void) #define PTR_STR "ffff0123456789ab" #define PTR_VAL_NO_CRNG "(____ptrval____)" #define ZEROS "00000000" /* hex 32 zero bits */ +#define ONES "ffffffff" /* hex 32 one bits */ static int __init plain_format(void) @@ -243,6 +244,7 @@ plain_format(void) #define PTR_STR "456789ab" #define PTR_VAL_NO_CRNG "(ptrval)" #define ZEROS "" +#define ONES "" static int __init plain_format(void) @@ -328,14 +330,28 @@ test_hashed(const char *fmt, const void *p) test(buf, fmt, p); } +/* + * NULL pointers aren't hashed. + */ static void __init null_pointer(void) { - test_hashed("%p", NULL); + test(ZEROS "00000000", "%p", NULL); test(ZEROS "00000000", "%px", NULL); test("(null)", "%pE", NULL); } +/* + * Error pointers aren't hashed. + */ +static void __init +error_pointer(void) +{ + test(ONES "fffffff5", "%p", ERR_PTR(-11)); + test(ONES "fffffff5", "%px", ERR_PTR(-11)); + test("(efault)", "%pE", ERR_PTR(-11)); +} + #define PTR_INVALID ((void *)0x000000ab) static void __init @@ -598,6 +614,7 @@ test_pointer(void) { plain(); null_pointer(); + error_pointer(); invalid_pointer(); symbol_ptr(); kernel_ptr(); diff --git a/lib/vsprintf.c b/lib/vsprintf.c index e78017a3e1bd..fb4af73142b4 100644 --- a/lib/vsprintf.c +++ b/lib/vsprintf.c @@ -746,6 +746,13 @@ static char *ptr_to_id(char *buf, char *end, const void *ptr, const char *str = sizeof(ptr) == 8 ? "(____ptrval____)" : "(ptrval)"; unsigned long hashval; + /* + * Print the real pointer value for NULL and error pointers, + * as they are not actual addresses. + */ + if (IS_ERR_OR_NULL(ptr)) + return pointer_string(buf, end, ptr, spec); + /* When debugging early boot use non-cryptographically secure hash. */ if (unlikely(debug_boot_weak_hash)) { hashval = hash_long((unsigned long)ptr, 32);