[0/2] introduce DUMP_PREFIX_UNHASHED for hex dumps
mbox series

Message ID 20210116220950.47078-1-timur@kernel.org
Headers show
Series
  • introduce DUMP_PREFIX_UNHASHED for hex dumps
Related show

Message

Timur Tabi Jan. 16, 2021, 10:09 p.m. UTC
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(-)

Comments

Matthew Wilcox Jan. 18, 2021, 6:26 p.m. UTC | #1
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.
Timur Tabi Jan. 18, 2021, 7:03 p.m. UTC | #2
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.
Sergey Senozhatsky Jan. 19, 2021, 12:53 a.m. UTC | #3
On (21/01/18 13:03), Timur Tabi wrote:
> 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.

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
Matthew Wilcox Jan. 19, 2021, 1:47 a.m. UTC | #4
On Tue, Jan 19, 2021 at 09:53:01AM +0900, Sergey Senozhatsky wrote:
> On (21/01/18 13:03), Timur Tabi wrote:
> > 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.
> 
> 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.

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.
Timur Tabi Jan. 19, 2021, 2:30 a.m. UTC | #5
On 1/18/21 6:53 PM, Sergey Senozhatsky wrote:
> I like the idea of a more radical name, e.g. DUMP_PREFIX_RAW_POINTERS or
> something similar.

Is "raw pointer" the common term for unhashed pointers?
Sergey Senozhatsky Jan. 19, 2021, 10:38 a.m. UTC | #6
On (21/01/19 01:47), Matthew Wilcox wrote:
[..]
> 
> > 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
Kees Cook Jan. 19, 2021, 7:45 p.m. UTC | #7
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:
> > On (21/01/18 13:03), Timur Tabi wrote:
> > > 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.
> > 
> > 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.
> 
> 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.

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:

diff --git a/lib/hexdump.c b/lib/hexdump.c
index 9301578f98e8..20264828752d 100644
--- a/lib/hexdump.c
+++ b/lib/hexdump.c
@@ -242,12 +242,17 @@ void print_hex_dump(const char *level, const char *prefix_str, int prefix_type,
 		    const void *buf, size_t len, bool ascii)
 {
 	const u8 *ptr = buf;
+	const u8 *addr;
 	int i, linelen, remaining = len;
 	unsigned char linebuf[32 * 3 + 2 + 32 + 1];
 
 	if (rowsize != 16 && rowsize != 32)
 		rowsize = 16;
 
+	if (prefix_type == DUMP_PREFIX_ADDRESS &&
+	    ptr_to_hashval(ptr, &addr))
+		addr = 0;
+
 	for (i = 0; i < len; i += rowsize) {
 		linelen = min(remaining, rowsize);
 		remaining -= rowsize;
@@ -258,7 +263,7 @@ void print_hex_dump(const char *level, const char *prefix_str, int prefix_type,
 		switch (prefix_type) {
 		case DUMP_PREFIX_ADDRESS:
 			printk("%s%s%p: %s\n",
-			       level, prefix_str, ptr + i, linebuf);
+			       level, prefix_str, addr + i, linebuf);
 			break;
 		case DUMP_PREFIX_OFFSET:
 			printk("%s%s%.8x: %s\n", level, prefix_str, i, linebuf);

-Kees

[1] https://www.kernel.org/doc/html/latest/process/deprecated.html#p-format-specifier
Kees Cook Jan. 19, 2021, 7:45 p.m. UTC | #8
On Tue, Jan 19, 2021 at 07:38:24PM +0900, Sergey Senozhatsky wrote:
> On (21/01/19 01:47), Matthew Wilcox wrote:
> [..]
> > 
> > > 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

Please no, this is effectively a toggle.
Timur Tabi Jan. 19, 2021, 7:55 p.m. UTC | #9
On 1/19/21 1:45 PM, 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:
> 
> diff --git a/lib/hexdump.c b/lib/hexdump.c
> index 9301578f98e8..20264828752d 100644
> --- a/lib/hexdump.c
> +++ b/lib/hexdump.c
> @@ -242,12 +242,17 @@ void print_hex_dump(const char *level, const char *prefix_str, int prefix_type,
>   		    const void *buf, size_t len, bool ascii)
>   {
>   	const u8 *ptr = buf;
> +	const u8 *addr;
>   	int i, linelen, remaining = len;
>   	unsigned char linebuf[32 * 3 + 2 + 32 + 1];
>   
>   	if (rowsize != 16 && rowsize != 32)
>   		rowsize = 16;
>   
> +	if (prefix_type == DUMP_PREFIX_ADDRESS &&
> +	    ptr_to_hashval(ptr, &addr))
> +		addr = 0;
> +
>   	for (i = 0; i < len; i += rowsize) {
>   		linelen = min(remaining, rowsize);
>   		remaining -= rowsize;
> @@ -258,7 +263,7 @@ void print_hex_dump(const char *level, const char *prefix_str, int prefix_type,
>   		switch (prefix_type) {
>   		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.
Steven Rostedt Jan. 19, 2021, 8:10 p.m. UTC | #10
On Tue, 19 Jan 2021 13:55:29 -0600
Timur Tabi <timur@kernel.org> wrote:
> >   		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
Randy Dunlap Jan. 19, 2021, 8:18 p.m. UTC | #11
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.

> diff --git a/lib/hexdump.c b/lib/hexdump.c
> index 9301578f98e8..20264828752d 100644
> --- a/lib/hexdump.c
> +++ b/lib/hexdump.c
> @@ -242,12 +242,17 @@ void print_hex_dump(const char *level, const char *prefix_str, int prefix_type,
>  		    const void *buf, size_t len, bool ascii)
>  {
>  	const u8 *ptr = buf;
> +	const u8 *addr;
>  	int i, linelen, remaining = len;
>  	unsigned char linebuf[32 * 3 + 2 + 32 + 1];
>  
>  	if (rowsize != 16 && rowsize != 32)
>  		rowsize = 16;
>  
> +	if (prefix_type == DUMP_PREFIX_ADDRESS &&
> +	    ptr_to_hashval(ptr, &addr))
> +		addr = 0;
> +
>  	for (i = 0; i < len; i += rowsize) {
>  		linelen = min(remaining, rowsize);
>  		remaining -= rowsize;
> @@ -258,7 +263,7 @@ void print_hex_dump(const char *level, const char *prefix_str, int prefix_type,
>  		switch (prefix_type) {
>  		case DUMP_PREFIX_ADDRESS:
>  			printk("%s%s%p: %s\n",
> -			       level, prefix_str, ptr + i, linebuf);
> +			       level, prefix_str, addr + i, linebuf);

Is 'addr' always set here?
It is only conditionally set above.

>  			break;
>  		case DUMP_PREFIX_OFFSET:
>  			printk("%s%s%.8x: %s\n", level, prefix_str, i, linebuf);
> 
> -Kees
> 
> [1] https://www.kernel.org/doc/html/latest/process/deprecated.html#p-format-specifier
>
Timur Tabi Jan. 19, 2021, 8:49 p.m. UTC | #12
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.
Steven Rostedt Jan. 19, 2021, 9:15 p.m. UTC | #13
On Tue, 19 Jan 2021 14:49:17 -0600
Timur Tabi <timur@kernel.org> wrote:

> 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.

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
Timur Tabi Jan. 19, 2021, 9:25 p.m. UTC | #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.
Petr Mladek Jan. 20, 2021, 9:19 a.m. UTC | #15
On Tue 2021-01-19 13:55:29, Timur Tabi wrote:
> On 1/19/21 1:45 PM, 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:
> > 
> > diff --git a/lib/hexdump.c b/lib/hexdump.c
> > index 9301578f98e8..20264828752d 100644
> > --- a/lib/hexdump.c
> > +++ b/lib/hexdump.c
> > @@ -242,12 +242,17 @@ void print_hex_dump(const char *level, const char *prefix_str, int prefix_type,
> >   		    const void *buf, size_t len, bool ascii)
> >   {
> >   	const u8 *ptr = buf;
> > +	const u8 *addr;
> >   	int i, linelen, remaining = len;
> >   	unsigned char linebuf[32 * 3 + 2 + 32 + 1];
> >   	if (rowsize != 16 && rowsize != 32)
> >   		rowsize = 16;
> > +	if (prefix_type == DUMP_PREFIX_ADDRESS &&
> > +	    ptr_to_hashval(ptr, &addr))
> > +		addr = 0;
> > +
> >   	for (i = 0; i < len; i += rowsize) {
> >   		linelen = min(remaining, rowsize);
> >   		remaining -= rowsize;
> > @@ -258,7 +263,7 @@ void print_hex_dump(const char *level, const char *prefix_str, int prefix_type,
> >   		switch (prefix_type) {
> >   		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.

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
Matthew Wilcox Jan. 20, 2021, 12:17 p.m. UTC | #16
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.
Linus Torvalds Jan. 20, 2021, 7:39 p.m. UTC | #17
On Wed, Jan 20, 2021 at 1:19 AM Petr Mladek <pmladek@suse.com> 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
Kees Cook Jan. 20, 2021, 8:28 p.m. UTC | #18
On Tue, Jan 19, 2021 at 12:18:17PM -0800, Randy Dunlap wrote:
> 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.
> 
> > diff --git a/lib/hexdump.c b/lib/hexdump.c
> > index 9301578f98e8..20264828752d 100644
> > --- a/lib/hexdump.c
> > +++ b/lib/hexdump.c
> > @@ -242,12 +242,17 @@ void print_hex_dump(const char *level, const char *prefix_str, int prefix_type,
> >  		    const void *buf, size_t len, bool ascii)
> >  {
> >  	const u8 *ptr = buf;
> > +	const u8 *addr;
> >  	int i, linelen, remaining = len;
> >  	unsigned char linebuf[32 * 3 + 2 + 32 + 1];
> >  
> >  	if (rowsize != 16 && rowsize != 32)
> >  		rowsize = 16;
> >  
> > +	if (prefix_type == DUMP_PREFIX_ADDRESS &&
> > +	    ptr_to_hashval(ptr, &addr))
> > +		addr = 0;
> > +
> >  	for (i = 0; i < len; i += rowsize) {
> >  		linelen = min(remaining, rowsize);
> >  		remaining -= rowsize;
> > @@ -258,7 +263,7 @@ void print_hex_dump(const char *level, const char *prefix_str, int prefix_type,
> >  		switch (prefix_type) {
> >  		case DUMP_PREFIX_ADDRESS:
> >  			printk("%s%s%p: %s\n",
> > -			       level, prefix_str, ptr + i, linebuf);
> > +			       level, prefix_str, addr + i, linebuf);
> 
> 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.
Vlastimil Babka Jan. 26, 2021, 4:47 p.m. UTC | #19
On 1/19/21 11:38 AM, Sergey Senozhatsky wrote:
> On (21/01/19 01:47), Matthew Wilcox wrote:
> [..]
>> 
>> > 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.

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
>
Timur Tabi Jan. 26, 2021, 4:59 p.m. UTC | #20
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?
Steven Rostedt Jan. 26, 2021, 5:14 p.m. UTC | #21
On Tue, 26 Jan 2021 10:59:12 -0600
Timur Tabi <timur@kernel.org> 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
Vlastimil Babka Jan. 26, 2021, 5:14 p.m. UTC | #22
On 1/26/21 5:59 PM, Timur Tabi wrote:
> 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.

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/
Timur Tabi Jan. 26, 2021, 5:30 p.m. UTC | #23
On 1/26/21 11:14 AM, Vlastimil Babka wrote:
> 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.
Steven Rostedt Jan. 26, 2021, 5:39 p.m. UTC | #24
On Tue, 26 Jan 2021 11:30:02 -0600
Timur Tabi <timur@kernel.org> wrote:

> On 1/26/21 11:14 AM, Vlastimil Babka wrote:
> > 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"

?

-- Steve
Steven Rostedt Jan. 26, 2021, 5:40 p.m. UTC | #25
On Tue, 26 Jan 2021 12:39:12 -0500
Steven Rostedt <rostedt@goodmis.org> wrote:

> On Tue, 26 Jan 2021 11:30:02 -0600
> Timur Tabi <timur@kernel.org> wrote:
> 
> > On 1/26/21 11:14 AM, Vlastimil Babka wrote:  
> > > 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
John Ogness Jan. 26, 2021, 7:23 p.m. UTC | #26
On 2021-01-26, Steven Rostedt <rostedt@goodmis.org> 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
Sergey Senozhatsky Jan. 27, 2021, 2:11 a.m. UTC | #27
On (21/01/26 20:29), John Ogness wrote:
> 
> On 2021-01-26, Steven Rostedt <rostedt@goodmis.org> 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.

+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
Timur Tabi Jan. 27, 2021, 3:22 a.m. UTC | #28
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?

Yes, I'll submit a patch.
Petr Mladek Jan. 27, 2021, 10:11 a.m. UTC | #29
On Tue 2021-01-26 12:40:32, Steven Rostedt wrote:
> On Tue, 26 Jan 2021 12:39:12 -0500
> Steven Rostedt <rostedt@goodmis.org> wrote:
> 
> > On Tue, 26 Jan 2021 11:30:02 -0600
> > Timur Tabi <timur@kernel.org> wrote:
> > 
> > > On 1/26/21 11:14 AM, Vlastimil Babka wrote:  
> > > > 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.

+1

Best Regards,
Petr
Vlastimil Babka Jan. 27, 2021, 10:38 a.m. UTC | #30
On 1/27/21 11:11 AM, Petr Mladek wrote:
> On Tue 2021-01-26 12:40:32, Steven Rostedt wrote:
>> On Tue, 26 Jan 2021 12:39:12 -0500
>> Steven Rostedt <rostedt@goodmis.org> wrote:
>> 
>> > On Tue, 26 Jan 2021 11:30:02 -0600
>> > Timur Tabi <timur@kernel.org> wrote:
>> > 
>> > > On 1/26/21 11:14 AM, Vlastimil Babka wrote:  
>> > > > 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.

>> 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 :)

> Best Regards,
> Petr
>