[PATCH 0/2] introduce DUMP_PREFIX_UNHASHED for hex dumps

[PATCH 0/2] introduce DUMP_PREFIX_UNHASHED for hex dumps

[Timur Tabi]

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

[PATCH 1/2] [v2] lib/hexdump: introduce DUMP_PREFIX_UNHASHED for unhashed addresses

[Timur Tabi]

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 <roman.fietze@magna.com>
---
 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(-)

diff --git a/fs/seq_file.c b/fs/seq_file.c
index 03a369ccd28c..b5b49a855894 100644
--- a/fs/seq_file.c
+++ b/fs/seq_file.c
@@ -864,6 +864,9 @@ void seq_hex_dump(struct seq_file *m, const char *prefix_str, int prefix_type,
 		remaining -= rowsize;
 
 		switch (prefix_type) {
+		case DUMP_PREFIX_UNHASHED:
+			seq_printf(m, "%s%px: ", prefix_str, ptr + i);
+			break;
 		case DUMP_PREFIX_ADDRESS:
 			seq_printf(m, "%s%p: ", prefix_str, ptr + i);
 			break;
diff --git a/include/linux/printk.h b/include/linux/printk.h
index fe7eb2351610..d3c08095a9a3 100644
--- a/include/linux/printk.h
+++ b/include/linux/printk.h
@@ -567,7 +567,8 @@ extern const struct file_operations kmsg_fops;
 enum {
 	DUMP_PREFIX_NONE,
 	DUMP_PREFIX_ADDRESS,
-	DUMP_PREFIX_OFFSET
+	DUMP_PREFIX_OFFSET,
+	DUMP_PREFIX_UNHASHED,
 };
 extern int hex_dump_to_buffer(const void *buf, size_t len, int rowsize,
 			      int groupsize, char *linebuf, size_t linebuflen,
@@ -612,8 +613,9 @@ static inline void print_hex_dump_debug(const char *prefix_str, int prefix_type,
  * print_hex_dump_bytes - shorthand form of print_hex_dump() with default params
  * @prefix_str: string to prefix each line with;
  *  caller supplies trailing spaces for alignment if desired
- * @prefix_type: controls whether prefix of an offset, address, or none
- *  is printed (%DUMP_PREFIX_OFFSET, %DUMP_PREFIX_ADDRESS, %DUMP_PREFIX_NONE)
+ * @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)
  * @buf: data blob to dump
  * @len: number of bytes in the @buf
  *
diff --git a/lib/hexdump.c b/lib/hexdump.c
index 9301578f98e8..b5acfc4168a8 100644
--- a/lib/hexdump.c
+++ b/lib/hexdump.c
@@ -211,8 +211,9 @@ EXPORT_SYMBOL(hex_dump_to_buffer);
  * @level: kernel log level (e.g. KERN_DEBUG)
  * @prefix_str: string to prefix each line with;
  *  caller supplies trailing spaces for alignment if desired
- * @prefix_type: controls whether prefix of an offset, address, or none
- *  is printed (%DUMP_PREFIX_OFFSET, %DUMP_PREFIX_ADDRESS, %DUMP_PREFIX_NONE)
+ * @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)
  * @rowsize: number of bytes to print per line; must be 16 or 32
  * @groupsize: number of bytes to print at a time (1, 2, 4, 8; default = 1)
  * @buf: data blob to dump
@@ -256,6 +257,10 @@ void print_hex_dump(const char *level, const char *prefix_str, int prefix_type,
 				   linebuf, sizeof(linebuf), ascii);
 
 		switch (prefix_type) {
+		case DUMP_PREFIX_UNHASHED:
+			printk("%s%s%px: %s\n",
+			       level, prefix_str, ptr + i, linebuf);
+			break;
 		case DUMP_PREFIX_ADDRESS:
 			printk("%s%s%p: %s\n",
 			       level, prefix_str, ptr + i, linebuf);
diff --git a/lib/seq_buf.c b/lib/seq_buf.c
index 707453f5d58e..017c4d7e93f1 100644
--- a/lib/seq_buf.c
+++ b/lib/seq_buf.c
@@ -335,8 +335,9 @@ int seq_buf_to_user(struct seq_buf *s, char __user *ubuf, int cnt)
  * @s: seq_buf descriptor
  * @prefix_str: string to prefix each line with;
  *  caller supplies trailing spaces for alignment if desired
- * @prefix_type: controls whether prefix of an offset, address, or none
- *  is printed (%DUMP_PREFIX_OFFSET, %DUMP_PREFIX_ADDRESS, %DUMP_PREFIX_NONE)
+ * @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)
  * @rowsize: number of bytes to print per line; must be 16 or 32
  * @groupsize: number of bytes to print at a time (1, 2, 4, 8; default = 1)
  * @buf: data blob to dump
@@ -374,6 +375,10 @@ int seq_buf_hex_dump(struct seq_buf *s, const char *prefix_str, int prefix_type,
 				   linebuf, sizeof(linebuf), ascii);
 
 		switch (prefix_type) {
+		case DUMP_PREFIX_UNHASHED:
+			ret = seq_buf_printf(s, "%s%px: %s\n",
+			       prefix_str, ptr + i, linebuf);
+			break;
 		case DUMP_PREFIX_ADDRESS:
 			ret = seq_buf_printf(s, "%s%p: %s\n",
 			       prefix_str, ptr + i, linebuf);
-- 
2.25.1

[PATCH 2/2] mm/page_poison: use unhashed address in hexdump for check_poison_mem()

[Timur Tabi]

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

diff --git a/mm/page_poison.c b/mm/page_poison.c
index 65cdf844c8ad..4902f3261ee4 100644
--- a/mm/page_poison.c
+++ b/mm/page_poison.c
@@ -67,7 +67,7 @@ static void check_poison_mem(unsigned char *mem, size_t bytes)
 	else
 		pr_err("pagealloc: memory corruption\n");
 
-	print_hex_dump(KERN_ERR, "", DUMP_PREFIX_ADDRESS, 16, 1, start,
+	print_hex_dump(KERN_ERR, "", DUMP_PREFIX_UNHASHED, 16, 1, start,
 			end - start + 1, 1);
 	dump_stack();
 }
-- 
2.25.1

Re: [PATCH 1/2] [v2] lib/hexdump: introduce DUMP_PREFIX_UNHASHED for unhashed addresses

[Andy Shevchenko]

On Sun, Jan 17, 2021 at 12:12 AM Timur Tabi <timur@kernel.org> 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)

...

>  enum {
>         DUMP_PREFIX_NONE,
>         DUMP_PREFIX_ADDRESS,
> -       DUMP_PREFIX_OFFSET
> +       DUMP_PREFIX_OFFSET,
> +       DUMP_PREFIX_UNHASHED,

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)

As above.

...

> +               case DUMP_PREFIX_UNHASHED:

As above.

>                 case DUMP_PREFIX_ADDRESS:


-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 1/2] [v2] lib/hexdump: introduce DUMP_PREFIX_UNHASHED for unhashed addresses

[Andy Shevchenko]

On Sun, Jan 17, 2021 at 12:12 AM Timur Tabi <timur@kernel.org> 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)

...

>  enum {
>         DUMP_PREFIX_NONE,
>         DUMP_PREFIX_ADDRESS,
> -       DUMP_PREFIX_OFFSET
> +       DUMP_PREFIX_OFFSET,
> +       DUMP_PREFIX_UNHASHED,

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)

As above.

...

> +               case DUMP_PREFIX_UNHASHED:

As above.

>                 case DUMP_PREFIX_ADDRESS:


-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 1/2] [v2] lib/hexdump: introduce DUMP_PREFIX_UNHASHED for unhashed addresses

[Timur Tabi]

On 1/18/21 4:03 AM, Andy Shevchenko wrote:
> On Sun, Jan 17, 2021 at 12:12 AM Timur Tabi <timur@kernel.org> 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.

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

It's patch #2 of this set.  They were all sent together.

http://lkml.iu.edu/hypermail/linux/kernel/2101.2/00245.html

Let me know what you think.

>>          DUMP_PREFIX_NONE,
>>          DUMP_PREFIX_ADDRESS,
>> -       DUMP_PREFIX_OFFSET
>> +       DUMP_PREFIX_OFFSET,
>> +       DUMP_PREFIX_UNHASHED,
> 
> 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.

>> + * @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.

>> + * @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.

>> +               case DUMP_PREFIX_UNHASHED:
> 
> Here is a third type of ordering, can you please be consistent?
> 
>>                  case DUMP_PREFIX_ADDRESS:

Fair enough.

Re: [PATCH 1/2] [v2] lib/hexdump: introduce DUMP_PREFIX_UNHASHED for unhashed addresses

[Andy Shevchenko]

On Mon, Jan 18, 2021 at 09:57:55AM -0600, Timur Tabi wrote:
> On 1/18/21 4:03 AM, Andy Shevchenko wrote:
> > On Sun, Jan 17, 2021 at 12:12 AM Timur Tabi <timur@kernel.org> wrote:

...

> > Any user of this? (For the record, I don't see any other mail except this one)

> It's patch #2 of this set.

I haven't got that one.

> They were all sent together.

Apparently not to me.

> http://lkml.iu.edu/hypermail/linux/kernel/2101.2/00245.html
> 
> Let me know what you think.

Makes sense. Hint: use lore.kernel.org references as they are much better in
terms of provided features and patch representation.

...

> > >          DUMP_PREFIX_NONE,
> > >          DUMP_PREFIX_ADDRESS,
> > > -       DUMP_PREFIX_OFFSET
> > > +       DUMP_PREFIX_OFFSET,
> > > +       DUMP_PREFIX_UNHASHED,
> > 
> > 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)?

...

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

Re: [PATCH 1/2] [v2] lib/hexdump: introduce DUMP_PREFIX_UNHASHED for unhashed addresses

[Timur Tabi]

On 1/18/21 11:14 AM, Andy Shevchenko wrote:
> But isn't it good to expose those issues (and fix them)?

I suppose.

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

Re: [PATCH 0/2] introduce DUMP_PREFIX_UNHASHED for hex dumps

[Matthew Wilcox]

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.

Re: [PATCH 0/2] introduce DUMP_PREFIX_UNHASHED for hex dumps

[Timur Tabi]

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.

Re: [PATCH 0/2] introduce DUMP_PREFIX_UNHASHED for hex dumps

[Sergey Senozhatsky]

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

Re: [PATCH 0/2] introduce DUMP_PREFIX_UNHASHED for hex dumps

[Matthew Wilcox]

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.

Re: [PATCH 0/2] introduce DUMP_PREFIX_UNHASHED for hex dumps

[Timur Tabi]

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?

Re: [PATCH 0/2] introduce DUMP_PREFIX_UNHASHED for hex dumps

[Sergey Senozhatsky]

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

Re: [PATCH 0/2] introduce DUMP_PREFIX_UNHASHED for hex dumps

[Kees Cook]

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

Re: [PATCH 0/2] introduce DUMP_PREFIX_UNHASHED for hex dumps

[Kees Cook]

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.

-- 
Kees Cook

Re: [PATCH 0/2] introduce DUMP_PREFIX_UNHASHED for hex dumps

[Timur Tabi]

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.

Re: [PATCH 0/2] introduce DUMP_PREFIX_UNHASHED for hex dumps

[Steven Rostedt]

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

Re: [PATCH 0/2] introduce DUMP_PREFIX_UNHASHED for hex dumps

[Randy Dunlap]

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
> 


-- 
~Randy
"He closes his eyes and drops the goggles.  You can't get hurt
by looking at a bitmap.  Or can you?"
(Neal Stephenson: Snow Crash)

Re: [PATCH 0/2] introduce DUMP_PREFIX_UNHASHED for hex dumps

[Timur Tabi]

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.

Re: [PATCH 0/2] introduce DUMP_PREFIX_UNHASHED for hex dumps

[Steven Rostedt]

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

Re: [PATCH 0/2] introduce DUMP_PREFIX_UNHASHED for hex dumps

[Timur Tabi]

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.

Re: [PATCH 0/2] introduce DUMP_PREFIX_UNHASHED for hex dumps

[Petr Mladek]

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

Re: [PATCH 0/2] introduce DUMP_PREFIX_UNHASHED for hex dumps

[Matthew Wilcox]

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.

Re: [PATCH 0/2] introduce DUMP_PREFIX_UNHASHED for hex dumps

[Linus Torvalds]

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

Re: [PATCH 0/2] introduce DUMP_PREFIX_UNHASHED for hex dumps

[Linus Torvalds]

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

Re: [PATCH 0/2] introduce DUMP_PREFIX_UNHASHED for hex dumps

[Kees Cook]

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.


-- 
Kees Cook

Re: [PATCH 0/2] introduce DUMP_PREFIX_UNHASHED for hex dumps

[Vlastimil Babka]

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
> 

Re: [PATCH 0/2] introduce DUMP_PREFIX_UNHASHED for hex dumps

[Timur Tabi]

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?

Re: [PATCH 0/2] introduce DUMP_PREFIX_UNHASHED for hex dumps

[Steven Rostedt]

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

Re: [PATCH 0/2] introduce DUMP_PREFIX_UNHASHED for hex dumps

[Vlastimil Babka]

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/

Re: [PATCH 0/2] introduce DUMP_PREFIX_UNHASHED for hex dumps

[Timur Tabi]

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.

Re: [PATCH 0/2] introduce DUMP_PREFIX_UNHASHED for hex dumps

[Steven Rostedt]

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

Re: [PATCH 0/2] introduce DUMP_PREFIX_UNHASHED for hex dumps

[Steven Rostedt]

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

Re: [PATCH 0/2] introduce DUMP_PREFIX_UNHASHED for hex dumps

[John Ogness]

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

Re: [PATCH 0/2] introduce DUMP_PREFIX_UNHASHED for hex dumps

[Sergey Senozhatsky]

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

Re: [PATCH 0/2] introduce DUMP_PREFIX_UNHASHED for hex dumps

[Timur Tabi]

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.

Re: [PATCH 0/2] introduce DUMP_PREFIX_UNHASHED for hex dumps

[Petr Mladek]

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

Re: [PATCH 0/2] introduce DUMP_PREFIX_UNHASHED for hex dumps

[Vlastimil Babka]

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
>