[The list of email addresses on CC: is getting quite lengthy,
so I hope I've included everyone.]
Although hashing addresses printed via printk does make the
kernel more secure, it interferes with debugging, especially
with some functions like print_hex_dump() which always uses
hashed addresses.
To avoid having to choose between %p and %px, it's easier to
add a kernel command line that treats all %p as %px. This
encourages developers to use %p more without making debugging
more difficult.
Patches #1 and #2 upgrade the kselftest framework so that
it can report on tests that were skipped outright. This
is needed for the test_printf module which will now skip
%p hashing tests if hashing is disabled.
Patch #2 upgrades the printf library to check the command
line. It also updates test_printf().
Full series:
Acked-by: Marco Elver <[email protected]>
Timur Tabi (3):
[v3] lib: use KSTM_MODULE_GLOBALS macro in kselftest drivers
[v3] kselftest: add support for skipped tests
[v3] lib/vsprintf: debug_never_hash_pointers prints all addresses as
unhashed
.../admin-guide/kernel-parameters.txt | 15 ++++++++
lib/test_bitmap.c | 3 +-
lib/test_printf.c | 12 +++++-
lib/vsprintf.c | 38 ++++++++++++++++++-
tools/testing/selftests/kselftest_module.h | 18 ++++++---
5 files changed, 74 insertions(+), 12 deletions(-)
--
2.25.1
Update the kselftest framework to allow client drivers to
specify that some tests were skipped.
Signed-off-by: Timur Tabi <[email protected]>
---
tools/testing/selftests/kselftest_module.h | 18 ++++++++++++------
1 file changed, 12 insertions(+), 6 deletions(-)
diff --git a/tools/testing/selftests/kselftest_module.h b/tools/testing/selftests/kselftest_module.h
index e8eafaf0941a..e2ea41de3f35 100644
--- a/tools/testing/selftests/kselftest_module.h
+++ b/tools/testing/selftests/kselftest_module.h
@@ -11,7 +11,8 @@
#define KSTM_MODULE_GLOBALS() \
static unsigned int total_tests __initdata; \
-static unsigned int failed_tests __initdata
+static unsigned int failed_tests __initdata; \
+static unsigned int skipped_tests __initdata
#define KSTM_CHECK_ZERO(x) do { \
total_tests++; \
@@ -21,11 +22,16 @@ static unsigned int failed_tests __initdata
} \
} while (0)
-static inline int kstm_report(unsigned int total_tests, unsigned int failed_tests)
+static inline int kstm_report(unsigned int total_tests, unsigned int failed_tests,
+ unsigned int skipped_tests)
{
- if (failed_tests == 0)
- pr_info("all %u tests passed\n", total_tests);
- else
+ if (failed_tests == 0) {
+ if (skipped_tests) {
+ pr_info("skipped %u tests\n", skipped_tests);
+ pr_info("remaining %u tests passed\n", total_tests);
+ } else
+ pr_info("all %u tests passed\n", total_tests);
+ } else
pr_warn("failed %u out of %u tests\n", failed_tests, total_tests);
return failed_tests ? -EINVAL : 0;
@@ -36,7 +42,7 @@ static int __init __module##_init(void) \
{ \
pr_info("loaded.\n"); \
selftest(); \
- return kstm_report(total_tests, failed_tests); \
+ return kstm_report(total_tests, failed_tests, skipped_tests); \
} \
static void __exit __module##_exit(void) \
{ \
--
2.25.1
Instead of defining the total/failed test counters manually,
test drivers that are clients of kselftest should use the
macro created for this purpose.
Signed-off-by: Timur Tabi <[email protected]>
Reviewed-by: Petr Mladek <[email protected]>
---
lib/test_bitmap.c | 3 +--
lib/test_printf.c | 4 ++--
2 files changed, 3 insertions(+), 4 deletions(-)
diff --git a/lib/test_bitmap.c b/lib/test_bitmap.c
index 4425a1dd4ef1..0ea0e8258f14 100644
--- a/lib/test_bitmap.c
+++ b/lib/test_bitmap.c
@@ -16,8 +16,7 @@
#include "../tools/testing/selftests/kselftest_module.h"
-static unsigned total_tests __initdata;
-static unsigned failed_tests __initdata;
+KSTM_MODULE_GLOBALS();
static char pbl_buffer[PAGE_SIZE] __initdata;
diff --git a/lib/test_printf.c b/lib/test_printf.c
index 7ac87f18a10f..ad2bcfa8caa1 100644
--- a/lib/test_printf.c
+++ b/lib/test_printf.c
@@ -30,8 +30,8 @@
#define PAD_SIZE 16
#define FILL_CHAR '$'
-static unsigned total_tests __initdata;
-static unsigned failed_tests __initdata;
+KSTM_MODULE_GLOBALS();
+
static char *test_buffer __initdata;
static char *alloced_buffer __initdata;
--
2.25.1
If the debug_never_hash_pointers command line parameter is set, then
printk("%p") will print pointers as unhashed, which is useful for
debugging purposes. This also applies to any function that uses
vsprintf, such as print_hex_dump() and seq_buf_printf().
A large warning message is displayed if this option is enabled.
Unhashed pointers expose kernel addresses, which can be a security
risk.
Also update test_printf to skip the hashed pointer tests if the
command-line option is set.
Signed-off-by: Timur Tabi <[email protected]>
Acked-by: Petr Mladek <[email protected]>
Acked-by: Randy Dunlap <[email protected]>
Acked-by: Sergey Senozhatsky <[email protected]>
Acked-by: Vlastimil Babka <[email protected]>
---
.../admin-guide/kernel-parameters.txt | 15 ++++++++
lib/test_printf.c | 8 ++++
lib/vsprintf.c | 38 ++++++++++++++++++-
3 files changed, 59 insertions(+), 2 deletions(-)
diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
index a10b545c2070..2a97e787f49c 100644
--- a/Documentation/admin-guide/kernel-parameters.txt
+++ b/Documentation/admin-guide/kernel-parameters.txt
@@ -810,6 +810,21 @@
1 will print _a lot_ more information - normally
only useful to kernel developers.
+ debug_never_hash_pointers
+ Force pointers printed to the console or buffers to be
+ unhashed. By default, when a pointer is printed via %p
+ format string, that pointer is "hashed", i.e. obscured
+ by hashing the pointer value. This is a security feature
+ that hides actual kernel addresses from unprivileged
+ users, but it also makes debugging the kernel more
+ difficult since unequal pointers can no longer be
+ compared. However, if this command-line option is
+ specified, then all normal pointers will have their true
+ value printed. Pointers printed via %pK may still be
+ hashed. This option should only be specified when
+ debugging the kernel. Please do not use on production
+ kernels.
+
debug_objects [KNL] Enable object debugging
no_debug_objects
diff --git a/lib/test_printf.c b/lib/test_printf.c
index ad2bcfa8caa1..b0b62d76e598 100644
--- a/lib/test_printf.c
+++ b/lib/test_printf.c
@@ -35,6 +35,8 @@ KSTM_MODULE_GLOBALS();
static char *test_buffer __initdata;
static char *alloced_buffer __initdata;
+extern bool debug_never_hash_pointers;
+
static int __printf(4, 0) __init
do_test(int bufsize, const char *expect, int elen,
const char *fmt, va_list ap)
@@ -301,6 +303,12 @@ plain(void)
{
int err;
+ if (debug_never_hash_pointers) {
+ pr_warn("skipping plain 'p' tests");
+ skipped_tests += 2;
+ return;
+ }
+
err = plain_hash();
if (err) {
pr_warn("plain 'p' does not appear to be hashed\n");
diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index 3b53c73580c5..b4e07ecb1cb2 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -2090,6 +2090,34 @@ char *fwnode_string(char *buf, char *end, struct fwnode_handle *fwnode,
return widen_string(buf, buf - buf_start, end, spec);
}
+/* Disable pointer hashing if requested */
+bool debug_never_hash_pointers __ro_after_init;
+EXPORT_SYMBOL_GPL(debug_never_hash_pointers);
+
+static int __init debug_never_hash_pointers_enable(char *str)
+{
+ debug_never_hash_pointers = true;
+
+ pr_warn("**********************************************************\n");
+ pr_warn("** NOTICE NOTICE NOTICE NOTICE NOTICE NOTICE NOTICE **\n");
+ pr_warn("** **\n");
+ pr_warn("** All pointers that are printed to the console will **\n");
+ pr_warn("** be printed as unhashed. **\n");
+ pr_warn("** **\n");
+ pr_warn("** Kernel memory addresses are exposed, which may **\n");
+ pr_warn("** reduce the security of your system. **\n");
+ pr_warn("** **\n");
+ pr_warn("** If you see this message and you are not debugging **\n");
+ pr_warn("** the kernel, report this immediately to your system **\n");
+ pr_warn("** administrator! **\n");
+ pr_warn("** **\n");
+ pr_warn("** NOTICE NOTICE NOTICE NOTICE NOTICE NOTICE NOTICE **\n");
+ pr_warn("**********************************************************\n");
+
+ return 0;
+}
+early_param("debug_never_hash_pointers", debug_never_hash_pointers_enable);
+
/*
* Show a '%p' thing. A kernel extension is that the '%p' is followed
* by an extra set of alphanumeric characters that are extended format
@@ -2297,8 +2325,14 @@ char *pointer(const char *fmt, char *buf, char *end, void *ptr,
}
}
- /* default is to _not_ leak addresses, hash before printing */
- return ptr_to_id(buf, end, ptr, spec);
+ /*
+ * default is to _not_ leak addresses, so hash before printing,
+ * unless debug_never_hash_pointers is specified on the command line.
+ */
+ if (unlikely(debug_never_hash_pointers))
+ return pointer_string(buf, end, ptr, spec);
+ else
+ return ptr_to_id(buf, end, ptr, spec);
}
/*
--
2.25.1
On Wed, Feb 10, 2021 at 03:34:50PM -0600, Timur Tabi wrote:
> [The list of email addresses on CC: is getting quite lengthy,
> so I hope I've included everyone.]
>
> Although hashing addresses printed via printk does make the
> kernel more secure, it interferes with debugging, especially
> with some functions like print_hex_dump() which always uses
> hashed addresses.
>
> To avoid having to choose between %p and %px, it's easier to
> add a kernel command line that treats all %p as %px. This
> encourages developers to use %p more without making debugging
> more difficult.
>
> Patches #1 and #2 upgrade the kselftest framework so that
> it can report on tests that were skipped outright. This
> is needed for the test_printf module which will now skip
> %p hashing tests if hashing is disabled.
>
> Patch #2 upgrades the printf library to check the command
> line. It also updates test_printf().
Side note to the future contributions.
> Full series:
>
> Acked-by: Marco Elver <[email protected]>
That's what you usually add to each patch in the series individually.
You may use simple oneliner for this, i.e. `git filter-branch --msg-filter ...`
--
With Best Regards,
Andy Shevchenko
Hi!
> If the debug_never_hash_pointers command line parameter is set, then
Can we make this something shorter? Clearly you don't want people
placing this in their grub config, so they'll be most likely typing
this a lot...
debug_pointers or debug_ptrs would be better.
Thanks,
Pavel
--
http://www.livejournal.com/~pavelmachek
On 2/11/21 6:31 AM, Pavel Machek wrote:
> Can we make this something shorter? Clearly you don't want people
> placing this in their grub config, so they'll be most likely typing
> this a lot...
>
> debug_pointers or debug_ptrs would be better.
dbg_unhash_ptrs? "debug_ptrs" is too vague IMHO, and I want to keep the
word "hash" somewhere there to indicate exactly what's happening.
On Thu, Feb 11, 2021 at 11:08:12AM -0600, Timur Tabi wrote:
>
>
> On 2/11/21 6:31 AM, Pavel Machek wrote:
> > Can we make this something shorter? Clearly you don't want people
> > placing this in their grub config, so they'll be most likely typing
> > this a lot...
> >
> > debug_pointers or debug_ptrs would be better.
>
> dbg_unhash_ptrs? "debug_ptrs" is too vague IMHO, and I want to keep the
> word "hash" somewhere there to indicate exactly what's happening.
no_hash_pointers ?
On Thu 2021-02-11 11:08:12, Timur Tabi wrote:
>
>
> On 2/11/21 6:31 AM, Pavel Machek wrote:
> > Can we make this something shorter? Clearly you don't want people
> > placing this in their grub config, so they'll be most likely typing
> > this a lot...
> >
> > debug_pointers or debug_ptrs would be better.
>
> dbg_unhash_ptrs? "debug_ptrs" is too vague IMHO, and I want to keep the
> word "hash" somewhere there to indicate exactly what's happening.
I understand that the long name is painful. But I would prefer to
avoid another bike shedding over it.
There was some pushback against this feature in general.
It should be used deliberately and people must be aware
of the consequences. This is why it is only boot option
and why it prints such a huge warning. The long clear
name helps as well.
I propose to keep the name as is for now. We could always
introduce an alias later when there is a wide preference
and consensus.
Best Regards,
Petr
On Wed 2021-02-10 15:34:53, Timur Tabi wrote:
> If the debug_never_hash_pointers command line parameter is set, then
> printk("%p") will print pointers as unhashed, which is useful for
> debugging purposes. This also applies to any function that uses
> vsprintf, such as print_hex_dump() and seq_buf_printf().
>
> A large warning message is displayed if this option is enabled.
> Unhashed pointers expose kernel addresses, which can be a security
> risk.
>
> Also update test_printf to skip the hashed pointer tests if the
> command-line option is set.
>
> Signed-off-by: Timur Tabi <[email protected]>
> Acked-by: Petr Mladek <[email protected]>
> Acked-by: Randy Dunlap <[email protected]>
> Acked-by: Sergey Senozhatsky <[email protected]>
> Acked-by: Vlastimil Babka <[email protected]>
> ---
> .../admin-guide/kernel-parameters.txt | 15 ++++++++
> lib/test_printf.c | 8 ++++
> lib/vsprintf.c | 38 ++++++++++++++++++-
> 3 files changed, 59 insertions(+), 2 deletions(-)
>
> diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
> index a10b545c2070..2a97e787f49c 100644
> --- a/Documentation/admin-guide/kernel-parameters.txt
> +++ b/Documentation/admin-guide/kernel-parameters.txt
> @@ -810,6 +810,21 @@
> 1 will print _a lot_ more information - normally
> only useful to kernel developers.
>
> + debug_never_hash_pointers
> + Force pointers printed to the console or buffers to be
> + unhashed. By default, when a pointer is printed via %p
> + format string, that pointer is "hashed", i.e. obscured
> + by hashing the pointer value. This is a security feature
> + that hides actual kernel addresses from unprivileged
> + users, but it also makes debugging the kernel more
> + difficult since unequal pointers can no longer be
> + compared. However, if this command-line option is
> + specified, then all normal pointers will have their true
> + value printed. Pointers printed via %pK may still be
> + hashed. This option should only be specified when
> + debugging the kernel. Please do not use on production
> + kernels.
I like this description.
> diff --git a/lib/vsprintf.c b/lib/vsprintf.c
> index 3b53c73580c5..b4e07ecb1cb2 100644
> --- a/lib/vsprintf.c
> +++ b/lib/vsprintf.c
> @@ -2090,6 +2090,34 @@ char *fwnode_string(char *buf, char *end, struct fwnode_handle *fwnode,
> return widen_string(buf, buf - buf_start, end, spec);
> }
>
> +/* Disable pointer hashing if requested */
> +bool debug_never_hash_pointers __ro_after_init;
> +EXPORT_SYMBOL_GPL(debug_never_hash_pointers);
> +
> +static int __init debug_never_hash_pointers_enable(char *str)
> +{
> + debug_never_hash_pointers = true;
> +
> + pr_warn("**********************************************************\n");
> + pr_warn("** NOTICE NOTICE NOTICE NOTICE NOTICE NOTICE NOTICE **\n");
> + pr_warn("** **\n");
> + pr_warn("** All pointers that are printed to the console will **\n");
> + pr_warn("** be printed as unhashed. **\n");
I would really like to make it clear here that it is not only about
consoles. Most people will see only this message. Only few people read
documentation. Many people will learn the parameter name from another
context by googling.
I know that it is not easy to find good words. Especially because
pointers printed by %pK might still be hashed.
> + pr_warn("** **\n");
> + pr_warn("** Kernel memory addresses are exposed, which may **\n");
> + pr_warn("** reduce the security of your system. **\n");
What about replacing the first two paragraphs with something like:
"This system shows unhashed kernel memory addresses via logs and
other interfaces. It might reduce the security of your system."
Best Regards,
Petr
> + pr_warn("** **\n");
> + pr_warn("** If you see this message and you are not debugging **\n");
> + pr_warn("** the kernel, report this immediately to your system **\n");
> + pr_warn("** administrator! **\n");
> + pr_warn("** **\n");
> + pr_warn("** NOTICE NOTICE NOTICE NOTICE NOTICE NOTICE NOTICE **\n");
> + pr_warn("**********************************************************\n");
> +
> + return 0;
> +}
> +early_param("debug_never_hash_pointers", debug_never_hash_pointers_enable);
> +
> /*
> * Show a '%p' thing. A kernel extension is that the '%p' is followed
> * by an extra set of alphanumeric characters that are extended format
> @@ -2297,8 +2325,14 @@ char *pointer(const char *fmt, char *buf, char *end, void *ptr,
> }
> }
>
> - /* default is to _not_ leak addresses, hash before printing */
> - return ptr_to_id(buf, end, ptr, spec);
> + /*
> + * default is to _not_ leak addresses, so hash before printing,
> + * unless debug_never_hash_pointers is specified on the command line.
> + */
> + if (unlikely(debug_never_hash_pointers))
> + return pointer_string(buf, end, ptr, spec);
> + else
> + return ptr_to_id(buf, end, ptr, spec);
> }
>
> /*
> --
> 2.25.1
On 2/11/21 11:23 AM, Petr Mladek wrote:
> There was some pushback against this feature in general.
> It should be used deliberately and people must be aware
> of the consequences. This is why it is only boot option
> and why it prints such a huge warning. The long clear
> name helps as well.
This is my thinking as well. I wanted it to be a bit obnoxious.
On 2/11/21 11:53 AM, Petr Mladek wrote:
> I would really like to make it clear here that it is not only about
> consoles. Most people will see only this message. Only few people read
> documentation. Many people will learn the parameter name from another
> context by googling.
>
> I know that it is not easy to find good words. Especially because
> pointers printed by %pK might still be hashed.
>
>> + pr_warn("** **\n");
>> + pr_warn("** Kernel memory addresses are exposed, which may **\n");
>> + pr_warn("** reduce the security of your system. **\n");
> What about replacing the first two paragraphs with something like:
>
> "This system shows unhashed kernel memory addresses via logs and
> other interfaces. It might reduce the security of your system."
That works, thanks. I'll post a v4 soon.
On Thu 2021-02-11 17:20:26, Matthew Wilcox wrote:
> On Thu, Feb 11, 2021 at 11:08:12AM -0600, Timur Tabi wrote:
> >
> >
> > On 2/11/21 6:31 AM, Pavel Machek wrote:
> > > Can we make this something shorter? Clearly you don't want people
> > > placing this in their grub config, so they'll be most likely typing
> > > this a lot...
> > >
> > > debug_pointers or debug_ptrs would be better.
> >
> > dbg_unhash_ptrs? "debug_ptrs" is too vague IMHO, and I want to keep the
> > word "hash" somewhere there to indicate exactly what's happening.
>
> no_hash_pointers ?
I am fine with this.
I am still a bit scared of a bikeshedng. But AFAIK, Mathew was most
active on proposing clear names. So, when he is fine with this...
Anyway, we should use the same name also for the variable.
Best Regards,
Petr
On Wed 2021-02-10 15:34:52, Timur Tabi wrote:
> Update the kselftest framework to allow client drivers to
> specify that some tests were skipped.
>
> Signed-off-by: Timur Tabi <[email protected]>
Reviewed-by: Petr Mladek <[email protected]>
Tested-by: Petr Mladek <[email protected]>
Best Regards,
Petr
On 2/12/21 4:01 AM, Petr Mladek wrote:
>> no_hash_pointers ?
> I am fine with this.
>
> I am still a bit scared of a bikeshedng. But AFAIK, Mathew was most
> active on proposing clear names. So, when he is fine with this...
>
> Anyway, we should use the same name also for the variable.
Ok, unless there are any objections, I will change the parameter and
variable to "no_hash_pointers" in v4, which I will send out later today.
I hope this patch set gets accepted for 5.12.