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

[PATCH] 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@tabi.org>
Cc: Roman Fietze <roman.fietze@magna.com>
---
 include/linux/printk.h | 3 ++-
 lib/hexdump.c          | 9 +++++++--
 2 files changed, 9 insertions(+), 3 deletions(-)

diff --git a/include/linux/printk.h b/include/linux/printk.h
index fe7eb2351610..5d833bad785c 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,
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);
-- 
2.25.1

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

[Petr Mladek]

Adding Kees into CC because it is security related.
Adding Andrew into CC because he usually takes patches for hexdump.

On Wed 2021-01-06 15:35:47, Timur Tabi wrote:
> 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@tabi.org>
> Cc: Roman Fietze <roman.fietze@magna.com>

I agree that there should be way to print the real address.

And it is sane to add a new mode so that the current
users stay hashed.

Reviewed-by: Petr Mladek <pmladek@suse.com>

> ---
>  include/linux/printk.h | 3 ++-
>  lib/hexdump.c          | 9 +++++++--
>  2 files changed, 9 insertions(+), 3 deletions(-)
> 
> diff --git a/include/linux/printk.h b/include/linux/printk.h
> index fe7eb2351610..5d833bad785c 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,
> 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);
> -- 
> 2.25.1

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

[Andrew Morton]

On Mon, 11 Jan 2021 11:10:56 +0100 Petr Mladek <pmladek@suse.com> wrote:

> Adding Kees into CC because it is security related.
> Adding Andrew into CC because he usually takes patches for hexdump.
> 
> On Wed 2021-01-06 15:35:47, Timur Tabi wrote:
> > 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@tabi.org>
> > Cc: Roman Fietze <roman.fietze@magna.com>
> 
> I agree that there should be way to print the real address.
> 
> And it is sane to add a new mode so that the current
> users stay hashed.
> 

I doubt if Kees (or I or anyone else) can review this change because
there are no callers which actually use the new DUMP_PREFIX_UNHASHED. 
Is it intended that some other places in the kernel be changed to use
this?  If so, please describe where and why, so that others can better
understand both the requirement and the security implications.

If it is intended that this be used mainly for developer debug and not
to be shipped in the mainline kernel then let's get this info into the
changelog as well.

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

[Timur Tabi]

On 1/11/21 7:30 PM, Andrew Morton wrote:
> I doubt if Kees (or I or anyone else) can review this change because
> there are no callers which actually use the new DUMP_PREFIX_UNHASHED.
> Is it intended that some other places in the kernel be changed to use
> this?  If so, please describe where and why, so that others can better
> understand both the requirement and the security implications.

In my opinion, hashed addresses make no sense in a hexdump, so I would 
say that ALL callers should change.  But none of the drivers I've 
written call print_hex_dump(), so I can't make those changes myself.

> If it is intended that this be used mainly for developer debug and not
> to be shipped in the mainline kernel then let's get this info into the
> changelog as well.

I definitely want this patch included in the mainline kernel.  Just 
because there aren't any users today doesn't mean that there won't be. 
In fact, I suspect that most current users haven't noticed that the 
addresses have changed or don't care any more, but if they were to write 
the code today, they would use unhashed addresses.

If you want, I can include a patch that changes a few callers of 
print_hex_dump() to use DUMP_PREFIX_UNHASHED, based on what I think 
would be useful.

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

[Petr Mladek]

On Thu 2021-01-14 20:56:36, Timur Tabi wrote:
> On 1/11/21 7:30 PM, Andrew Morton wrote:
> > I doubt if Kees (or I or anyone else) can review this change because
> > there are no callers which actually use the new DUMP_PREFIX_UNHASHED.
> > Is it intended that some other places in the kernel be changed to use
> > this?  If so, please describe where and why, so that others can better
> > understand both the requirement and the security implications.
> 
> In my opinion, hashed addresses make no sense in a hexdump, so I would say
> that ALL callers should change.  But none of the drivers I've written call
> print_hex_dump(), so I can't make those changes myself.

I know that you probably know it because you introduced new mode
instead of updating the existing one. But to be sure.

We need to be careful here. The hashed pointer has been introduced for
a reason. It prevents leaking pointers and helping bad guys.

The original plan was to introduce %pK. It was supposed to prevent
non-privileged users from seeing the real pointer value. It did not
really worked because it was only rarely used. The plain %p was
heavily used in historical and even in a new code.

By other words, every print_hex_dump() used need to be reviewed in
which context might be called.

> > If it is intended that this be used mainly for developer debug and not
> > to be shipped in the mainline kernel then let's get this info into the
> > changelog as well.
> 
> I definitely want this patch included in the mainline kernel.  Just because
> there aren't any users today doesn't mean that there won't be. In fact, I
> suspect that most current users haven't noticed that the addresses have
> changed or don't care any more, but if they were to write the code today,
> they would use unhashed addresses.

I am pretty sure that will look for this functionality sooner or
later. The hashed pointers make debugging really complicated.

> If you want, I can include a patch that changes a few callers of
> print_hex_dump() to use DUMP_PREFIX_UNHASHED, based on what I think would be
> useful.

It would be nice.

Best Regards,
Petr

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

[Timur Tabi]

On Fri, Jan 15, 2021 at 3:24 AM Petr Mladek <pmladek@suse.com> wrote:

> By other words, every print_hex_dump() used need to be reviewed in
> which context might be called.

I did a rough analysis of all current usage of DUMP_PREFIX_ADDRESS in
the kernel, compared to the introduction of %px and hashed addresses,
using git-blame to find the most recent commit that modifies a line
that contains _ADDRESS.  Of 150 cases, 110 of them are newer than %px,
so I'm assuming that these developers chose _ADDRESS instead of
_OFFSET knowing that the addresses are hashed.

> > If you want, I can include a patch that changes a few callers of
> > print_hex_dump() to use DUMP_PREFIX_UNHASHED, based on what I think would be
> > useful.
>
> It would be nice.

I have a v2 that I'm about to post, and I included a patch that
modifies check_poison_mem() in mm/page_poison.c.  I chose this file
because I figured actual addresses would probably be necessary for
tracking memory-related errors.  Also, that call to print_hex_dump()
was added back in 2009, so it predates the introduction of %px.