Re: [PATCH v3] vsprintf: don't obfuscate NULL and error pointers
On Tue 2020-05-19 11:36:52, Linus Torvalds wrote: > On Tue, May 19, 2020 at 4:27 AM Ilya Dryomov wrote: > > > > This just came up again, please consider sending this to Linus > > for 5.7. > > I just took it directly, since I like it and it looks trivial. Great, I am happy that it has got resolved. I am sorry for the inconvenience. I have started working again last week. But I am still fighting with many pending mails. BTW: printk maintainers are switching to a git repo with shared write access to avoid these delays in the future. Best Regards, Petr
Re: [PATCH v3] vsprintf: don't obfuscate NULL and error pointers
On Tue, May 19, 2020 at 4:27 AM Ilya Dryomov wrote: > > This just came up again, please consider sending this to Linus > for 5.7. I just took it directly, since I like it and it looks trivial. Linus
Re: [PATCH v3] vsprintf: don't obfuscate NULL and error pointers
On Tue, May 19, 2020 at 01:26:57PM +0200, Ilya Dryomov wrote:
> 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 1) 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:01f8cc5b fff2
> %pK, kptr = 0: 01f8cc5b fff2
> %px: 888048c04020 fff2
> %pK, kptr = 1: 888048c04020 fff2
> %pK, kptr = 2:
>
> Fixes: 3e5903eb9cff ("vsprintf: Prevent crash when dereferencing invalid
> pointers")
> Signed-off-by: Ilya Dryomov
> Reviewed-by: Petr Mladek
> Reviewed-by: Sergey Senozhatsky
FWIW,
Reviewed-by: Andy Shevchenko
> Acked-by: Steven Rostedt (VMware)
> Acked-by: Linus Torvalds
> ---
> lib/test_printf.c | 19 ++-
> lib/vsprintf.c| 7 +++
> 2 files changed, 25 insertions(+), 1 deletion(-)
>
> Hi Petr,
>
> This just came up again, please consider sending this to Linus
> for 5.7.
>
> Prior discussion was split in three threads and revolved around the
> vision for how lib/test_printf.c should be structured between Rasmus
> and yourself. The fix itself wasn't disputed and has several acks.
>
> If you want to restructure the test suite before adding any new
> test cases, v1 doesn't have them, but I'm reposting with test cases
> because I think it's best to add them right away to prevent further
> regressions.
>
> v3:
> - don't use EAGAIN macro in error_pointer() test case as the
> actual error code varies between architectures
>
> v2:
> - fix null_pointer() test case (it didn't catch the original
> regression because test_hashed() doesn't really test much)
> and add error_pointer() test case
>
> diff --git a/lib/test_printf.c b/lib/test_printf.c
> index 2d9f520d2f27..6b1622f4d7c2 100644
> --- a/lib/test_printf.c
> +++ b/lib/test_printf.c
> @@ -214,6 +214,7 @@ test_string(void)
> #define PTR_STR "0123456789ab"
> #define PTR_VAL_NO_CRNG "(ptrval)"
> #define ZEROS "" /* hex 32 zero bits */
> +#define ONES "" /* hex 32 one bits */
>
> static int __init
> plain_format(void)
> @@ -245,6 +246,7 @@ plain_format(void)
> #define PTR_STR "456789ab"
> #define PTR_VAL_NO_CRNG "(ptrval)"
> #define ZEROS ""
> +#define ONES ""
>
> static int __init
> plain_format(void)
> @@ -330,14 +332,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 "", "%p", NULL);
> test(ZEROS "", "%px", NULL);
> test("(null)", "%pE", NULL);
> }
>
> +/*
> + * Error pointers aren't hashed.
> + */
> +static void __init
> +error_pointer(void)
> +{
> + test(ONES "fff5", "%p", ERR_PTR(-11));
> + test(ONES "fff5", "%px", ERR_PTR(-11));
> + test("(efault)", "%pE", ERR_PTR(-11));
> +}
> +
> #define PTR_INVALID ((void *)0x00ab)
>
> static void __init
> @@ -649,6 +665,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 7c488a1ce318..f0f0522cd5a7 100644
> --- a/lib/vsprintf.c
> +++ b/lib/vsprintf.c
> @@ -794,6 +794,13 @@ static char *ptr_to_id(char *buf, char *end, const void
> *ptr,
> unsigned long hashval;
> int ret;
>
> + /*
> + * 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, 3
[PATCH v3] vsprintf: don't obfuscate NULL and error pointers
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 1) 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:01f8cc5b fff2
%pK, kptr = 0: 01f8cc5b fff2
%px: 888048c04020 fff2
%pK, kptr = 1: 888048c04020 fff2
%pK, kptr = 2:
Fixes: 3e5903eb9cff ("vsprintf: Prevent crash when dereferencing invalid
pointers")
Signed-off-by: Ilya Dryomov
Reviewed-by: Petr Mladek
Reviewed-by: Sergey Senozhatsky
Acked-by: Steven Rostedt (VMware)
Acked-by: Linus Torvalds
---
lib/test_printf.c | 19 ++-
lib/vsprintf.c| 7 +++
2 files changed, 25 insertions(+), 1 deletion(-)
Hi Petr,
This just came up again, please consider sending this to Linus
for 5.7.
Prior discussion was split in three threads and revolved around the
vision for how lib/test_printf.c should be structured between Rasmus
and yourself. The fix itself wasn't disputed and has several acks.
If you want to restructure the test suite before adding any new
test cases, v1 doesn't have them, but I'm reposting with test cases
because I think it's best to add them right away to prevent further
regressions.
v3:
- don't use EAGAIN macro in error_pointer() test case as the
actual error code varies between architectures
v2:
- fix null_pointer() test case (it didn't catch the original
regression because test_hashed() doesn't really test much)
and add error_pointer() test case
diff --git a/lib/test_printf.c b/lib/test_printf.c
index 2d9f520d2f27..6b1622f4d7c2 100644
--- a/lib/test_printf.c
+++ b/lib/test_printf.c
@@ -214,6 +214,7 @@ test_string(void)
#define PTR_STR "0123456789ab"
#define PTR_VAL_NO_CRNG "(ptrval)"
#define ZEROS "" /* hex 32 zero bits */
+#define ONES ""/* hex 32 one bits */
static int __init
plain_format(void)
@@ -245,6 +246,7 @@ plain_format(void)
#define PTR_STR "456789ab"
#define PTR_VAL_NO_CRNG "(ptrval)"
#define ZEROS ""
+#define ONES ""
static int __init
plain_format(void)
@@ -330,14 +332,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 "", "%p", NULL);
test(ZEROS "", "%px", NULL);
test("(null)", "%pE", NULL);
}
+/*
+ * Error pointers aren't hashed.
+ */
+static void __init
+error_pointer(void)
+{
+ test(ONES "fff5", "%p", ERR_PTR(-11));
+ test(ONES "fff5", "%px", ERR_PTR(-11));
+ test("(efault)", "%pE", ERR_PTR(-11));
+}
+
#define PTR_INVALID ((void *)0x00ab)
static void __init
@@ -649,6 +665,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 7c488a1ce318..f0f0522cd5a7 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -794,6 +794,13 @@ static char *ptr_to_id(char *buf, char *end, const void
*ptr,
unsigned long hashval;
int ret;
+ /*
+* 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);
--
2.19.2
