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
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 <[email protected]>
---
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
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.
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.
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
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.
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?
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
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
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.
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
On Tue, 19 Jan 2021 13:55:29 -0600
Timur Tabi <[email protected]> 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
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)
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.
On Tue, 19 Jan 2021 14:49:17 -0600
Timur Tabi <[email protected]> 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
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.
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
On Wed, Jan 20, 2021 at 10:19:17AM +0100, Petr Mladek wrote:
> And we should definitely add Linus into CC when sending v2.
> His expected opinion has been mentioned several times in this
> thread. It would be better to avoid these speculations
> and get his real opinion. IMHO, it is too late to add
> him in this long thread.
He was on the cc since the first mail in this thread.
On Wed, Jan 20, 2021 at 1:19 AM Petr Mladek <[email protected]> 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
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
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.
On Tue, 26 Jan 2021 12:39:12 -0500
Steven Rostedt <[email protected]> wrote:
> On Tue, 26 Jan 2021 11:30:02 -0600
> Timur Tabi <[email protected]> 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
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
>
On Tue, 26 Jan 2021 10:59:12 -0600
Timur Tabi <[email protected]> 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
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/
On Tue, 26 Jan 2021 11:30:02 -0600
Timur Tabi <[email protected]> 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
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?
On 2021-01-26, Steven Rostedt <[email protected]> wrote:
> And even if we make this a boot time option, perhaps we should still
> include that nasty dmesg notice, which will let people know that the
> kernel has unhashed values.
+1
The notice would probably be the main motivation for distros/users to
avoid unhashed values unless truly debugging. Which is what we want.
John Ogness
On (21/01/26 20:29), John Ogness wrote:
>
> On 2021-01-26, Steven Rostedt <[email protected]> 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
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.
On Tue 2021-01-26 12:40:32, Steven Rostedt wrote:
> On Tue, 26 Jan 2021 12:39:12 -0500
> Steven Rostedt <[email protected]> wrote:
>
> > On Tue, 26 Jan 2021 11:30:02 -0600
> > Timur Tabi <[email protected]> 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
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 <[email protected]> wrote:
>>
>> > On Tue, 26 Jan 2021 11:30:02 -0600
>> > Timur Tabi <[email protected]> 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
>