| Message ID | 20210210213453.1504219-4-timur@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | add support for never printing hashed addresses | expand |
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
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 <timur@kernel.org> > Acked-by: Petr Mladek <pmladek@suse.com> > Acked-by: Randy Dunlap <rdunlap@infradead.org> > Acked-by: Sergey Senozhatsky <sergey.senozhatsky@gmail.com> > Acked-by: Vlastimil Babka <vbabka@suse.cz> > --- > .../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: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 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 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 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.
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); } /*
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 <timur@kernel.org> Acked-by: Petr Mladek <pmladek@suse.com> Acked-by: Randy Dunlap <rdunlap@infradead.org> Acked-by: Sergey Senozhatsky <sergey.senozhatsky@gmail.com> Acked-by: Vlastimil Babka <vbabka@suse.cz> --- .../admin-guide/kernel-parameters.txt | 15 ++++++++ lib/test_printf.c | 8 ++++ lib/vsprintf.c | 38 ++++++++++++++++++- 3 files changed, 59 insertions(+), 2 deletions(-)