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 <[email protected]>
---
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);
}
/*
--
2.25.1
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 <[email protected]>
Looks good to me.
Acked-by: Sergey Senozhatsky <[email protected]>
-ss
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 <[email protected]>
>
> Looks good to me.
>
> Acked-by: Sergey Senozhatsky <[email protected]>
>
> -ss
Acked-by: Randy Dunlap <[email protected]>
thanks.
--
~Randy
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 <[email protected]>
> ---
> 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
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 <[email protected]>
> > ---
> > 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
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?
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
On Wed, 3 Feb 2021 12:02:05 -0800
Kees Cook <[email protected]> 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
On Wed, Feb 03, 2021 at 03:25:13PM -0500, Steven Rostedt wrote:
> On Wed, 3 Feb 2021 12:02:05 -0800
> Kees Cook <[email protected]> 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
On Wed, 3 Feb 2021 12:35:07 -0800
Kees Cook <[email protected]> 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
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.
On Wed, 3 Feb 2021 15:56:20 -0600
Timur Tabi <[email protected]> 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
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
On Wed 2021-02-03 15:47:27, Steven Rostedt wrote:
> On Wed, 3 Feb 2021 12:35:07 -0800
> Kees Cook <[email protected]> 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
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 <[email protected]>
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);
> }
>
> /*
>
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.
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 <[email protected]>
> ---
> 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
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.
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