| Message ID | 20210210051814.845713-4-timur@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | add support for never printing hashed addresses | expand |
On 2/10/21 6:18 AM, Timur Tabi wrote: > If the make-printk-non-secret command line parameter is set, then > printk("%p") will print pointers as unhashed. This is useful for > debugging purposes. > > A large warning message is displayed if this option is enabled. > Unhashed pointers, while useful for debugging, 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> Thanks! > --- > .../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..6962379469e4 100644 > --- a/Documentation/admin-guide/kernel-parameters.txt > +++ b/Documentation/admin-guide/kernel-parameters.txt > @@ -2613,6 +2613,21 @@ > different yeeloong laptops. > Example: machtype=lemote-yeeloong-2f-7inch > > + make-printk-non-secret > + Force pointers printed to the console to be unhashed. > + By default, when a pointer is printed to the kernel > + console (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. If this 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. > + > max_addr=nn[KMG] [KNL,BOOT,ia64] All physical memory greater > than or equal to this physical address is ignored. > > 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..1296d9b0b328 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("make-printk-non-secret", 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 > + * make-printk-non-secret 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); > } > > /* >
On Tue 2021-02-09 23:18:14, Timur Tabi wrote: > If the make-printk-non-secret command line parameter is set, then > printk("%p") will print pointers as unhashed. This is useful for > debugging purposes. > > A large warning message is displayed if this option is enabled. > Unhashed pointers, while useful for debugging, 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> > --- > .../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..6962379469e4 100644 > --- a/Documentation/admin-guide/kernel-parameters.txt > +++ b/Documentation/admin-guide/kernel-parameters.txt > @@ -2613,6 +2613,21 @@ > different yeeloong laptops. > Example: machtype=lemote-yeeloong-2f-7inch > > + make-printk-non-secret > + Force pointers printed to the console to be unhashed. > + By default, when a pointer is printed to the kernel > + console (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. If this 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. > + > max_addr=nn[KMG] [KNL,BOOT,ia64] All physical memory greater > than or equal to this physical address is ignored. > > 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..1296d9b0b328 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("make-printk-non-secret", debug_never_hash_pointers_enable); The mail about EFENCE, and the different names of the parameter and the variable, made realized one important thing. The kernel parameter and the message does not describe the full effect. The option causes that vsprintf() will not hash pointers. Yes, it is primary used by printk(). But it is used also in some other interfaces, especially trace_printk(), seq_buf() API. The naked pointers might appear more or less anywhere, including procfs, sysfs, debugfs. IMHO, we should fix this. The long discussion was about how to make this option safe. Users should be aware that it is not only about the kernel log. I suggest to rename the parameter "debug_never_hash_pointer" and use the same name for the parameter and the variable. We also should make the warning more generic. I suggest to replace the first paragraph with something like: pr_warn("** The hashing of printed pointers has been disabled **\n"); pr_warn("** for debugging purposes. **\n"); Feel free to use a better wording. I am not a native speaker. Of course, also kernel-parameters.txt has to be updated accordingly. Best Regards, Petr
On 2/10/21 7:41 AM, Petr Mladek wrote: > > The option causes that vsprintf() will not hash pointers. Yes, it is > primary used by printk(). But it is used also in some other > interfaces, especially trace_printk(), seq_buf() API. The naked > pointers might appear more or less anywhere, including procfs, > sysfs, debugfs. Fair point. Shouldn't calls to seq_buf_printf() (and any printk usage that always exists in the context of a user-space process) use %pK anyway? Hmmm.... maybe vsprintf() should automatically replace %p with %pK if it detects a user-space context? > IMHO, we should fix this. The long discussion was about how to make > this option safe. Users should be aware that it is not only about > the kernel log. Agreed. > I suggest to rename the parameter "debug_never_hash_pointer" and use > the same name for the parameter and the variable. Will do. > We also should make the warning more generic. I suggest to replace the > first paragraph with something like: > > pr_warn("** The hashing of printed pointers has been disabled **\n"); > pr_warn("** for debugging purposes. **\n"); > > Feel free to use a better wording. I am not a native speaker. You could have fooled me. > Of course, also kernel-parameters.txt has to be updated accordingly. Ok.
Hi, I have realized that I did not comment the two ideas. On Wed 2021-02-10 11:27:45, Timur Tabi wrote: > > > On 2/10/21 7:41 AM, Petr Mladek wrote: > > > > The option causes that vsprintf() will not hash pointers. Yes, it is > > primary used by printk(). But it is used also in some other > > interfaces, especially trace_printk(), seq_buf() API. The naked > > pointers might appear more or less anywhere, including procfs, > > sysfs, debugfs. > > Fair point. Shouldn't calls to seq_buf_printf() (and any printk usage that > always exists in the context of a user-space process) use %pK anyway? seq_buf is a handy API that might be used for different purpose. For example, it seems to be used ftrace where people might want to see real pointers when debugging. > Hmmm.... maybe vsprintf() should automatically replace %p with %pK if it > detects a user-space context? I am not sure if there is an easy and reliable way how to detect the user-space context. On some architectures, it might be possible to guess it by the address of the buffer. But it will not work when the message is temporary printed into a local buffer and copied later. Let's keep it simple. Heuristics often become very complex over time. Best Regards, Petr
diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt index a10b545c2070..6962379469e4 100644 --- a/Documentation/admin-guide/kernel-parameters.txt +++ b/Documentation/admin-guide/kernel-parameters.txt @@ -2613,6 +2613,21 @@ different yeeloong laptops. Example: machtype=lemote-yeeloong-2f-7inch + make-printk-non-secret + Force pointers printed to the console to be unhashed. + By default, when a pointer is printed to the kernel + console (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. If this 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. + max_addr=nn[KMG] [KNL,BOOT,ia64] All physical memory greater than or equal to this physical address is ignored. 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..1296d9b0b328 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("make-printk-non-secret", 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 + * make-printk-non-secret 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 make-printk-non-secret command line parameter is set, then printk("%p") will print pointers as unhashed. This is useful for debugging purposes. A large warning message is displayed if this option is enabled. Unhashed pointers, while useful for debugging, 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> --- .../admin-guide/kernel-parameters.txt | 15 ++++++++ lib/test_printf.c | 8 ++++ lib/vsprintf.c | 38 ++++++++++++++++++- 3 files changed, 59 insertions(+), 2 deletions(-)