From: Timur Tabi <timur@kernel.org> Date: 2021-01-16 22:10:59
First patch updates print_hex_dump() and related functions to
allow callers to print hex dumps with unhashed addresses. It
adds a new prefix type, so existing code is unchanged.
Second patch changes a page poising function to use the new
address type. This is just an example of a change. If it's
wrong, it doesn't need to be applied.
IMHO, hashed addresses make very little sense for hex dumps,
which print addresses in 16- or 32-byte increments. Typical
use-case is to correlate an addresses in between one of these
increments with some other address, but that can't be done
if the addresses are hashed. I expect most developers to
want to replace their usage of DUMP_PREFIX_ADDRESS with
DUMP_PREFIX_UNHASHED, now that they have the opportunity.
Timur Tabi (2):
[v2] lib/hexdump: introduce DUMP_PREFIX_UNHASHED for unhashed
addresses
mm/page_poison: use unhashed address in hexdump for check_poison_mem()
fs/seq_file.c | 3 +++
include/linux/printk.h | 8 +++++---
lib/hexdump.c | 9 +++++++--
lib/seq_buf.c | 9 +++++++--
mm/page_poison.c | 2 +-
5 files changed, 23 insertions(+), 8 deletions(-)
--
2.25.1
From: Timur Tabi <timur@kernel.org> Date: 2021-01-16 22:10:59
Hashed addresses are useless in hexdumps unless you're comparing
with other hashed addresses, which is unlikely. However, there's
no need to break existing code, so introduce a new prefix type
that prints unhashed addresses.
Signed-off-by: Timur Tabi <timur@kernel.org>
Reviewed-by: Petr Mladek <pmladek@suse.com>
Cc: Roman Fietze <redacted>
---
fs/seq_file.c | 3 +++
include/linux/printk.h | 8 +++++---
lib/hexdump.c | 9 +++++++--
lib/seq_buf.c | 9 +++++++--
4 files changed, 22 insertions(+), 7 deletions(-)
From: Andy Shevchenko <hidden> Date: 2021-01-18 21:36:53
On Sun, Jan 17, 2021 at 12:12 AM Timur Tabi [off-list ref] wrote:
(Hint: -v<n> to the git format-patch will create a versioned subject
prefix for you automatically)
Hashed addresses are useless in hexdumps unless you're comparing
with other hashed addresses, which is unlikely. However, there's
no need to break existing code, so introduce a new prefix type
that prints unhashed addresses.
Any user of this? (For the record, I don't see any other mail except this one)
...
Since it's an address, I would like to group them together, i.e. put
after DUMP_PREFIX_ADDRESS.
Perhaps even add _ADDRESS to DUMP_PREFIX_UNHASHED, but this maybe too long.
};
...
+ * @prefix_type: controls whether prefix of an offset, hashed address,
+ * unhashed address, or none is printed (%DUMP_PREFIX_OFFSET,
+ * %DUMP_PREFIX_ADDRESS, %DUMP_PREFIX_UNHASHED, %DUMP_PREFIX_NONE)
Yeah, exactly, here you use different ordering.
...
+ * @prefix_type: controls whether prefix of an offset, hashed address,
+ * unhashed address, or none is printed (%DUMP_PREFIX_OFFSET,
+ * %DUMP_PREFIX_ADDRESS, %DUMP_PREFIX_UNHASHED, %DUMP_PREFIX_NONE)
In both cases I would rather use colon and list one per line. What do you think?
...
+ case DUMP_PREFIX_UNHASHED:
Here is a third type of ordering, can you please be consistent?
case DUMP_PREFIX_ADDRESS:
...
+ * @prefix_type: controls whether prefix of an offset, hashed address,
+ * unhashed address, or none is printed (%DUMP_PREFIX_OFFSET,
+ * %DUMP_PREFIX_ADDRESS, %DUMP_PREFIX_UNHASHED, %DUMP_PREFIX_NONE)
From: Timur Tabi <timur@kernel.org> Date: 2021-01-18 16:18:07
On 1/18/21 4:03 AM, Andy Shevchenko wrote:
On Sun, Jan 17, 2021 at 12:12 AM Timur Tabi [off-list ref] wrote:
(Hint: -v<n> to the git format-patch will create a versioned subject
prefix for you automatically)
I like to keep the version in the git repo itself so that I don't need
to keep track of it separately, but thanks for the hint. I might use it
somewhere else.
quoted
Hashed addresses are useless in hexdumps unless you're comparing
with other hashed addresses, which is unlikely. However, there's
no need to break existing code, so introduce a new prefix type
that prints unhashed addresses.
Any user of this? (For the record, I don't see any other mail except this one)
Since it's an address, I would like to group them together, i.e. put
after DUMP_PREFIX_ADDRESS.
I didn't want to change the numbering of any existing enums, just in
case there are users that accidentally hard-code the values. I'm trying
to make this patch as unobtrusive as possible.
> Perhaps even add _ADDRESS to DUMP_PREFIX_UNHASHED, but this maybe too
long.
I think DUMP_PREFIX_ADDRESS_UNHASHED is too long.
quoted
+ * @prefix_type: controls whether prefix of an offset, hashed address,
+ * unhashed address, or none is printed (%DUMP_PREFIX_OFFSET,
+ * %DUMP_PREFIX_ADDRESS, %DUMP_PREFIX_UNHASHED, %DUMP_PREFIX_NONE)
Yeah, exactly, here you use different ordering.
That's because it's a comment.
quoted
+ * @prefix_type: controls whether prefix of an offset, hashed address,
+ * unhashed address, or none is printed (%DUMP_PREFIX_OFFSET,
+ * %DUMP_PREFIX_ADDRESS, %DUMP_PREFIX_UNHASHED, %DUMP_PREFIX_NONE)
In both cases I would rather use colon and list one per line. What do you think?
Hmmmm.... if I'm going to change the patch anyway, sure.
quoted
+ case DUMP_PREFIX_UNHASHED:
Here is a third type of ordering, can you please be consistent?
Since it's an address, I would like to group them together, i.e. put
after DUMP_PREFIX_ADDRESS.
I didn't want to change the numbering of any existing enums, just in case
there are users that accidentally hard-code the values. I'm trying to make
this patch as unobtrusive as possible.
But isn't it good to expose those issues (and fix them)?
...
quoted
Perhaps even add _ADDRESS to DUMP_PREFIX_UNHASHED, but this maybe too
long.
I think DUMP_PREFIX_ADDRESS_UNHASHED is too long.
What about introducing new two like these:
DUMP_PREFIX_OFFSET,
DUMP_PREFIX_ADDRESS,
DUMP_PREFIX_ADDR_UNHASHED,
DUMP_PREFIX_ADDR_HASHED,
and allow people step-by-step move to them?
--
With Best Regards,
Andy Shevchenko
From: Timur Tabi <timur@kernel.org> Date: 2021-01-18 17:54:32
On 1/18/21 11:14 AM, Andy Shevchenko wrote:
But isn't it good to expose those issues (and fix them)?
I suppose.
quoted
quoted
Perhaps even add _ADDRESS to DUMP_PREFIX_UNHASHED, but this maybe too
long.
I think DUMP_PREFIX_ADDRESS_UNHASHED is too long.
What about introducing new two like these:
DUMP_PREFIX_OFFSET,
DUMP_PREFIX_ADDRESS,
DUMP_PREFIX_ADDR_UNHASHED,
DUMP_PREFIX_ADDR_HASHED,
I think we're approaching bike-shedding. DUMP_PREFIX_ADDR_HASHED and
DUMP_PREFIX_ADDRESS are the same thing.
I don't want people to have to move from DUMP_PREFIX_ADDRESS to some
other enum for no change in functionality. I'm willing to rearrange the
code so that it's enumerated more consistently, but I don't think
there's anything wrong with DUMP_PREFIX_UNHASHED. It's succinct and clear.
From: Timur Tabi <timur@kernel.org> Date: 2021-01-16 22:12:33
Now that print_hex_dump() is capable of printing unhashed addresses,
use that feature in the code that reports memory errors. Hashed
addresses don't tell you where the corrupted page actually is.
Signed-off-by: Timur Tabi <timur@kernel.org>
---
mm/page_poison.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
From: Matthew Wilcox <willy@infradead.org> Date: 2021-01-18 18:46:20
On Sat, Jan 16, 2021 at 04:09:48PM -0600, Timur Tabi wrote:
First patch updates print_hex_dump() and related functions to
allow callers to print hex dumps with unhashed addresses. It
adds a new prefix type, so existing code is unchanged.
Second patch changes a page poising function to use the new
address type. This is just an example of a change. If it's
wrong, it doesn't need to be applied.
IMHO, hashed addresses make very little sense for hex dumps,
which print addresses in 16- or 32-byte increments. Typical
use-case is to correlate an addresses in between one of these
increments with some other address, but that can't be done
if the addresses are hashed. I expect most developers to
want to replace their usage of DUMP_PREFIX_ADDRESS with
DUMP_PREFIX_UNHASHED, now that they have the opportunity.
Yes, I'm sure most kernel developers actually do want to leak kernel
addresses into kernel messages. The important thing though is that it
should be hard for them to do that, and it should stick out like a sore
thumb if they do it.
Don't make it easy. And don't make it look like they're doing
something innocent. DUMP_PREFIX_SECURITY_HOLE would be OK
by me. DUMP_PREFIX_LEAK_INFORMATION would work fine too.
DUMP_PREFIX_MAKE_ATTACKERS_LIFE_EASY might be a bit too far.
From: Timur Tabi <timur@kernel.org> Date: 2021-01-18 19:06:26
On 1/18/21 12:26 PM, Matthew Wilcox wrote:
Don't make it easy. And don't make it look like they're doing
something innocent. DUMP_PREFIX_SECURITY_HOLE would be OK
by me. DUMP_PREFIX_LEAK_INFORMATION would work fine too.
DUMP_PREFIX_MAKE_ATTACKERS_LIFE_EASY might be a bit too far.
It's already extremely easy to replace %p with %px in your own printks,
so I don't really understand your argument.
Seriously, this patch should not be so contentious. If you want hashed
addresses, then nothing changes. If you need unhashed addresses while
debugging, then use DUMP_PREFIX_UNHASHED. Just like you can use %px in
printk. I never use %p in my printks, but then I never submit code
upstream that prints addresses, hashed or unhashed.
Don't make it easy. And don't make it look like they're doing
something innocent. DUMP_PREFIX_SECURITY_HOLE would be OK
by me. DUMP_PREFIX_LEAK_INFORMATION would work fine too.
DUMP_PREFIX_MAKE_ATTACKERS_LIFE_EASY might be a bit too far.
It's already extremely easy to replace %p with %px in your own printks, so I
don't really understand your argument.
I like the idea of a more radical name, e.g. DUMP_PREFIX_RAW_POINTERS or
something similar.
Seriously, this patch should not be so contentious. If you want hashed
addresses, then nothing changes. If you need unhashed addresses while
debugging, then use DUMP_PREFIX_UNHASHED. Just like you can use %px in
printk. I never use %p in my printks, but then I never submit code upstream
that prints addresses, hashed or unhashed.
So maybe DUMP_PREFIX_UNHASHED can do the unhashed dump only when
CONFIG_DEBUG_KERNEL=y and fallback to DUMP_PREFIX_ADDRESS otherwise?
-ss
From: Matthew Wilcox <willy@infradead.org> Date: 2021-01-19 01:49:08
On Tue, Jan 19, 2021 at 09:53:01AM +0900, Sergey Senozhatsky wrote:
On (21/01/18 13:03), Timur Tabi wrote:
quoted
On 1/18/21 12:26 PM, Matthew Wilcox wrote:
quoted
Don't make it easy. And don't make it look like they're doing
something innocent. DUMP_PREFIX_SECURITY_HOLE would be OK
by me. DUMP_PREFIX_LEAK_INFORMATION would work fine too.
DUMP_PREFIX_MAKE_ATTACKERS_LIFE_EASY might be a bit too far.
It's already extremely easy to replace %p with %px in your own printks, so I
don't really understand your argument.
I like the idea of a more radical name, e.g. DUMP_PREFIX_RAW_POINTERS or
something similar.
quoted
Seriously, this patch should not be so contentious. If you want hashed
addresses, then nothing changes. If you need unhashed addresses while
debugging, then use DUMP_PREFIX_UNHASHED. Just like you can use %px in
printk. I never use %p in my printks, but then I never submit code upstream
that prints addresses, hashed or unhashed.
I'm glad to hear you never make mistakes. I make lots of mistakes, so
I prefer them to be big, loud and obvious so they're easy for people
to spot.
So maybe DUMP_PREFIX_UNHASHED can do the unhashed dump only when
CONFIG_DEBUG_KERNEL=y and fallback to DUMP_PREFIX_ADDRESS otherwise?
Distros enable CONFIG_DEBUG_KERNEL. If you want to add
CONFIG_DEBUG_LEAK_ADDRESSES, then that's great, and you won't even have
to change users, you can just change how %p behaves.
So maybe DUMP_PREFIX_UNHASHED can do the unhashed dump only when
CONFIG_DEBUG_KERNEL=y and fallback to DUMP_PREFIX_ADDRESS otherwise?
Distros enable CONFIG_DEBUG_KERNEL.
Oh, I see.
If you want to add CONFIG_DEBUG_LEAK_ADDRESSES, then that's great,
and you won't even have to change users, you can just change how %p
behaves.
I like the name. config dependent behaviour of %p wouldn't be new,
well, to some extent, e.g. XFS does something similar (see below).
I don't think Linus will be sold on this, however.
fs/xfs/xfs_linux.h:
/*
* Starting in Linux 4.15, the %p (raw pointer value) printk modifier
* prints a hashed version of the pointer to avoid leaking kernel
* pointers into dmesg. If we're trying to debug the kernel we want the
* raw values, so override this behavior as best we can.
*/
#ifdef DEBUG
# define PTR_FMT "%px"
#else
# define PTR_FMT "%p"
#endif
And then they just use it as
xfs_alert(mp, "%s: bad inode magic number, dip = "ptr_fmt",
dino bp = "ptr_fmt", ino = %ld",
__func__, dip, bp, in_f->ilf_ino);
-ss
On Tue, Jan 19, 2021 at 07:38:24PM +0900, Sergey Senozhatsky wrote:
On (21/01/19 01:47), Matthew Wilcox wrote:
[..]
quoted
quoted
So maybe DUMP_PREFIX_UNHASHED can do the unhashed dump only when
CONFIG_DEBUG_KERNEL=y and fallback to DUMP_PREFIX_ADDRESS otherwise?
Distros enable CONFIG_DEBUG_KERNEL.
Oh, I see.
quoted
If you want to add CONFIG_DEBUG_LEAK_ADDRESSES, then that's great,
and you won't even have to change users, you can just change how %p
behaves.
I like the name. config dependent behaviour of %p wouldn't be new,
well, to some extent, e.g. XFS does something similar (see below).
I don't think Linus will be sold on this, however.
fs/xfs/xfs_linux.h:
/*
* Starting in Linux 4.15, the %p (raw pointer value) printk modifier
* prints a hashed version of the pointer to avoid leaking kernel
* pointers into dmesg. If we're trying to debug the kernel we want the
* raw values, so override this behavior as best we can.
*/
#ifdef DEBUG
# define PTR_FMT "%px"
#else
# define PTR_FMT "%p"
#endif
And then they just use it as
xfs_alert(mp, "%s: bad inode magic number, dip = "ptr_fmt",
dino bp = "ptr_fmt", ino = %ld",
__func__, dip, bp, in_f->ilf_ino);
-ss
Please no, this is effectively a toggle.
--
Kees Cook
So maybe DUMP_PREFIX_UNHASHED can do the unhashed dump only when
CONFIG_DEBUG_KERNEL=y and fallback to DUMP_PREFIX_ADDRESS otherwise?
Distros enable CONFIG_DEBUG_KERNEL.
Oh, I see.
quoted
If you want to add CONFIG_DEBUG_LEAK_ADDRESSES, then that's great,
and you won't even have to change users, you can just change how %p
behaves.
I like the name. config dependent behaviour of %p wouldn't be new,
well, to some extent, e.g. XFS does something similar (see below).
I don't think Linus will be sold on this, however.
Given Linus' current stance later in this thread, could we revive the idea of a
boot time option, or at least a CONFIG (I assume a runtime toggle would be too
much, even if limited to !kernel_lockdown :) , that would disable all hashing?
It would be really useful for a development/active debugging, as evidenced
below. Thanks.
fs/xfs/xfs_linux.h:
/*
* Starting in Linux 4.15, the %p (raw pointer value) printk modifier
* prints a hashed version of the pointer to avoid leaking kernel
* pointers into dmesg. If we're trying to debug the kernel we want the
* raw values, so override this behavior as best we can.
*/
#ifdef DEBUG
# define PTR_FMT "%px"
#else
# define PTR_FMT "%p"
#endif
And then they just use it as
xfs_alert(mp, "%s: bad inode magic number, dip = "ptr_fmt",
dino bp = "ptr_fmt", ino = %ld",
__func__, dip, bp, in_f->ilf_ino);
-ss
From: Timur Tabi <timur@kernel.org> Date: 2021-01-27 07:40:03
On 1/26/21 10:47 AM, Vlastimil Babka wrote:
Given Linus' current stance later in this thread, could we revive the idea of a
boot time option, or at least a CONFIG (I assume a runtime toggle would be too
much, even if limited to !kernel_lockdown:) , that would disable all hashing?
It would be really useful for a development/active debugging, as evidenced
below. Thanks.
So you're saying:
if CONFIG_PRINTK_NEVER_HASH is disabled, then %p prints hashed addresses
and %px prints unhashed.
If CONFIG_PRINTK_NEVER_HASH is enabled, then %p and %px both print
unhashed addresses.
I like this idea, and I would accept it as a solution if I had to, but I
still would also like for an option for print_hex_dump() to print
unhashed addresses even when CONFIG_PRINTK_NEVER_HASH is disabled. I
can't always recompile the entire kernel for my testing purposes.
The only drawback to this idea is: what happens if distros start
enabling CONFIG_PRINTK_NEVER_HASH by default, just because it makes
debugging easier?
From: Steven Rostedt <rostedt@goodmis.org> Date: 2021-01-27 00:28:10
On Tue, 26 Jan 2021 10:59:12 -0600
Timur Tabi [off-list ref] wrote:
The only drawback to this idea is: what happens if distros start
enabling CONFIG_PRINTK_NEVER_HASH by default, just because it makes
debugging easier?
I do believe distros should be more concerned about security than using
this for making debugging easier.
Perhaps we should add the same banner print if that config is set as
trace_printk() has if it is detected in the kernel or a module:
**********************************************************
** NOTICE NOTICE NOTICE NOTICE NOTICE NOTICE NOTICE **
** **
** trace_printk() being used. Allocating extra memory. **
** **
** This means that this is a DEBUG kernel and it is **
** unsafe for production use. **
** **
** If you see this message and you are not debugging **
** the kernel, report this immediately to your vendor! **
** **
** NOTICE NOTICE NOTICE NOTICE NOTICE NOTICE NOTICE **
**********************************************************
But have:
**********************************************************
** NOTICE NOTICE NOTICE NOTICE NOTICE NOTICE NOTICE **
** **
** CONFIG_PRINTK_NEVER_HASH enabled **
** **
** This means that this is a DEBUG kernel and it is **
** unsafe for production use. **
** **
** If you see this message and you are not debugging **
** the kernel, report this immediately to your vendor! **
** **
** NOTICE NOTICE NOTICE NOTICE NOTICE NOTICE NOTICE **
**********************************************************
The above appears to keep people from using trace_printk(), I don't see why
it wouldn't work for this config ;-)
-- Steve
Given Linus' current stance later in this thread, could we revive the idea of a
boot time option, or at least a CONFIG (I assume a runtime toggle would be too
much, even if limited to !kernel_lockdown:) , that would disable all hashing?
It would be really useful for a development/active debugging, as evidenced
below. Thanks.
So you're saying:
if CONFIG_PRINTK_NEVER_HASH is disabled, then %p prints hashed addresses and %px
prints unhashed.
If CONFIG_PRINTK_NEVER_HASH is enabled, then %p and %px both print unhashed
addresses.
Minimally, yes. KASLR is configurable like this, so why not printing of kernel
pointers?
I like this idea, and I would accept it as a solution if I had to, but I still
would also like for an option for print_hex_dump() to print unhashed addresses
even when CONFIG_PRINTK_NEVER_HASH is disabled. I can't always recompile the
entire kernel for my testing purposes.
Yeah, obviously a boot option would be nicer. The discussion Kees pointed to [1]
seemed to be about papering over problems with entropy? Not about making
development/debugging easier. But I understand it was not the first time and I
didn't check the older ones.
The only drawback to this idea is: what happens if distros start enabling
CONFIG_PRINTK_NEVER_HASH by default, just because it makes debugging easier?
There's tons of other options already where the choice is between security and
performance, and distros make their choice (including, again, KASLR itself).
Pointer hashing would be just another one.
If it was a boot option, I would personally be for leaving hashing enabled by
default, with opt-in boot option to disable it. Then if I'm instructing a user
to boot the distro kernel (without recompile) with e.g. slub_debug or
debug_pagealloc or page_owner in order to debug some issue, I would additionally
instruct them to add the 'no_pointer_hashing' parameter.
[1]
https://lore.kernel.org/lkml/CA+55aFwieC1-nAs+NFq9RTwaR8ef9hWa4MjNBWL41F-8wM49eA@mail.gmail.com/
From: Steven Rostedt <rostedt@goodmis.org> Date: 2021-01-26 18:14:53
On Tue, 26 Jan 2021 12:39:12 -0500
Steven Rostedt [off-list ref] wrote:
On Tue, 26 Jan 2021 11:30:02 -0600
Timur Tabi [off-list ref] wrote:
quoted
On 1/26/21 11:14 AM, Vlastimil Babka wrote:
quoted
If it was a boot option, I would personally be for leaving hashing enabled by
default, with opt-in boot option to disable it.
A boot option would solve all my problems. I wouldn't need to recompile
the kernel, and it would apply to all variations of printk.
Should it be called "make-printk-insecure"
?
And even if we make this a boot time option, perhaps we should still
include that nasty dmesg notice, which will let people know that the kernel
has unhashed values.
-- Steve
From: John Ogness <john.ogness@linutronix.de> Date: 2021-01-27 06:18:28
On 2021-01-26, Steven Rostedt [off-list ref] wrote:
And even if we make this a boot time option, perhaps we should still
include that nasty dmesg notice, which will let people know that the
kernel has unhashed values.
+1
The notice would probably be the main motivation for distros/users to
avoid unhashed values unless truly debugging. Which is what we want.
John Ogness
On 2021-01-26, Steven Rostedt [off-list ref] wrote:
quoted
And even if we make this a boot time option, perhaps we should still
include that nasty dmesg notice, which will let people know that the
kernel has unhashed values.
+1
The notice would probably be the main motivation for distros/users to
avoid unhashed values unless truly debugging. Which is what we want.
+1 for boot param with a scary name and dmesg WARNING WARNING WARNING that
should look even scarier.
Timur, do you have time to take a look and submit a patch?
-ss
From: Timur Tabi <timur@kernel.org> Date: 2021-01-27 06:00:25
On 1/26/21 8:11 PM, Sergey Senozhatsky wrote:
+1 for boot param with a scary name and dmesg WARNING WARNING WARNING that
should look even scarier.
Timur, do you have time to take a look and submit a patch?
From: Petr Mladek <pmladek@suse.com> Date: 2021-01-27 10:14:20
On Tue 2021-01-26 12:40:32, Steven Rostedt wrote:
On Tue, 26 Jan 2021 12:39:12 -0500
Steven Rostedt [off-list ref] wrote:
quoted
On Tue, 26 Jan 2021 11:30:02 -0600
Timur Tabi [off-list ref] wrote:
quoted
On 1/26/21 11:14 AM, Vlastimil Babka wrote:
quoted
If it was a boot option, I would personally be for leaving hashing enabled by
default, with opt-in boot option to disable it.
A boot option would solve all my problems. I wouldn't need to recompile
the kernel, and it would apply to all variations of printk.
Should it be called "make-printk-insecure"
Nit: This makes me feel that printk() might break (block) the system.
Please, make it more clear that it is about unveiling some secret
information, something like:
"non-secret-printk"
"non-confidental-printk"
"unretricted-printk"
I do not mind about the words order or using the
"make-printk-non-secret" form.
And even if we make this a boot time option, perhaps we should still
include that nasty dmesg notice, which will let people know that the kernel
has unhashed values.
On Tue, 26 Jan 2021 12:39:12 -0500
Steven Rostedt [off-list ref] wrote:
quoted
On Tue, 26 Jan 2021 11:30:02 -0600
Timur Tabi [off-list ref] wrote:
quoted
On 1/26/21 11:14 AM, Vlastimil Babka wrote:
quoted
If it was a boot option, I would personally be for leaving hashing enabled by
default, with opt-in boot option to disable it.
A boot option would solve all my problems. I wouldn't need to recompile
the kernel, and it would apply to all variations of printk.
Should it be called "make-printk-insecure"
Nit: This makes me feel that printk() might break (block) the system.
Please, make it more clear that it is about unveiling some secret
information, something like:
"non-secret-printk"
"non-confidental-printk"
"unretricted-printk"
I do not mind about the words order or using the
"make-printk-non-secret" form.
Yeah, let's not be overly dramatic here.
quoted
And even if we make this a boot time option, perhaps we should still
include that nasty dmesg notice, which will let people know that the kernel
has unhashed values.
+1
If it's what it takes to have that option, fine :)
On Tue, Jan 19, 2021 at 01:47:25AM +0000, Matthew Wilcox wrote:
On Tue, Jan 19, 2021 at 09:53:01AM +0900, Sergey Senozhatsky wrote:
quoted
On (21/01/18 13:03), Timur Tabi wrote:
quoted
On 1/18/21 12:26 PM, Matthew Wilcox wrote:
quoted
Don't make it easy. And don't make it look like they're doing
something innocent. DUMP_PREFIX_SECURITY_HOLE would be OK
by me. DUMP_PREFIX_LEAK_INFORMATION would work fine too.
DUMP_PREFIX_MAKE_ATTACKERS_LIFE_EASY might be a bit too far.
It's already extremely easy to replace %p with %px in your own printks, so I
don't really understand your argument.
I like the idea of a more radical name, e.g. DUMP_PREFIX_RAW_POINTERS or
something similar.
quoted
Seriously, this patch should not be so contentious. If you want hashed
addresses, then nothing changes. If you need unhashed addresses while
debugging, then use DUMP_PREFIX_UNHASHED. Just like you can use %px in
printk. I never use %p in my printks, but then I never submit code upstream
that prints addresses, hashed or unhashed.
I'm glad to hear you never make mistakes. I make lots of mistakes, so
I prefer them to be big, loud and obvious so they're easy for people
to spot.
quoted
So maybe DUMP_PREFIX_UNHASHED can do the unhashed dump only when
CONFIG_DEBUG_KERNEL=y and fallback to DUMP_PREFIX_ADDRESS otherwise?
Distros enable CONFIG_DEBUG_KERNEL. If you want to add
CONFIG_DEBUG_LEAK_ADDRESSES, then that's great, and you won't even have
to change users, you can just change how %p behaves.
Following Linus's guidance[1] on this kind of thing, I think the correct
patch would be to actually _remove_ DUMP_PREFIX_ADDRESS entirely (or
make the offset math be hash-based). There isn't a strong enough reason
for it to exist in the first place:
- If the hashed “%p” value is pointless, ask yourself whether the pointer
itself is important. Maybe it should be removed entirely?
- If you really think the true pointer value is important, why is some
system state or user privilege level considered “special”? If you think
you can justify it (in comments and commit log) well enough to stand up
to Linus’s scrutiny, maybe you can use “%px”, along with making sure you
have sensible permissions.
- A toggle for “%p” hashing will not be accepted.
How about this so the base address is hashed once, with the offset added
to it for each line instead of each line having a "new" hash that makes
no sense:
From: Timur Tabi <timur@kernel.org> Date: 2021-01-19 19:57:32
On 1/19/21 1:45 PM, Kees Cook wrote:
quoted hunk
How about this so the base address is hashed once, with the offset added
to it for each line instead of each line having a "new" hash that makes
no sense:
Well, this is better than nothing, but not by much. Again, as long as
%px exists for printk(), I just cannot understand any resistance to
allowing it in print_hex_dump().
Frankly, I think this patch and my patch should both be added. During
debugging, it's very difficult if not impossible to work with hashed
addresses. I use print_hex_dump() with an unhashed address all the
time, either by applying my patch to my own kernel or just replacing the
%p with %px.
From: Steven Rostedt <rostedt@goodmis.org> Date: 2021-01-19 20:13:36
On Tue, 19 Jan 2021 13:55:29 -0600
Timur Tabi [off-list ref] wrote:
quoted
case DUMP_PREFIX_ADDRESS:
printk("%s%s%p: %s\n",
- level, prefix_str, ptr + i, linebuf);
+ level, prefix_str, addr + i, linebuf);
Well, this is better than nothing, but not by much. Again, as long as
%px exists for printk(), I just cannot understand any resistance to
allowing it in print_hex_dump().
Frankly, I think this patch and my patch should both be added. During
debugging, it's very difficult if not impossible to work with hashed
addresses. I use print_hex_dump() with an unhashed address all the
time, either by applying my patch to my own kernel or just replacing the
%p with %px.
I'm curious, what is the result if you replaced %p with %pS?
That way you get a kallsyms offset version of the output, which could still
be very useful depending on what you are dumping.
-- Steve
From: Timur Tabi <timur@kernel.org> Date: 2021-01-19 20:52:00
On 1/19/21 2:10 PM, Steven Rostedt wrote:
I'm curious, what is the result if you replaced %p with %pS?
That way you get a kallsyms offset version of the output, which could still
be very useful depending on what you are dumping.
%pS versatile_init+0x0/0x110
The address is question is often not related to any symbol, so it
wouldn't make sense to use %pS.
Maybe you meant %pK? I'm okay with that instead of %px.
From: Steven Rostedt <rostedt@goodmis.org> Date: 2021-01-19 21:17:48
On Tue, 19 Jan 2021 14:49:17 -0600
Timur Tabi [off-list ref] wrote:
On 1/19/21 2:10 PM, Steven Rostedt wrote:
quoted
I'm curious, what is the result if you replaced %p with %pS?
That way you get a kallsyms offset version of the output, which could still
be very useful depending on what you are dumping.
%pS versatile_init+0x0/0x110
The address is question is often not related to any symbol, so it
wouldn't make sense to use %pS.
When it's not related to any symbol, doesn't it still produce an offset
with something close by, that could still give you information that's
better than a hashed number.
Maybe you meant %pK? I'm okay with that instead of %px.
If others are OK with that, perhaps that should be the compromise then?
-- Steve
From: Timur Tabi <timur@kernel.org> Date: 2021-01-19 21:27:14
On 1/19/21 3:15 PM, Steven Rostedt wrote:
When it's not related to any symbol, doesn't it still produce an offset
with something close by, that could still give you information that's
better than a hashed number.
No. I often need the actual unhashed address in the hex dump so that I
can see if some other pointer is correct.
For example, I could be doing pointer math to calculate the address of
some data inside a block. In this case, I would %px the pointer, and
then hex_dump the block. I can then see not only where inside the block
the pointer is pointing to, but what data it points to.
From: Petr Mladek <pmladek@suse.com> Date: 2021-01-20 10:25:02
On Tue 2021-01-19 13:55:29, Timur Tabi wrote:
On 1/19/21 1:45 PM, Kees Cook wrote:
quoted
How about this so the base address is hashed once, with the offset added
to it for each line instead of each line having a "new" hash that makes
no sense:
Well, this is better than nothing, but not by much. Again, as long as %px
exists for printk(), I just cannot understand any resistance to allowing it
in print_hex_dump().
Frankly, I think this patch and my patch should both be added. During
debugging, it's very difficult if not impossible to work with hashed
addresses. I use print_hex_dump() with an unhashed address all the time,
either by applying my patch to my own kernel or just replacing the %p with
%px.
This was my view as well. People should not need to add features into
hexdump code when they want to use is for debugging. And the address
is pretty useful.
A solution might be to prevent using this in the mainline:
+ it might be reported by checkpatch.pl
+ it might print some bold warning on the first use, similar to
trace_printk().
Or we need this in the mainline. Then the use of %pK looks
like the best compromise to me. We could also add some warning
as a comment and use some provocative name for the flag
as suggested by Matthew.
And we should definitely add Linus into CC when sending v2.
His expected opinion has been mentioned several times in this
thread. It would be better to avoid these speculations
and get his real opinion. IMHO, it is too late to add
him in this long thread.
Best Regards,
Petr
From: Matthew Wilcox <willy@infradead.org> Date: 2021-01-20 13:48:21
On Wed, Jan 20, 2021 at 10:19:17AM +0100, Petr Mladek wrote:
And we should definitely add Linus into CC when sending v2.
His expected opinion has been mentioned several times in this
thread. It would be better to avoid these speculations
and get his real opinion. IMHO, it is too late to add
him in this long thread.
He was on the cc since the first mail in this thread.
On Wed, Jan 20, 2021 at 1:19 AM Petr Mladek [off-list ref] wrote:
And we should definitely add Linus into CC when sending v2.
His expected opinion has been mentioned several times in this
thread. It would be better to avoid these speculations
and get his real opinion. IMHO, it is too late to add
him in this long thread.
I've seen it, I've just not cared deeply.
I suspect the main issue is if you can cause debug dumps as a normal
user and find kernel addresses that way, but I'm not sure how much we
care. Somebody _actively_ debugging things might need the address, and
KASRL etc be damned.
I also suspect that everybody has already accepted that KASLR isn't
really working locally anyway (due to all the hw leak models with
cache and TLB timing), so anybody who can look at kernel messages
already probably could figure most of those things out.
So as long as the dumping isn't doing something actively stupid, and
as long as hex dumping isn't something that is easily triggered, this
probably falls under "nobody cares".
Linus
From: Randy Dunlap <hidden> Date: 2021-01-19 20:20:59
On 1/19/21 11:45 AM, Kees Cook wrote:
How about this so the base address is hashed once, with the offset added
to it for each line instead of each line having a "new" hash that makes
no sense:
Yes, good patch. Should have been like this to begin with IMO.
On Tue, Jan 19, 2021 at 12:18:17PM -0800, Randy Dunlap wrote:
On 1/19/21 11:45 AM, Kees Cook wrote:
quoted
How about this so the base address is hashed once, with the offset added
to it for each line instead of each line having a "new" hash that makes
no sense:
Yes, good patch. Should have been like this to begin with IMO.
Is 'addr' always set here?
It is only conditionally set above.
It should be, yes. Though I agree, it's not obvious. ptr_to_hashval()
will write to it when returning 0. So if that fails, addr will have 0
written. Both only happen under DUMP_PREFIX_ADDRESS.
--
Kees Cook