Re: [PATCH][RESEND] lib/vsprintf: make-printk-non-secret printks all addresses as unhashed
On Fri, Feb 05, 2021 at 12:25:22PM -0600, Timur Tabi wrote: > I can extend make-printk-non-secret to %pK if everyone agrees. Let's just leave those alone. There is already a toggle for that in /proc. -- Kees Cook
Re: [PATCH][RESEND] lib/vsprintf: make-printk-non-secret printks all addresses as unhashed
On 2/9/21 3:59 PM, Marco Elver wrote: Would it be reasonable to make this non-static? Or somehow make it possible to get this flag from other subsystems? There are other places in the kernel that dump sensitive data such as registers. We'd like to be able to use 'debug_never_hash_pointers' to decide if our debugging tools can dump registers etc. What we really need is info if the kernel is in debug mode and we can dump all kinds of sensitive info; debug_never_hash_pointers is would be a good enough proxy for that. The next version of my patch (coming soon) will export the symbol. It's intended for test_printf, but if you think it can be used by others, so much the better.
Re: [PATCH][RESEND] lib/vsprintf: make-printk-non-secret printks all addresses as unhashed
On Tue, Feb 02, 2021 at 03:36PM -0600, Timur Tabi wrote:
> If the make-printk-non-secret command-line parameter is set, then
> printk("%p") will print addresses as unhashed. This is useful for
> debugging purposes.
>
> A large warning message is displayed if this option is enabled,
> because unhashed addresses, while useful for debugging, exposes
> kernel addresses which can be a security risk.
>
> Signed-off-by: Timur Tabi
> ---
> lib/vsprintf.c | 34 --
> 1 file changed, 32 insertions(+), 2 deletions(-)
>
> diff --git a/lib/vsprintf.c b/lib/vsprintf.c
> index 3b53c73580c5..b9f87084afb0 100644
> --- a/lib/vsprintf.c
> +++ b/lib/vsprintf.c
> @@ -2090,6 +2090,30 @@ 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 */
> +static bool debug_never_hash_pointers __ro_after_init;
Would it be reasonable to make this non-static? Or somehow make it
possible to get this flag from other subsystems?
There are other places in the kernel that dump sensitive data such as
registers. We'd like to be able to use 'debug_never_hash_pointers' to
decide if our debugging tools can dump registers etc. What we really
need is info if the kernel is in debug mode and we can dump all kinds of
sensitive info; debug_never_hash_pointers is would be a good enough
proxy for that.
Thanks,
-- Marco
Re: [PATCH][RESEND] lib/vsprintf: make-printk-non-secret printks all addresses as unhashed
On 2/5/21 4:59 AM, Vlastimil Babka wrote: Thanks a lot. Should this also affect %pK though? IIUC, there's currently no way to achieve non-mangled %pK in all cases, even with the most permissive kptr_restrict=1 setting: - in IRQ, there's "pK-error" instead - in a context of non-CAP_SYSLOG process, nulls are printed Hmmm.. I thought %pK prints an unhashed pointer when the user is root, at least in situations where the user can be known (e.g. during an ioctl call). Yes, neither should matter if %pK were only used for prints that generate content of some kind of /proc file read by a CAP_SYSLOG process, but that doesn't seem to be the case and there are %pK used for printing to dmesg too... I thought about that. On one hand, people who use %pK probably really wanted a hashed pointer printed. On the other hand, I agree that %pK should not be used for dmesg prints. I get the feeling that some (most?) people who use %pK don't really understand how it's supposed to be used. I can extend make-printk-non-secret to %pK if everyone agrees.
Re: [PATCH][RESEND] lib/vsprintf: make-printk-non-secret printks all addresses as unhashed
On 2/2/21 10:36 PM, Timur Tabi wrote:
> If the make-printk-non-secret command-line parameter is set, then
> printk("%p") will print addresses as unhashed. This is useful for
> debugging purposes.
>
> A large warning message is displayed if this option is enabled,
> because unhashed addresses, while useful for debugging, exposes
> kernel addresses which can be a security risk.
>
> Signed-off-by: Timur Tabi
Thanks a lot. Should this also affect %pK though? IIUC, there's currently no way
to achieve non-mangled %pK in all cases, even with the most permissive
kptr_restrict=1 setting:
- in IRQ, there's "pK-error" instead
- in a context of non-CAP_SYSLOG process, nulls are printed
Yes, neither should matter if %pK were only used for prints that generate
content of some kind of /proc file read by a CAP_SYSLOG process, but that
doesn't seem to be the case and there are %pK used for printing to dmesg too...
> ---
> lib/vsprintf.c | 34 --
> 1 file changed, 32 insertions(+), 2 deletions(-)
>
> diff --git a/lib/vsprintf.c b/lib/vsprintf.c
> index 3b53c73580c5..b9f87084afb0 100644
> --- a/lib/vsprintf.c
> +++ b/lib/vsprintf.c
> @@ -2090,6 +2090,30 @@ 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 */
> +static bool debug_never_hash_pointers __ro_after_init;
> +
> +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("** compromise security on 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 vendor! **\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 +2321,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);
> }
>
> /*
>
Re: [PATCH][RESEND] lib/vsprintf: make-printk-non-secret printks all addresses as unhashed
On Wed 2021-02-03 15:47:27, Steven Rostedt wrote:
> On Wed, 3 Feb 2021 12:35:07 -0800
> Kees Cook wrote:
>
> > > With a big notice that all pointers of unhashed, I don't think we need to
> > > print it failed when we expect it to fail.
> > >
> > > If anything, skip the test and state:
> > >
> > > test_printf: hash test skipped because "make-printk-non-secret" is on
> > > the
> > > command line.
> >
> > Yeah, I'm fine with "fail" or "skip". "pass" is mainly what I don't
> > like. :)
>
> Is there any printing of the tests being done? Looks to me that the tests
> only print something if they fail. Thus "skip" and "pass" are basically the
> same (if "skip" is simply not to do the test).
It prints the total number of tests done. It should not count the
skipped tests.
We actually print a warning when crng is not initialized. In this
case, the test passes because we actually check the value and it
is an expected one.
> I mean, we could simply have:
>
>
> static void __init
> plain(void)
> {
> int err;
>
> + if (debug_never_hash_pointers)
> + return;
I am not 100% sure. But this might work. Just please print a warning
about the tests are skipped.
Best Regards,
Petr
Re: [PATCH][RESEND] lib/vsprintf: make-printk-non-secret printks all addresses as unhashed
On Wed, 3 Feb 2021 15:56:20 -0600
Timur Tabi wrote:
> On 2/3/21 2:47 PM, Steven Rostedt wrote:
> > static void __init
> > plain(void)
> > {
> > int err;
> >
> > + if (debug_never_hash_pointers)
> > + return;
>
> So, I have a stupid question. What's the best way for test_printf.c to
> read the command line parameter? Should I just do this in vsprintf.c:
>
> /* Disable pointer hashing if requested */
> static bool debug_never_hash_pointers __ro_after_init;
It wont be static.
> EXPORT_SYMBOL_GPL(debug_never_hash_pointers);
>
> I'm not crazy about exporting this variable to other drivers. It could
> be used to disable hashing by any driver.
But it is set as "__ro_after_init". That is, every module will see it as
read only. IOW, they wont be able to modify it.
>
> AFAIK, the only command-line parameter code that works in drivers is
> module_parm, and that expects the module prefix on the command-line.
This is just a constant variable for others to see. The command line itself
is visible (see saved_command_line, it's even exported to modules in sparc).
-- Steve
Re: [PATCH][RESEND] lib/vsprintf: make-printk-non-secret printks all addresses as unhashed
On 2/3/21 2:47 PM, Steven Rostedt wrote:
static void __init
plain(void)
{
int err;
+ if (debug_never_hash_pointers)
+ return;
So, I have a stupid question. What's the best way for test_printf.c to
read the command line parameter? Should I just do this in vsprintf.c:
/* Disable pointer hashing if requested */
static bool debug_never_hash_pointers __ro_after_init;
EXPORT_SYMBOL_GPL(debug_never_hash_pointers);
I'm not crazy about exporting this variable to other drivers. It could
be used to disable hashing by any driver.
AFAIK, the only command-line parameter code that works in drivers is
module_parm, and that expects the module prefix on the command-line.
Re: [PATCH][RESEND] lib/vsprintf: make-printk-non-secret printks all addresses as unhashed
On Wed, 3 Feb 2021 12:35:07 -0800
Kees Cook wrote:
> > With a big notice that all pointers of unhashed, I don't think we need to
> > print it failed when we expect it to fail.
> >
> > If anything, skip the test and state:
> >
> > test_printf: hash test skipped because "make-printk-non-secret" is on the
> > command line.
>
> Yeah, I'm fine with "fail" or "skip". "pass" is mainly what I don't
> like. :)
Is there any printing of the tests being done? Looks to me that the tests
only print something if they fail. Thus "skip" and "pass" are basically the
same (if "skip" is simply not to do the test).
I mean, we could simply have:
static void __init
plain(void)
{
int err;
+ if (debug_never_hash_pointers)
+ return;
-- Steve
Re: [PATCH][RESEND] lib/vsprintf: make-printk-non-secret printks all addresses as unhashed
On Wed, Feb 03, 2021 at 03:25:13PM -0500, Steven Rostedt wrote: > On Wed, 3 Feb 2021 12:02:05 -0800 > Kees Cook wrote: > > > On Wed, Feb 03, 2021 at 12:58:41PM -0600, Timur Tabi wrote: > > > On 2/3/21 7:31 AM, Petr Mladek wrote: > > > > Also please make sure that lib/test_printf.c will work with > > > > the new option. > > > > > > As you suspected, it doesn't work: > > > > > > [ 206.966478] test_printf: loaded. > > > [ 206.966528] test_printf: plain 'p' does not appear to be hashed > > > [ 206.966740] test_printf: failed 1 out of 388 tests > > > > > > What should I do about this? > > > > > > On one hand, it is working as expected: %p is not hashed, and that should > > > be > > > a warning. > > > > > > On the other hand, maybe test_printf should be aware of the command line > > > parameter and test to make sure that %p is NOT hashed? > > > > It seems like it'd be best for the test to fail, yes? It _is_ a problem > > that %p is unhashed; it's just that the failure was intended. > > > > I disagree. > > With a big notice that all pointers of unhashed, I don't think we need to > print it failed when we expect it to fail. > > If anything, skip the test and state: > > test_printf: hash test skipped because "make-printk-non-secret" is on the > command line. Yeah, I'm fine with "fail" or "skip". "pass" is mainly what I don't like. :) -- Kees Cook
Re: [PATCH][RESEND] lib/vsprintf: make-printk-non-secret printks all addresses as unhashed
On Wed, 3 Feb 2021 12:02:05 -0800 Kees Cook wrote: > On Wed, Feb 03, 2021 at 12:58:41PM -0600, Timur Tabi wrote: > > On 2/3/21 7:31 AM, Petr Mladek wrote: > > > Also please make sure that lib/test_printf.c will work with > > > the new option. > > > > As you suspected, it doesn't work: > > > > [ 206.966478] test_printf: loaded. > > [ 206.966528] test_printf: plain 'p' does not appear to be hashed > > [ 206.966740] test_printf: failed 1 out of 388 tests > > > > What should I do about this? > > > > On one hand, it is working as expected: %p is not hashed, and that should be > > a warning. > > > > On the other hand, maybe test_printf should be aware of the command line > > parameter and test to make sure that %p is NOT hashed? > > It seems like it'd be best for the test to fail, yes? It _is_ a problem > that %p is unhashed; it's just that the failure was intended. > I disagree. With a big notice that all pointers of unhashed, I don't think we need to print it failed when we expect it to fail. If anything, skip the test and state: test_printf: hash test skipped because "make-printk-non-secret" is on the command line. -- Steve
Re: [PATCH][RESEND] lib/vsprintf: make-printk-non-secret printks all addresses as unhashed
On Wed, Feb 03, 2021 at 12:58:41PM -0600, Timur Tabi wrote: > On 2/3/21 7:31 AM, Petr Mladek wrote: > > Also please make sure that lib/test_printf.c will work with > > the new option. > > As you suspected, it doesn't work: > > [ 206.966478] test_printf: loaded. > [ 206.966528] test_printf: plain 'p' does not appear to be hashed > [ 206.966740] test_printf: failed 1 out of 388 tests > > What should I do about this? > > On one hand, it is working as expected: %p is not hashed, and that should be > a warning. > > On the other hand, maybe test_printf should be aware of the command line > parameter and test to make sure that %p is NOT hashed? It seems like it'd be best for the test to fail, yes? It _is_ a problem that %p is unhashed; it's just that the failure was intended. -- Kees Cook
Re: [PATCH][RESEND] lib/vsprintf: make-printk-non-secret printks all addresses as unhashed
On Wed, Feb 03, 2021 at 12:58:41PM -0600, Timur Tabi wrote: > On 2/3/21 7:31 AM, Petr Mladek wrote: > > Also please make sure that lib/test_printf.c will work with > > the new option. > > As you suspected, it doesn't work: > > [ 206.966478] test_printf: loaded. > [ 206.966528] test_printf: plain 'p' does not appear to be hashed > [ 206.966740] test_printf: failed 1 out of 388 tests > > What should I do about this? > > On one hand, it is working as expected: %p is not hashed, and that should be > a warning. > > On the other hand, maybe test_printf should be aware of the command line > parameter and test to make sure that %p is NOT hashed? test_printf.c should be altered accordingly to avoid any failed test cases. I.o.w. you need to have some kind of conditional there: if (kernel_cmdline_parameter_foo) expect bar else expect baz -- With Best Regards, Andy Shevchenko
Re: [PATCH][RESEND] lib/vsprintf: make-printk-non-secret printks all addresses as unhashed
On 2/3/21 7:31 AM, Petr Mladek wrote: Also please make sure that lib/test_printf.c will work with the new option. As you suspected, it doesn't work: [ 206.966478] test_printf: loaded. [ 206.966528] test_printf: plain 'p' does not appear to be hashed [ 206.966740] test_printf: failed 1 out of 388 tests What should I do about this? On one hand, it is working as expected: %p is not hashed, and that should be a warning. On the other hand, maybe test_printf should be aware of the command line parameter and test to make sure that %p is NOT hashed?
Re: [PATCH][RESEND] lib/vsprintf: make-printk-non-secret printks all addresses as unhashed
On Wed 2021-02-03 10:54:24, Petr Mladek wrote:
> On Tue 2021-02-02 15:36:33, Timur Tabi wrote:
> > If the make-printk-non-secret command-line parameter is set, then
> > printk("%p") will print addresses as unhashed. This is useful for
> > debugging purposes.
> >
> > A large warning message is displayed if this option is enabled,
> > because unhashed addresses, while useful for debugging, exposes
> > kernel addresses which can be a security risk.
> >
> > Signed-off-by: Timur Tabi
> > ---
> > lib/vsprintf.c | 34 --
> > 1 file changed, 32 insertions(+), 2 deletions(-)
>
> Please, add also entry into
> Documentation/admin-guide/kernel-parameters.txt.
Adding Andy and Rasmus into CC. They are official vsprintf
co-maintainers and reviewers (MAINTAINERS file).
Also please make sure that lib/test_printf.c will work with
the new option.
Best Regards,
Petr
Re: [PATCH][RESEND] lib/vsprintf: make-printk-non-secret printks all addresses as unhashed
On Tue 2021-02-02 15:36:33, Timur Tabi wrote:
> If the make-printk-non-secret command-line parameter is set, then
> printk("%p") will print addresses as unhashed. This is useful for
> debugging purposes.
>
> A large warning message is displayed if this option is enabled,
> because unhashed addresses, while useful for debugging, exposes
> kernel addresses which can be a security risk.
>
> Signed-off-by: Timur Tabi
> ---
> lib/vsprintf.c | 34 --
> 1 file changed, 32 insertions(+), 2 deletions(-)
Please, add also entry into
Documentation/admin-guide/kernel-parameters.txt.
If we agree that the parameter is acceptable then let's make
it properly.
> diff --git a/lib/vsprintf.c b/lib/vsprintf.c
> index 3b53c73580c5..b9f87084afb0 100644
> --- a/lib/vsprintf.c
> +++ b/lib/vsprintf.c
> @@ -2090,6 +2090,30 @@ 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 */
> +static bool debug_never_hash_pointers __ro_after_init;
> +
> +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("** compromise security on 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 vendor! **\n");
It is a boot parameter. So it should be "system administrtor" instead
of vendor.
Otherwise, it looks good to me.
Best Regards,
Petr
Re: [PATCH][RESEND] lib/vsprintf: make-printk-non-secret printks all addresses as unhashed
On 2/2/21 7:24 PM, Sergey Senozhatsky wrote:
> On (21/02/02 15:36), Timur Tabi wrote:
>> If the make-printk-non-secret command-line parameter is set, then
>> printk("%p") will print addresses as unhashed. This is useful for
>> debugging purposes.
>>
>> A large warning message is displayed if this option is enabled,
>> because unhashed addresses, while useful for debugging, exposes
>> kernel addresses which can be a security risk.
>>
>> Signed-off-by: Timur Tabi
>
> Looks good to me.
>
> Acked-by: Sergey Senozhatsky
>
> -ss
Acked-by: Randy Dunlap
thanks.
--
~Randy
Re: [PATCH][RESEND] lib/vsprintf: make-printk-non-secret printks all addresses as unhashed
On (21/02/02 15:36), Timur Tabi wrote:
> If the make-printk-non-secret command-line parameter is set, then
> printk("%p") will print addresses as unhashed. This is useful for
> debugging purposes.
>
> A large warning message is displayed if this option is enabled,
> because unhashed addresses, while useful for debugging, exposes
> kernel addresses which can be a security risk.
>
> Signed-off-by: Timur Tabi
Looks good to me.
Acked-by: Sergey Senozhatsky
-ss
