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().
Timur Tabi (3):
[v4] lib: use KSTM_MODULE_GLOBALS macro in kselftest drivers
[v4] kselftest: add support for skipped tests
[v4] 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]>
Reviewed-by: Petr Mladek <[email protected]>
Tested-by: Petr Mladek <[email protected]>
Acked-by: Marco Elver <[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
If the no_hash_pointers command line parameter is set, then
printk("%p") will print pointers as unhashed, which is useful for
debugging purposes. This change 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]>
Acked-by: Marco Elver <[email protected]>
---
.../admin-guide/kernel-parameters.txt | 15 ++++++++
lib/test_printf.c | 8 +++++
lib/vsprintf.c | 36 +++++++++++++++++--
3 files changed, 57 insertions(+), 2 deletions(-)
diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
index a10b545c2070..c8993a296e71 100644
--- a/Documentation/admin-guide/kernel-parameters.txt
+++ b/Documentation/admin-guide/kernel-parameters.txt
@@ -3281,6 +3281,21 @@
in certain environments such as networked servers or
real-time systems.
+ no_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.
+
nohibernate [HIBERNATION] Disable hibernation and resume.
nohz= [KNL] Boottime enable/disable dynamic ticks
diff --git a/lib/test_printf.c b/lib/test_printf.c
index ad2bcfa8caa1..a6755798e9e6 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 no_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 (no_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..41ddc353ebb8 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -2090,6 +2090,32 @@ 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 no_hash_pointers __ro_after_init;
+EXPORT_SYMBOL_GPL(no_hash_pointers);
+
+static int __init no_hash_pointers_enable(char *str)
+{
+ no_hash_pointers = true;
+
+ pr_warn("**********************************************************\n");
+ pr_warn("** NOTICE NOTICE NOTICE NOTICE NOTICE NOTICE NOTICE **\n");
+ pr_warn("** **\n");
+ pr_warn("** This system shows unhashed kernel memory addresses **\n");
+ pr_warn("** via the console, logs, and other interfaces. This **\n");
+ pr_warn("** might 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("no_hash_pointers", no_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 +2323,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 no_hash_pointers is specified on the command line.
+ */
+ if (unlikely(no_hash_pointers))
+ return pointer_string(buf, end, ptr, spec);
+ else
+ return ptr_to_id(buf, end, ptr, spec);
}
/*
--
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]>
Acked-by: Marco Elver <[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
On 2/14/21 10:13 AM, Timur Tabi wrote:
> 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.
I believe that this version addresses all outstanding issues, so unless
there are any complaints, I would like for this patch set to be merged
for 5.12-rc1. I don't know who should pick it up, though.
Thanks.
On Sun 2021-02-14 10:18:39, Timur Tabi wrote:
> On 2/14/21 10:13 AM, Timur Tabi wrote:
> > 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.
>
> I believe that this version addresses all outstanding issues, so unless
> there are any complaints, I would like for this patch set to be merged for
> 5.12-rc1. I don't know who should pick it up, though.
I have pushed the patchset into printk/linux.git,
branch for-5.12-no_hash_pointers.
I am going to send the pull request on Thursday. Anyone still could
comment and even stop it until them. I just wanted to give it at
least few days in linux-next.
Best Regards,
Petr
Hi Timur,
On Sun, Feb 14, 2021 at 5:17 PM Timur Tabi <[email protected]> wrote:
> If the no_hash_pointers command line parameter is set, then
> printk("%p") will print pointers as unhashed, which is useful for
> debugging purposes. This change 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]>
Thanks for your patch, which is now commit 5ead723a20e0447b
("lib/vsprintf: no_hash_pointers prints all addresses as unhashed") in
v5.12-rc1.
> --- a/lib/vsprintf.c
> +++ b/lib/vsprintf.c
> @@ -2090,6 +2090,32 @@ 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 no_hash_pointers __ro_after_init;
> +EXPORT_SYMBOL_GPL(no_hash_pointers);
> +
> +static int __init no_hash_pointers_enable(char *str)
> +{
> + no_hash_pointers = true;
> +
> + pr_warn("**********************************************************\n");
> + pr_warn("** NOTICE NOTICE NOTICE NOTICE NOTICE NOTICE NOTICE **\n");
> + pr_warn("** **\n");
> + pr_warn("** This system shows unhashed kernel memory addresses **\n");
> + pr_warn("** via the console, logs, and other interfaces. This **\n");
> + pr_warn("** might 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("no_hash_pointers", no_hash_pointers_enable);
While bloat-o-meter is not smart enough to notice the real size impact,
this does add more than 500 bytes of string data to the kernel.
Do we really need such a large message?
Perhaps the whole no_hash_pointers machinery should be protected by
"#ifdef CONFIG_DEBUG_KERNEL"?
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
On Tue, 2 Mar 2021 at 12:51, Geert Uytterhoeven <[email protected]> wrote:
> Hi Timur,
>
> On Sun, Feb 14, 2021 at 5:17 PM Timur Tabi <[email protected]> wrote:
> > If the no_hash_pointers command line parameter is set, then
> > printk("%p") will print pointers as unhashed, which is useful for
> > debugging purposes. This change 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]>
>
> Thanks for your patch, which is now commit 5ead723a20e0447b
> ("lib/vsprintf: no_hash_pointers prints all addresses as unhashed") in
> v5.12-rc1.
>
> > --- a/lib/vsprintf.c
> > +++ b/lib/vsprintf.c
> > @@ -2090,6 +2090,32 @@ 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 no_hash_pointers __ro_after_init;
> > +EXPORT_SYMBOL_GPL(no_hash_pointers);
> > +
> > +static int __init no_hash_pointers_enable(char *str)
> > +{
> > + no_hash_pointers = true;
> > +
> > + pr_warn("**********************************************************\n");
> > + pr_warn("** NOTICE NOTICE NOTICE NOTICE NOTICE NOTICE NOTICE **\n");
> > + pr_warn("** **\n");
> > + pr_warn("** This system shows unhashed kernel memory addresses **\n");
> > + pr_warn("** via the console, logs, and other interfaces. This **\n");
> > + pr_warn("** might 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("no_hash_pointers", no_hash_pointers_enable);
>
> While bloat-o-meter is not smart enough to notice the real size impact,
> this does add more than 500 bytes of string data to the kernel.
> Do we really need such a large message?
> Perhaps the whole no_hash_pointers machinery should be protected by
> "#ifdef CONFIG_DEBUG_KERNEL"?
We recently stumbled across this, and it appears an increasing number
of production kernels enable CONFIG_DEBUG_KERNEL [1], so it likely
isn't the solution (we tried to use CONFIG_DEBUG_KERNEL in similar
way, and it wasn't reliable). Having no_hash_pointers frees us of
having to rely on CONFIG_DEBUG_KERNEL. (Perhaps somebody else will
comment, but I believe there were strong objections to making the
pointer hashing dependent on more Kconfig options.)
[1] https://lkml.kernel.org/r/[email protected]
Would placing the strings into an __initconst array help?
Thanks,
-- Marco
Hi Marco,
On Tue, Mar 2, 2021 at 1:45 PM Marco Elver <[email protected]> wrote:
> On Tue, 2 Mar 2021 at 12:51, Geert Uytterhoeven <[email protected]> wrote:
> > On Sun, Feb 14, 2021 at 5:17 PM Timur Tabi <[email protected]> wrote:
> > > If the no_hash_pointers command line parameter is set, then
> > > printk("%p") will print pointers as unhashed, which is useful for
> > > debugging purposes. This change 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]>
> >
> > Thanks for your patch, which is now commit 5ead723a20e0447b
> > ("lib/vsprintf: no_hash_pointers prints all addresses as unhashed") in
> > v5.12-rc1.
> >
> > > --- a/lib/vsprintf.c
> > > +++ b/lib/vsprintf.c
> > > @@ -2090,6 +2090,32 @@ 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 no_hash_pointers __ro_after_init;
> > > +EXPORT_SYMBOL_GPL(no_hash_pointers);
> > > +
> > > +static int __init no_hash_pointers_enable(char *str)
> > > +{
> > > + no_hash_pointers = true;
> > > +
> > > + pr_warn("**********************************************************\n");
> > > + pr_warn("** NOTICE NOTICE NOTICE NOTICE NOTICE NOTICE NOTICE **\n");
> > > + pr_warn("** **\n");
> > > + pr_warn("** This system shows unhashed kernel memory addresses **\n");
> > > + pr_warn("** via the console, logs, and other interfaces. This **\n");
> > > + pr_warn("** might 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("no_hash_pointers", no_hash_pointers_enable);
> >
> > While bloat-o-meter is not smart enough to notice the real size impact,
> > this does add more than 500 bytes of string data to the kernel.
> > Do we really need such a large message?
> > Perhaps the whole no_hash_pointers machinery should be protected by
> > "#ifdef CONFIG_DEBUG_KERNEL"?
>
> We recently stumbled across this, and it appears an increasing number
> of production kernels enable CONFIG_DEBUG_KERNEL [1], so it likely
> isn't the solution (we tried to use CONFIG_DEBUG_KERNEL in similar
I guess the people who do care about kernel size do know to disable
CONFIG_DEBUG_KERNEL, so it would help them.
The everything-but-the-kitchen-sink distro people don't care about kernel
size anyway.
> way, and it wasn't reliable). Having no_hash_pointers frees us of
> having to rely on CONFIG_DEBUG_KERNEL. (Perhaps somebody else will
> comment, but I believe there were strong objections to making the
> pointer hashing dependent on more Kconfig options.)
>
> [1] https://lkml.kernel.org/r/[email protected]
>
> Would placing the strings into an __initconst array help?
That would indeed help to reduce run-time memory consumption.
It would not solve the raw kernel size increase.
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
On Tue 2021-03-02 13:51:35, Geert Uytterhoeven wrote:
> Hi Marco,
>
> On Tue, Mar 2, 2021 at 1:45 PM Marco Elver <[email protected]> wrote:
> > On Tue, 2 Mar 2021 at 12:51, Geert Uytterhoeven <[email protected]> wrote:
> > > On Sun, Feb 14, 2021 at 5:17 PM Timur Tabi <[email protected]> wrote:
> > > > If the no_hash_pointers command line parameter is set, then
> > > > printk("%p") will print pointers as unhashed, which is useful for
> > > > debugging purposes. This change 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.
> > > >
> > > > --- a/lib/vsprintf.c
> > > > +++ b/lib/vsprintf.c
> > > > @@ -2090,6 +2090,32 @@ 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 no_hash_pointers __ro_after_init;
> > > > +EXPORT_SYMBOL_GPL(no_hash_pointers);
> > > > +
> > > > +static int __init no_hash_pointers_enable(char *str)
> > > > +{
> > > > + no_hash_pointers = true;
> > > > +
> > > > + pr_warn("**********************************************************\n");
> > > > + pr_warn("** NOTICE NOTICE NOTICE NOTICE NOTICE NOTICE NOTICE **\n");
> > > > + pr_warn("** **\n");
> > > > + pr_warn("** This system shows unhashed kernel memory addresses **\n");
> > > > + pr_warn("** via the console, logs, and other interfaces. This **\n");
> > > > + pr_warn("** might 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("no_hash_pointers", no_hash_pointers_enable);
> > >
> > > While bloat-o-meter is not smart enough to notice the real size impact,
> > > this does add more than 500 bytes of string data to the kernel.
> > > Do we really need such a large message?
> > > Perhaps the whole no_hash_pointers machinery should be protected by
> > > "#ifdef CONFIG_DEBUG_KERNEL"?
This was the deal. The configure option is a no-go, see below and also
https://lore.kernel.org/r/CAHk-=wgaK4cz=K-JB4p-KPXBV73m9bja2w1W1Lr3iu8+NEPk7A@mail.gmail.com
> > We recently stumbled across this, and it appears an increasing number
> > of production kernels enable CONFIG_DEBUG_KERNEL [1], so it likely
> > isn't the solution (we tried to use CONFIG_DEBUG_KERNEL in similar
>
> I guess the people who do care about kernel size do know to disable
> CONFIG_DEBUG_KERNEL, so it would help them.
> The everything-but-the-kitchen-sink distro people don't care about kernel
> size anyway.
The problem with the configure option is not about size. The problem is
that there would be many kernels in the wild with this option enabled.
People would install them without knowing that they are less secure.
Distros would need to provide both kernels. Well, they already do.
But it might be worse. Some distros might even want to enable it
by default.
Also many bugs might be debugged without this option. Some bugs
are hard to reproduce and might be visible only on production
systems. It should be possible to enable this only when really
needed and the user must be aware of the risk.
> > Would placing the strings into an __initconst array help?
>
> That would indeed help to reduce run-time memory consumption.
Sure. We could do this. Do you want to send a patch, please?
> It would not solve the raw kernel size increase.
I see. Well, the compression should be pretty efficient
for a text (with many spaces).
Best Regards,
Petr
On 3/2/21 2:29 PM, Petr Mladek wrote:
> On Tue 2021-03-02 13:51:35, Geert Uytterhoeven wrote:
>> > > > +
>> > > > + pr_warn("**********************************************************\n");
>> > > > + pr_warn("** NOTICE NOTICE NOTICE NOTICE NOTICE NOTICE NOTICE **\n");
>> > > > + pr_warn("** **\n");
>> > > > + pr_warn("** This system shows unhashed kernel memory addresses **\n");
>> > > > + pr_warn("** via the console, logs, and other interfaces. This **\n");
>> > > > + pr_warn("** might 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("no_hash_pointers", no_hash_pointers_enable);
>> > >
>> > > While bloat-o-meter is not smart enough to notice the real size impact,
>> > > this does add more than 500 bytes of string data to the kernel.
>> > > Do we really need such a large message?
>> > > Perhaps the whole no_hash_pointers machinery should be protected by
>> > > "#ifdef CONFIG_DEBUG_KERNEL"?
>
> This was the deal. The configure option is a no-go, see below and also
> https://lore.kernel.org/r/CAHk-=wgaK4cz=K-JB4p-KPXBV73m9bja2w1W1Lr3iu8+NEPk7A@mail.gmail.com
I think it's a no-go only when enabling such option equals to "no_hash_pointers"
being always passed. What Geert suggests is that you need both
CONFIG_DEBUG_KERNEL *and* no_hash_pointers and that's different.
>> > We recently stumbled across this, and it appears an increasing number
>> > of production kernels enable CONFIG_DEBUG_KERNEL [1], so it likely
>> > isn't the solution (we tried to use CONFIG_DEBUG_KERNEL in similar
>>
>> I guess the people who do care about kernel size do know to disable
>> CONFIG_DEBUG_KERNEL, so it would help them.
>> The everything-but-the-kitchen-sink distro people don't care about kernel
>> size anyway.
>
> The problem with the configure option is not about size. The problem is
> that there would be many kernels in the wild with this option enabled.
> People would install them without knowing that they are less secure.
Same as above.
> Distros would need to provide both kernels. Well, they already do.
> But it might be worse. Some distros might even want to enable it
> by default.
>
> Also many bugs might be debugged without this option. Some bugs
> are hard to reproduce and might be visible only on production
> systems. It should be possible to enable this only when really
> needed and the user must be aware of the risk.
So this is basically a kernel tinyfication issue, right? Is that still pursued
today? Are there better config options suitable for this than CONFIG_DEBUG_KERNEL?
>> > Would placing the strings into an __initconst array help?
>>
>> That would indeed help to reduce run-time memory consumption.
>
> Sure. We could do this. Do you want to send a patch, please?
>
>> It would not solve the raw kernel size increase.
>
> I see. Well, the compression should be pretty efficient
> for a text (with many spaces).
>
> Best Regards,
> Petr
>
On Tue, 2 Mar 2021 14:49:42 +0100
Geert Uytterhoeven <[email protected]> wrote:
> > So this is basically a kernel tinyfication issue, right? Is that still pursued
> > today? Are there better config options suitable for this than CONFIG_DEBUG_KERNEL?
>
> As long as I hear about products running Linux on SoCs with 10 MiB of
> SRAM, I think the answer is yes.
> I'm not immediately aware of a better config option. There are no more
> TINY options left, and EXPERT selects DEBUG_KERNEL.
Since the trace_printk() uses the same type of notice, I wonder if we could
make this into a helper function and just pass in the top part.
+ pr_warn("**********************************************************\n");
+ pr_warn("** NOTICE NOTICE NOTICE NOTICE NOTICE NOTICE NOTICE **\n");
+ pr_warn("** **\n");
+ pr_warn("** This system shows unhashed kernel memory addresses **\n");
+ pr_warn("** via the console, logs, and other interfaces. This **\n");
+ pr_warn("** might reduce the security of your system. **\n");
Only the above section is really unique. The rest can be a boiler plate.
-- Steve
+ 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");
+
Hi Vlastimil, Petr,
On Tue, Mar 2, 2021 at 2:37 PM Vlastimil Babka <[email protected]> wrote:
> On 3/2/21 2:29 PM, Petr Mladek wrote:
> > On Tue 2021-03-02 13:51:35, Geert Uytterhoeven wrote:
> >> > > > +
> >> > > > + pr_warn("**********************************************************\n");
> >> > > > + pr_warn("** NOTICE NOTICE NOTICE NOTICE NOTICE NOTICE NOTICE **\n");
> >> > > > + pr_warn("** **\n");
> >> > > > + pr_warn("** This system shows unhashed kernel memory addresses **\n");
> >> > > > + pr_warn("** via the console, logs, and other interfaces. This **\n");
> >> > > > + pr_warn("** might 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("no_hash_pointers", no_hash_pointers_enable);
> >> > >
> >> > > While bloat-o-meter is not smart enough to notice the real size impact,
> >> > > this does add more than 500 bytes of string data to the kernel.
> >> > > Do we really need such a large message?
> >> > > Perhaps the whole no_hash_pointers machinery should be protected by
> >> > > "#ifdef CONFIG_DEBUG_KERNEL"?
> >
> > This was the deal. The configure option is a no-go, see below and also
> > https://lore.kernel.org/r/CAHk-=wgaK4cz=K-JB4p-KPXBV73m9bja2w1W1Lr3iu8+NEPk7A@mail.gmail.com
>
> I think it's a no-go only when enabling such option equals to "no_hash_pointers"
> being always passed. What Geert suggests is that you need both
> CONFIG_DEBUG_KERNEL *and* no_hash_pointers and that's different.
Exactly.
> >> > We recently stumbled across this, and it appears an increasing number
> >> > of production kernels enable CONFIG_DEBUG_KERNEL [1], so it likely
> >> > isn't the solution (we tried to use CONFIG_DEBUG_KERNEL in similar
> >>
> >> I guess the people who do care about kernel size do know to disable
> >> CONFIG_DEBUG_KERNEL, so it would help them.
> >> The everything-but-the-kitchen-sink distro people don't care about kernel
> >> size anyway.
> >
> > The problem with the configure option is not about size. The problem is
> > that there would be many kernels in the wild with this option enabled.
> > People would install them without knowing that they are less secure.
>
> Same as above.
>
> > Distros would need to provide both kernels. Well, they already do.
> > But it might be worse. Some distros might even want to enable it
> > by default.
> >
> > Also many bugs might be debugged without this option. Some bugs
> > are hard to reproduce and might be visible only on production
> > systems. It should be possible to enable this only when really
> > needed and the user must be aware of the risk.
>
> So this is basically a kernel tinyfication issue, right? Is that still pursued
> today? Are there better config options suitable for this than CONFIG_DEBUG_KERNEL?
As long as I hear about products running Linux on SoCs with 10 MiB of
SRAM, I think the answer is yes.
I'm not immediately aware of a better config option. There are no more
TINY options left, and EXPERT selects DEBUG_KERNEL.
> >> > Would placing the strings into an __initconst array help?
> >>
> >> That would indeed help to reduce run-time memory consumption.
> >
> > Sure. We could do this. Do you want to send a patch, please?
Added to my list.
> >> It would not solve the raw kernel size increase.
> >
> > I see. Well, the compression should be pretty efficient
> > for a text (with many spaces).
My worry is not about the medium for storing the kernel image, but the
RAM where the kernel image is loaded. The former is usually less
restricted in size, and easier to expand, than the latter,
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
On Tue, Mar 02, 2021 at 03:26:50PM +0100, Marco Elver wrote:
> +static const char no_hash_pointers_warning[9][55] __initconst = {
> + "******************************************************",
> + " NOTICE NOTICE NOTICE NOTICE NOTICE NOTICE NOTICE ",
> + " ",
> + " This system shows unhashed kernel memory addresses ",
> + " via the console, logs, and other interfaces. This ",
> + " might reduce the security of your system. ",
> + " If you see this message and you are not debugging ",
> + " the kernel, report this immediately to your system ",
> + " administrator! ",
> +};
> +
> static int __init no_hash_pointers_enable(char *str)
> {
> + const int lines[] = { 0, 1, 2, 3, 4, 5, 2, 6, 7, 8, 2, 1, 0 };
> + int i;
> +
> no_hash_pointers = true;
>
> - pr_warn("**********************************************************\n");
> - pr_warn("** NOTICE NOTICE NOTICE NOTICE NOTICE NOTICE NOTICE **\n");
> - pr_warn("** **\n");
> - pr_warn("** This system shows unhashed kernel memory addresses **\n");
> - pr_warn("** via the console, logs, and other interfaces. This **\n");
> - pr_warn("** might 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");
> + for (i = 0; i < ARRAY_SIZE(lines); i++)
> + pr_warn("**%s**\n", no_hash_pointers_warning[lines[i]]);
+ for (i = 0; i < 3; i++)
+ pr_warn("**%s**\n", no_hash_pointers_warning[lines[2 - i]]);
On Tue, Mar 02, 2021 at 09:08AM -0500, Steven Rostedt wrote:
> On Tue, 2 Mar 2021 14:49:42 +0100
> Geert Uytterhoeven <[email protected]> wrote:
>
> > > So this is basically a kernel tinyfication issue, right? Is that still pursued
> > > today? Are there better config options suitable for this than CONFIG_DEBUG_KERNEL?
> >
> > As long as I hear about products running Linux on SoCs with 10 MiB of
> > SRAM, I think the answer is yes.
> > I'm not immediately aware of a better config option. There are no more
> > TINY options left, and EXPERT selects DEBUG_KERNEL.
>
> Since the trace_printk() uses the same type of notice, I wonder if we could
> make this into a helper function and just pass in the top part.
>
> + pr_warn("**********************************************************\n");
> + pr_warn("** NOTICE NOTICE NOTICE NOTICE NOTICE NOTICE NOTICE **\n");
> + pr_warn("** **\n");
>
>
> + pr_warn("** This system shows unhashed kernel memory addresses **\n");
> + pr_warn("** via the console, logs, and other interfaces. This **\n");
> + pr_warn("** might reduce the security of your system. **\n");
>
> Only the above section is really unique. The rest can be a boiler plate.
Short of procedurally generating some of these strings, I was
experimenting with the below.
Would that be reasonable?
Thanks,
-- Marco
------ >8 ------
From: Marco Elver <[email protected]>
Date: Tue, 2 Mar 2021 15:07:28 +0100
Subject: [PATCH] lib/vsprintf: reduce space taken by no_hash_pointers warning
Move the no_hash_pointers warning string into __initconst section, so
that it is discarded after init. Remove common start/end characters and
remove repeated lines from the array.
Link: https://lkml.kernel.org/r/CAMuHMdULKZCJevVJcp7TxzLdWLjsQPhE8hqxhnztNi9bjT_cEw@mail.gmail.com
Reported-by: Geert Uytterhoeven <[email protected]>
Signed-off-by: Marco Elver <[email protected]>
---
lib/vsprintf.c | 30 +++++++++++++++++-------------
1 file changed, 17 insertions(+), 13 deletions(-)
diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index 41ddc353ebb8..1e63b43955f6 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -2094,23 +2094,27 @@ char *fwnode_string(char *buf, char *end, struct fwnode_handle *fwnode,
bool no_hash_pointers __ro_after_init;
EXPORT_SYMBOL_GPL(no_hash_pointers);
+static const char no_hash_pointers_warning[9][55] __initconst = {
+ "******************************************************",
+ " NOTICE NOTICE NOTICE NOTICE NOTICE NOTICE NOTICE ",
+ " ",
+ " This system shows unhashed kernel memory addresses ",
+ " via the console, logs, and other interfaces. This ",
+ " might reduce the security of your system. ",
+ " If you see this message and you are not debugging ",
+ " the kernel, report this immediately to your system ",
+ " administrator! ",
+};
+
static int __init no_hash_pointers_enable(char *str)
{
+ const int lines[] = { 0, 1, 2, 3, 4, 5, 2, 6, 7, 8, 2, 1, 0 };
+ int i;
+
no_hash_pointers = true;
- pr_warn("**********************************************************\n");
- pr_warn("** NOTICE NOTICE NOTICE NOTICE NOTICE NOTICE NOTICE **\n");
- pr_warn("** **\n");
- pr_warn("** This system shows unhashed kernel memory addresses **\n");
- pr_warn("** via the console, logs, and other interfaces. This **\n");
- pr_warn("** might 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");
+ for (i = 0; i < ARRAY_SIZE(lines); i++)
+ pr_warn("**%s**\n", no_hash_pointers_warning[lines[i]]);
return 0;
}
--
2.30.1.766.gb4fecdf3b7-goog
On Tue, Mar 02, 2021 at 03:28:09PM +0100, Geert Uytterhoeven wrote:
> On Tue, Mar 2, 2021 at 3:08 PM Steven Rostedt <[email protected]> wrote:
> > On Tue, 2 Mar 2021 14:49:42 +0100
> > Geert Uytterhoeven <[email protected]> wrote:
> > > > So this is basically a kernel tinyfication issue, right? Is that still pursued
> > > > today? Are there better config options suitable for this than CONFIG_DEBUG_KERNEL?
> > >
> > > As long as I hear about products running Linux on SoCs with 10 MiB of
> > > SRAM, I think the answer is yes.
> > > I'm not immediately aware of a better config option. There are no more
> > > TINY options left, and EXPERT selects DEBUG_KERNEL.
> >
> > Since the trace_printk() uses the same type of notice, I wonder if we could
> > make this into a helper function and just pass in the top part.
> >
> > + pr_warn("**********************************************************\n");
> > + pr_warn("** NOTICE NOTICE NOTICE NOTICE NOTICE NOTICE NOTICE **\n");
> > + pr_warn("** **\n");
> >
> >
> > + pr_warn("** This system shows unhashed kernel memory addresses **\n");
> > + pr_warn("** via the console, logs, and other interfaces. This **\n");
> > + pr_warn("** might reduce the security of your system. **\n");
> >
> > Only the above section is really unique. The rest can be a boiler plate.
>
> Good idea. drivers/iommu/iommu-debugfs.c has a third copy.
+1. Let's keep it in some helper that can be added if we have a corresponding
functionality.
> > + 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");
>
> Fortunately gcc is already smart enough to deduplicate identical strings,
> but only in the same source file.
--
With Best Regards,
Andy Shevchenko
On Tue, 2 Mar 2021 at 15:55, Geert Uytterhoeven <[email protected]> wrote:
>
> Hi Marco,
>
> On Tue, Mar 2, 2021 at 3:40 PM Marco Elver <[email protected]> wrote:
> > On Tue, 2 Mar 2021 at 15:35, Matthew Wilcox <[email protected]> wrote:
> > > On Tue, Mar 02, 2021 at 03:26:50PM +0100, Marco Elver wrote:
> > > > +static const char no_hash_pointers_warning[9][55] __initconst = {
> > > > + "******************************************************",
> > > > + " NOTICE NOTICE NOTICE NOTICE NOTICE NOTICE NOTICE ",
> > > > + " ",
> > > > + " This system shows unhashed kernel memory addresses ",
> > > > + " via the console, logs, and other interfaces. This ",
> > > > + " might reduce the security of your system. ",
> > > > + " If you see this message and you are not debugging ",
> > > > + " the kernel, report this immediately to your system ",
> > > > + " administrator! ",
> > > > +};
> > > > +
> > > > static int __init no_hash_pointers_enable(char *str)
> > > > {
> > > > + const int lines[] = { 0, 1, 2, 3, 4, 5, 2, 6, 7, 8, 2, 1, 0 };
> > > > + int i;
> > > > +
> > > > no_hash_pointers = true;
> > > >
> > > > - pr_warn("**********************************************************\n");
> > > > - pr_warn("** NOTICE NOTICE NOTICE NOTICE NOTICE NOTICE NOTICE **\n");
> > > > - pr_warn("** **\n");
> > > > - pr_warn("** This system shows unhashed kernel memory addresses **\n");
> > > > - pr_warn("** via the console, logs, and other interfaces. This **\n");
> > > > - pr_warn("** might 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");
> > > > + for (i = 0; i < ARRAY_SIZE(lines); i++)
> > > > + pr_warn("**%s**\n", no_hash_pointers_warning[lines[i]]);
> > >
> > > + for (i = 0; i < 3; i++)
> > > + pr_warn("**%s**\n", no_hash_pointers_warning[lines[2 - i]]);
> >
> > Yeah, I had that before, but then wanted to deal with the blank line
> > in the middle of the thing. So I just went with the lines array above,
> > which seemed cleanest for dealing with the middle blank line and
> > footer. Or maybe there's something even nicer I missed? :-)
>
> Gcc deduplicates the identical strings, so you don't have to go through
> a double indirection at all?
In this case I think we do, because we're asking the compiler to
create a giant array char[9][55]. If we had char*[9], then you're
right, but in that case we would not benefit from __initconst for the
majority of the data.
Thanks,
-- Marco
On 02/03/2021 15.28, Geert Uytterhoeven wrote:
> Fortunately gcc is already smart enough to deduplicate identical strings,
> but only in the same source file.
Yeah, gcc can't do much more since it only handles one source file at a
time. However, the linker certainly deduplicates strings across
translation units :)
I don't think gcc bothers to do the tail merging since the linker does
it, but that may depend on optimization level and gcc version; it does
seem to merge identical strings within a TU, at least in very simple cases.
Rasmus
$ grep . *.c
a.c:const char *a1(void) { return "string"; }
a.c:const char *a2(void) { return "string"; }
a.c:const char *a3(void) { return "longer string"; }
b.c:const char *b1(void) { return "string"; }
b.c:const char *b2(void) { return "longer string"; }
b.c:const char *b3(void) { return "much longer string"; }
main.c:#include <stdio.h>
main.c:const char *a1(void);
main.c:const char *a2(void);
main.c:const char *a3(void);
main.c:const char *b1(void);
main.c:const char *b2(void);
main.c:const char *b3(void);
main.c:int main(int argc, char *argv[])
main.c:{
main.c:#define p(x) printf("%s(): %p - [%s]\n", #x, x(), x())
main.c: p(a1);
main.c: p(a2);
main.c: p(a3);
main.c: p(b1);
main.c: p(b2);
main.c: p(b3);
main.c:
main.c: return 0;
main.c:}
$ gcc -O2 -o a.o -c a.c ; gcc -O2 -o b.o -c b.c ; gcc -O2 -o main.o -c
main.c ; gcc -o main main.o a.o b.o ; ./main
a1(): 0x560b1bc3d033 - [string]
a2(): 0x560b1bc3d033 - [string]
a3(): 0x560b1bc3d02c - [longer string]
b1(): 0x560b1bc3d033 - [string]
b2(): 0x560b1bc3d02c - [longer string]
b3(): 0x560b1bc3d027 - [much longer string]
On Tue, 2 Mar 2021 at 15:35, Matthew Wilcox <[email protected]> wrote:
>
> On Tue, Mar 02, 2021 at 03:26:50PM +0100, Marco Elver wrote:
> > +static const char no_hash_pointers_warning[9][55] __initconst = {
> > + "******************************************************",
> > + " NOTICE NOTICE NOTICE NOTICE NOTICE NOTICE NOTICE ",
> > + " ",
> > + " This system shows unhashed kernel memory addresses ",
> > + " via the console, logs, and other interfaces. This ",
> > + " might reduce the security of your system. ",
> > + " If you see this message and you are not debugging ",
> > + " the kernel, report this immediately to your system ",
> > + " administrator! ",
> > +};
> > +
> > static int __init no_hash_pointers_enable(char *str)
> > {
> > + const int lines[] = { 0, 1, 2, 3, 4, 5, 2, 6, 7, 8, 2, 1, 0 };
> > + int i;
> > +
> > no_hash_pointers = true;
> >
> > - pr_warn("**********************************************************\n");
> > - pr_warn("** NOTICE NOTICE NOTICE NOTICE NOTICE NOTICE NOTICE **\n");
> > - pr_warn("** **\n");
> > - pr_warn("** This system shows unhashed kernel memory addresses **\n");
> > - pr_warn("** via the console, logs, and other interfaces. This **\n");
> > - pr_warn("** might 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");
> > + for (i = 0; i < ARRAY_SIZE(lines); i++)
> > + pr_warn("**%s**\n", no_hash_pointers_warning[lines[i]]);
>
> + for (i = 0; i < 3; i++)
> + pr_warn("**%s**\n", no_hash_pointers_warning[lines[2 - i]]);
Yeah, I had that before, but then wanted to deal with the blank line
in the middle of the thing. So I just went with the lines array above,
which seemed cleanest for dealing with the middle blank line and
footer. Or maybe there's something even nicer I missed? :-)
Thanks,
-- Marco
Hi Steven,
On Tue, Mar 2, 2021 at 3:08 PM Steven Rostedt <[email protected]> wrote:
> On Tue, 2 Mar 2021 14:49:42 +0100
> Geert Uytterhoeven <[email protected]> wrote:
> > > So this is basically a kernel tinyfication issue, right? Is that still pursued
> > > today? Are there better config options suitable for this than CONFIG_DEBUG_KERNEL?
> >
> > As long as I hear about products running Linux on SoCs with 10 MiB of
> > SRAM, I think the answer is yes.
> > I'm not immediately aware of a better config option. There are no more
> > TINY options left, and EXPERT selects DEBUG_KERNEL.
>
> Since the trace_printk() uses the same type of notice, I wonder if we could
> make this into a helper function and just pass in the top part.
>
> + pr_warn("**********************************************************\n");
> + pr_warn("** NOTICE NOTICE NOTICE NOTICE NOTICE NOTICE NOTICE **\n");
> + pr_warn("** **\n");
>
>
> + pr_warn("** This system shows unhashed kernel memory addresses **\n");
> + pr_warn("** via the console, logs, and other interfaces. This **\n");
> + pr_warn("** might reduce the security of your system. **\n");
>
> Only the above section is really unique. The rest can be a boiler plate.
Good idea. drivers/iommu/iommu-debugfs.c has a third copy.
> + 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");
Fortunately gcc is already smart enough to deduplicate identical strings,
but only in the same source file.
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
On Tue 2021-03-02 14:49:42, Geert Uytterhoeven wrote:
> Hi Vlastimil, Petr,
>
> On Tue, Mar 2, 2021 at 2:37 PM Vlastimil Babka <[email protected]> wrote:
> > On 3/2/21 2:29 PM, Petr Mladek wrote:
> > > On Tue 2021-03-02 13:51:35, Geert Uytterhoeven wrote:
> > >> > > > +
> > >> > > > + pr_warn("**********************************************************\n");
> > >> > > > + pr_warn("** NOTICE NOTICE NOTICE NOTICE NOTICE NOTICE NOTICE **\n");
> > >> > > > + pr_warn("** **\n");
> > >> > > > + pr_warn("** This system shows unhashed kernel memory addresses **\n");
> > >> > > > + pr_warn("** via the console, logs, and other interfaces. This **\n");
> > >> > > > + pr_warn("** might 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("no_hash_pointers", no_hash_pointers_enable);
> > >> > >
> > >> > > While bloat-o-meter is not smart enough to notice the real size impact,
> > >> > > this does add more than 500 bytes of string data to the kernel.
> > >> > > Do we really need such a large message?
> > >> > > Perhaps the whole no_hash_pointers machinery should be protected by
> > >> > > "#ifdef CONFIG_DEBUG_KERNEL"?
> >
> > I think it's a no-go only when enabling such option equals to "no_hash_pointers"
> > being always passed. What Geert suggests is that you need both
> > CONFIG_DEBUG_KERNEL *and* no_hash_pointers and that's different.
>
> Exactly.
>
> > So this is basically a kernel tinyfication issue, right? Is that still pursued
> > today? Are there better config options suitable for this than CONFIG_DEBUG_KERNEL?
>
> As long as I hear about products running Linux on SoCs with 10 MiB of
> SRAM, I think the answer is yes.
> I'm not immediately aware of a better config option. There are no more
> TINY options left, and EXPERT selects DEBUG_KERNEL.
DEBUG_KERNEL might actually makes sense. A possibility to see real
pointers might be pretty useful for the other debugging code.
It is a common thing.
DEBUG_KERNEL is even needed for many basics debugging helpers,
for example, for FRAME_POINTERS.
So, if it would be good for SoCs...
> > >> > Would placing the strings into an __initconst array help?
> > >>
> > >> That would indeed help to reduce run-time memory consumption.
> > >
> > > Sure. We could do this. Do you want to send a patch, please?
>
> Added to my list.
>
> > >> It would not solve the raw kernel size increase.
> > >
> > > I see. Well, the compression should be pretty efficient
> > > for a text (with many spaces).
>
> My worry is not about the medium for storing the kernel image, but the
> RAM where the kernel image is loaded. The former is usually less
> restricted in size, and easier to expand, than the latter,
Well, the __initconst might be enough then.
I personally do not have any preference whether to do __initconst
or DEBUG_KERNEL or both. We should just keep it simple and
do not over engineer it.
Best Regards,
Petr
Hi Marco,
On Tue, Mar 2, 2021 at 3:40 PM Marco Elver <[email protected]> wrote:
> On Tue, 2 Mar 2021 at 15:35, Matthew Wilcox <[email protected]> wrote:
> > On Tue, Mar 02, 2021 at 03:26:50PM +0100, Marco Elver wrote:
> > > +static const char no_hash_pointers_warning[9][55] __initconst = {
> > > + "******************************************************",
> > > + " NOTICE NOTICE NOTICE NOTICE NOTICE NOTICE NOTICE ",
> > > + " ",
> > > + " This system shows unhashed kernel memory addresses ",
> > > + " via the console, logs, and other interfaces. This ",
> > > + " might reduce the security of your system. ",
> > > + " If you see this message and you are not debugging ",
> > > + " the kernel, report this immediately to your system ",
> > > + " administrator! ",
> > > +};
> > > +
> > > static int __init no_hash_pointers_enable(char *str)
> > > {
> > > + const int lines[] = { 0, 1, 2, 3, 4, 5, 2, 6, 7, 8, 2, 1, 0 };
> > > + int i;
> > > +
> > > no_hash_pointers = true;
> > >
> > > - pr_warn("**********************************************************\n");
> > > - pr_warn("** NOTICE NOTICE NOTICE NOTICE NOTICE NOTICE NOTICE **\n");
> > > - pr_warn("** **\n");
> > > - pr_warn("** This system shows unhashed kernel memory addresses **\n");
> > > - pr_warn("** via the console, logs, and other interfaces. This **\n");
> > > - pr_warn("** might 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");
> > > + for (i = 0; i < ARRAY_SIZE(lines); i++)
> > > + pr_warn("**%s**\n", no_hash_pointers_warning[lines[i]]);
> >
> > + for (i = 0; i < 3; i++)
> > + pr_warn("**%s**\n", no_hash_pointers_warning[lines[2 - i]]);
>
> Yeah, I had that before, but then wanted to deal with the blank line
> in the middle of the thing. So I just went with the lines array above,
> which seemed cleanest for dealing with the middle blank line and
> footer. Or maybe there's something even nicer I missed? :-)
Gcc deduplicates the identical strings, so you don't have to go through
a double indirection at all?
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
On 2021/2/15 0:13, Timur Tabi wrote:
> If the no_hash_pointers command line parameter is set, then
> printk("%p") will print pointers as unhashed, which is useful for
> debugging purposes. This change 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]>
> Acked-by: Marco Elver <[email protected]>
> ---
> .../admin-guide/kernel-parameters.txt | 15 ++++++++
> lib/test_printf.c | 8 +++++
> lib/vsprintf.c | 36 +++++++++++++++++--
> 3 files changed, 57 insertions(+), 2 deletions(-)
>
> diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
> index a10b545c2070..c8993a296e71 100644
> --- a/Documentation/admin-guide/kernel-parameters.txt
> +++ b/Documentation/admin-guide/kernel-parameters.txt
> @@ -3281,6 +3281,21 @@
> in certain environments such as networked servers or
> real-time systems.
>
> + no_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.
> +
> nohibernate [HIBERNATION] Disable hibernation and resume.
>
> nohz= [KNL] Boottime enable/disable dynamic ticks
> diff --git a/lib/test_printf.c b/lib/test_printf.c
> index ad2bcfa8caa1..a6755798e9e6 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 no_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 (no_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..41ddc353ebb8 100644
> --- a/lib/vsprintf.c
> +++ b/lib/vsprintf.c
> @@ -2090,6 +2090,32 @@ 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 no_hash_pointers __ro_after_init;
> +EXPORT_SYMBOL_GPL(no_hash_pointers);
Why do we need to export the no_hash_pointers variable and not declare
it in any header file?
Thanks.
Xiaoming Ni
> +
> +static int __init no_hash_pointers_enable(char *str)
> +{
> + no_hash_pointers = true;
> +
> + pr_warn("**********************************************************\n");
> + pr_warn("** NOTICE NOTICE NOTICE NOTICE NOTICE NOTICE NOTICE **\n");
> + pr_warn("** **\n");
> + pr_warn("** This system shows unhashed kernel memory addresses **\n");
> + pr_warn("** via the console, logs, and other interfaces. This **\n");
> + pr_warn("** might 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("no_hash_pointers", no_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 +2323,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 no_hash_pointers is specified on the command line.
> + */
> + if (unlikely(no_hash_pointers))
> + return pointer_string(buf, end, ptr, spec);
> + else
> + return ptr_to_id(buf, end, ptr, spec);
> }
>
> /*
>
On 2021/09/11 11:25, Xiaoming Ni wrote:
> Why do we need to export the no_hash_pointers variable and
> not declare it in any header file?
Because lib/test_printf.c wants to use no_hash_pointers for testing
purpose, and lib/test_printf.c might be built as a loadable kernel module.
That is, no_hash_pointers is not meant for general use.
config TEST_PRINTF
tristate "Test printf() family of functions at runtime"
obj-$(CONFIG_TEST_PRINTF) += test_printf.o