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

[Kees Cook <keescook@chromium.org>]

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