* [PATCH 0/2] introduce DUMP_PREFIX_UNHASHED for hex dumps
@ 2021-01-16 22:09 Timur Tabi
2021-01-16 22:09 ` [PATCH 1/2] [v2] lib/hexdump: introduce DUMP_PREFIX_UNHASHED for unhashed addresses Timur Tabi
` (2 more replies)
0 siblings, 3 replies; 37+ messages in thread
From: Timur Tabi @ 2021-01-16 22:09 UTC (permalink / raw)
To: Andrew Morton, linux-kernel, torvalds, Sergey Senozhatsky,
Petr Mladek, roman.fietze, keescook, Steven Rostedt, John Ogness,
linux-mm, Akinobu Mita
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
^ permalink raw reply [flat|nested] 37+ messages in thread* [PATCH 1/2] [v2] lib/hexdump: introduce DUMP_PREFIX_UNHASHED for unhashed addresses 2021-01-16 22:09 [PATCH 0/2] introduce DUMP_PREFIX_UNHASHED for hex dumps Timur Tabi @ 2021-01-16 22:09 ` Timur Tabi 2021-01-18 10:03 ` Andy Shevchenko 2021-01-16 22:09 ` [PATCH 2/2] mm/page_poison: use unhashed address in hexdump for check_poison_mem() Timur Tabi 2021-01-18 18:26 ` [PATCH 0/2] introduce DUMP_PREFIX_UNHASHED for hex dumps Matthew Wilcox 2 siblings, 1 reply; 37+ messages in thread From: Timur Tabi @ 2021-01-16 22:09 UTC (permalink / raw) To: Andrew Morton, linux-kernel, torvalds, Sergey Senozhatsky, Petr Mladek, roman.fietze, keescook, Steven Rostedt, John Ogness, linux-mm, Akinobu Mita, Alexander Viro, Andy Shevchenko, Vaibhav Jain, Dan Williams, linux-fsdevel 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 ^ permalink raw reply related [flat|nested] 37+ messages in thread
* Re: [PATCH 1/2] [v2] lib/hexdump: introduce DUMP_PREFIX_UNHASHED for unhashed addresses 2021-01-16 22:09 ` [PATCH 1/2] [v2] lib/hexdump: introduce DUMP_PREFIX_UNHASHED for unhashed addresses Timur Tabi @ 2021-01-18 10:03 ` Andy Shevchenko 2021-01-18 15:57 ` Timur Tabi 0 siblings, 1 reply; 37+ messages in thread From: Andy Shevchenko @ 2021-01-18 10:03 UTC (permalink / raw) To: Timur Tabi Cc: Andrew Morton, Linux Kernel Mailing List, Linus Torvalds, Sergey Senozhatsky, Petr Mladek, roman.fietze, Kees Cook, Steven Rostedt, John Ogness, linux-mm, Akinobu Mita, Alexander Viro, Andy Shevchenko, Vaibhav Jain, Dan Williams, Linux FS Devel 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 ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH 1/2] [v2] lib/hexdump: introduce DUMP_PREFIX_UNHASHED for unhashed addresses 2021-01-18 10:03 ` Andy Shevchenko @ 2021-01-18 15:57 ` Timur Tabi 2021-01-18 17:14 ` Andy Shevchenko 0 siblings, 1 reply; 37+ messages in thread From: Timur Tabi @ 2021-01-18 15:57 UTC (permalink / raw) To: Andy Shevchenko Cc: Andrew Morton, Linux Kernel Mailing List, Linus Torvalds, Sergey Senozhatsky, Petr Mladek, roman.fietze, Kees Cook, Steven Rostedt, John Ogness, linux-mm, Akinobu Mita, Alexander Viro, Andy Shevchenko, Vaibhav Jain, Dan Williams, Linux FS Devel 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. ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH 1/2] [v2] lib/hexdump: introduce DUMP_PREFIX_UNHASHED for unhashed addresses 2021-01-18 15:57 ` Timur Tabi @ 2021-01-18 17:14 ` Andy Shevchenko 2021-01-18 17:53 ` Timur Tabi 0 siblings, 1 reply; 37+ messages in thread From: Andy Shevchenko @ 2021-01-18 17:14 UTC (permalink / raw) To: Timur Tabi Cc: Andrew Morton, Linux Kernel Mailing List, Linus Torvalds, Sergey Senozhatsky, Petr Mladek, roman.fietze, Kees Cook, Steven Rostedt, John Ogness, linux-mm, Akinobu Mita, Alexander Viro, Vaibhav Jain, Dan Williams, Linux FS Devel 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 ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH 1/2] [v2] lib/hexdump: introduce DUMP_PREFIX_UNHASHED for unhashed addresses 2021-01-18 17:14 ` Andy Shevchenko @ 2021-01-18 17:53 ` Timur Tabi 0 siblings, 0 replies; 37+ messages in thread From: Timur Tabi @ 2021-01-18 17:53 UTC (permalink / raw) To: Andy Shevchenko Cc: Andrew Morton, Linux Kernel Mailing List, Linus Torvalds, Sergey Senozhatsky, Petr Mladek, roman.fietze, Kees Cook, Steven Rostedt, John Ogness, linux-mm, Akinobu Mita, Alexander Viro, Vaibhav Jain, Dan Williams, Linux FS Devel 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. ^ permalink raw reply [flat|nested] 37+ messages in thread
* [PATCH 2/2] mm/page_poison: use unhashed address in hexdump for check_poison_mem() 2021-01-16 22:09 [PATCH 0/2] introduce DUMP_PREFIX_UNHASHED for hex dumps Timur Tabi 2021-01-16 22:09 ` [PATCH 1/2] [v2] lib/hexdump: introduce DUMP_PREFIX_UNHASHED for unhashed addresses Timur Tabi @ 2021-01-16 22:09 ` Timur Tabi 2021-01-18 18:26 ` [PATCH 0/2] introduce DUMP_PREFIX_UNHASHED for hex dumps Matthew Wilcox 2 siblings, 0 replies; 37+ messages in thread From: Timur Tabi @ 2021-01-16 22:09 UTC (permalink / raw) To: Andrew Morton, linux-kernel, torvalds, Sergey Senozhatsky, Petr Mladek, roman.fietze, keescook, Steven Rostedt, John Ogness, linux-mm, Akinobu Mita 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 ^ permalink raw reply related [flat|nested] 37+ messages in thread
* Re: [PATCH 0/2] introduce DUMP_PREFIX_UNHASHED for hex dumps 2021-01-16 22:09 [PATCH 0/2] introduce DUMP_PREFIX_UNHASHED for hex dumps Timur Tabi 2021-01-16 22:09 ` [PATCH 1/2] [v2] lib/hexdump: introduce DUMP_PREFIX_UNHASHED for unhashed addresses Timur Tabi 2021-01-16 22:09 ` [PATCH 2/2] mm/page_poison: use unhashed address in hexdump for check_poison_mem() Timur Tabi @ 2021-01-18 18:26 ` Matthew Wilcox 2021-01-18 19:03 ` Timur Tabi 2 siblings, 1 reply; 37+ messages in thread From: Matthew Wilcox @ 2021-01-18 18:26 UTC (permalink / raw) To: Timur Tabi Cc: Andrew Morton, linux-kernel, torvalds, Sergey Senozhatsky, Petr Mladek, roman.fietze, keescook, Steven Rostedt, John Ogness, linux-mm, Akinobu Mita 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. ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH 0/2] introduce DUMP_PREFIX_UNHASHED for hex dumps 2021-01-18 18:26 ` [PATCH 0/2] introduce DUMP_PREFIX_UNHASHED for hex dumps Matthew Wilcox @ 2021-01-18 19:03 ` Timur Tabi 2021-01-19 0:53 ` Sergey Senozhatsky 0 siblings, 1 reply; 37+ messages in thread From: Timur Tabi @ 2021-01-18 19:03 UTC (permalink / raw) To: Matthew Wilcox Cc: Andrew Morton, linux-kernel, torvalds, Sergey Senozhatsky, Petr Mladek, roman.fietze, keescook, Steven Rostedt, John Ogness, linux-mm, Akinobu Mita 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. ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH 0/2] introduce DUMP_PREFIX_UNHASHED for hex dumps 2021-01-18 19:03 ` Timur Tabi @ 2021-01-19 0:53 ` Sergey Senozhatsky 2021-01-19 1:47 ` Matthew Wilcox 2021-01-19 2:30 ` Timur Tabi 0 siblings, 2 replies; 37+ messages in thread From: Sergey Senozhatsky @ 2021-01-19 0:53 UTC (permalink / raw) To: Timur Tabi Cc: Matthew Wilcox, Andrew Morton, linux-kernel, torvalds, Sergey Senozhatsky, Petr Mladek, roman.fietze, keescook, Steven Rostedt, John Ogness, linux-mm, Akinobu Mita 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 ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH 0/2] introduce DUMP_PREFIX_UNHASHED for hex dumps 2021-01-19 0:53 ` Sergey Senozhatsky @ 2021-01-19 1:47 ` Matthew Wilcox 2021-01-19 10:38 ` Sergey Senozhatsky 2021-01-19 19:45 ` Kees Cook 2021-01-19 2:30 ` Timur Tabi 1 sibling, 2 replies; 37+ messages in thread From: Matthew Wilcox @ 2021-01-19 1:47 UTC (permalink / raw) To: Sergey Senozhatsky Cc: Timur Tabi, Andrew Morton, linux-kernel, torvalds, Petr Mladek, roman.fietze, keescook, Steven Rostedt, John Ogness, linux-mm, Akinobu Mita 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. ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH 0/2] introduce DUMP_PREFIX_UNHASHED for hex dumps 2021-01-19 1:47 ` Matthew Wilcox @ 2021-01-19 10:38 ` Sergey Senozhatsky 2021-01-19 19:45 ` Kees Cook 2021-01-26 16:47 ` Vlastimil Babka 2021-01-19 19:45 ` Kees Cook 1 sibling, 2 replies; 37+ messages in thread From: Sergey Senozhatsky @ 2021-01-19 10:38 UTC (permalink / raw) To: Matthew Wilcox Cc: Sergey Senozhatsky, Timur Tabi, Andrew Morton, linux-kernel, torvalds, Petr Mladek, roman.fietze, keescook, Steven Rostedt, John Ogness, linux-mm, Akinobu Mita 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 ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH 0/2] introduce DUMP_PREFIX_UNHASHED for hex dumps 2021-01-19 10:38 ` Sergey Senozhatsky @ 2021-01-19 19:45 ` Kees Cook 2021-01-26 16:47 ` Vlastimil Babka 1 sibling, 0 replies; 37+ messages in thread From: Kees Cook @ 2021-01-19 19:45 UTC (permalink / raw) To: Sergey Senozhatsky Cc: Matthew Wilcox, Timur Tabi, Andrew Morton, linux-kernel, torvalds, Petr Mladek, roman.fietze, Steven Rostedt, John Ogness, linux-mm, Akinobu Mita 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 ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH 0/2] introduce DUMP_PREFIX_UNHASHED for hex dumps 2021-01-19 10:38 ` Sergey Senozhatsky 2021-01-19 19:45 ` Kees Cook @ 2021-01-26 16:47 ` Vlastimil Babka 2021-01-26 16:59 ` Timur Tabi 1 sibling, 1 reply; 37+ messages in thread From: Vlastimil Babka @ 2021-01-26 16:47 UTC (permalink / raw) To: Sergey Senozhatsky, Matthew Wilcox Cc: Timur Tabi, Andrew Morton, linux-kernel, torvalds, Petr Mladek, roman.fietze, keescook, Steven Rostedt, John Ogness, linux-mm, Akinobu Mita 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 > ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH 0/2] introduce DUMP_PREFIX_UNHASHED for hex dumps 2021-01-26 16:47 ` Vlastimil Babka @ 2021-01-26 16:59 ` Timur Tabi 2021-01-26 17:14 ` Steven Rostedt 2021-01-26 17:14 ` Vlastimil Babka 0 siblings, 2 replies; 37+ messages in thread From: Timur Tabi @ 2021-01-26 16:59 UTC (permalink / raw) To: Vlastimil Babka, Sergey Senozhatsky, Matthew Wilcox Cc: Andrew Morton, linux-kernel, torvalds, Petr Mladek, roman.fietze, keescook, Steven Rostedt, John Ogness, linux-mm, Akinobu Mita 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? ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH 0/2] introduce DUMP_PREFIX_UNHASHED for hex dumps 2021-01-26 16:59 ` Timur Tabi @ 2021-01-26 17:14 ` Steven Rostedt 2021-01-26 17:14 ` Vlastimil Babka 1 sibling, 0 replies; 37+ messages in thread From: Steven Rostedt @ 2021-01-26 17:14 UTC (permalink / raw) To: Timur Tabi Cc: Vlastimil Babka, Sergey Senozhatsky, Matthew Wilcox, Andrew Morton, linux-kernel, torvalds, Petr Mladek, roman.fietze, keescook, John Ogness, linux-mm, Akinobu Mita 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 ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH 0/2] introduce DUMP_PREFIX_UNHASHED for hex dumps 2021-01-26 16:59 ` Timur Tabi 2021-01-26 17:14 ` Steven Rostedt @ 2021-01-26 17:14 ` Vlastimil Babka 2021-01-26 17:30 ` Timur Tabi 1 sibling, 1 reply; 37+ messages in thread From: Vlastimil Babka @ 2021-01-26 17:14 UTC (permalink / raw) To: Timur Tabi, Sergey Senozhatsky, Matthew Wilcox Cc: Andrew Morton, linux-kernel, torvalds, Petr Mladek, roman.fietze, keescook, Steven Rostedt, John Ogness, linux-mm, Akinobu Mita 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/ ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH 0/2] introduce DUMP_PREFIX_UNHASHED for hex dumps 2021-01-26 17:14 ` Vlastimil Babka @ 2021-01-26 17:30 ` Timur Tabi 2021-01-26 17:39 ` Steven Rostedt 0 siblings, 1 reply; 37+ messages in thread From: Timur Tabi @ 2021-01-26 17:30 UTC (permalink / raw) To: Vlastimil Babka, Sergey Senozhatsky, Matthew Wilcox Cc: Andrew Morton, linux-kernel, torvalds, Petr Mladek, roman.fietze, keescook, Steven Rostedt, John Ogness, linux-mm, Akinobu Mita 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. ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH 0/2] introduce DUMP_PREFIX_UNHASHED for hex dumps 2021-01-26 17:30 ` Timur Tabi @ 2021-01-26 17:39 ` Steven Rostedt 2021-01-26 17:40 ` Steven Rostedt 0 siblings, 1 reply; 37+ messages in thread From: Steven Rostedt @ 2021-01-26 17:39 UTC (permalink / raw) To: Timur Tabi Cc: Vlastimil Babka, Sergey Senozhatsky, Matthew Wilcox, Andrew Morton, linux-kernel, torvalds, Petr Mladek, roman.fietze, keescook, John Ogness, linux-mm, Akinobu Mita 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 ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH 0/2] introduce DUMP_PREFIX_UNHASHED for hex dumps 2021-01-26 17:39 ` Steven Rostedt @ 2021-01-26 17:40 ` Steven Rostedt 2021-01-26 19:23 ` John Ogness 2021-01-27 10:11 ` Petr Mladek 0 siblings, 2 replies; 37+ messages in thread From: Steven Rostedt @ 2021-01-26 17:40 UTC (permalink / raw) To: Timur Tabi Cc: Vlastimil Babka, Sergey Senozhatsky, Matthew Wilcox, Andrew Morton, linux-kernel, torvalds, Petr Mladek, roman.fietze, keescook, John Ogness, linux-mm, Akinobu Mita 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 ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH 0/2] introduce DUMP_PREFIX_UNHASHED for hex dumps 2021-01-26 17:40 ` Steven Rostedt @ 2021-01-26 19:23 ` John Ogness 2021-01-27 2:11 ` Sergey Senozhatsky 2021-01-27 10:11 ` Petr Mladek 1 sibling, 1 reply; 37+ messages in thread From: John Ogness @ 2021-01-26 19:23 UTC (permalink / raw) To: Steven Rostedt, Timur Tabi Cc: Vlastimil Babka, Sergey Senozhatsky, Matthew Wilcox, Andrew Morton, linux-kernel, torvalds, Petr Mladek, roman.fietze, keescook, linux-mm, Akinobu Mita 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 ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH 0/2] introduce DUMP_PREFIX_UNHASHED for hex dumps 2021-01-26 19:23 ` John Ogness @ 2021-01-27 2:11 ` Sergey Senozhatsky 2021-01-27 3:22 ` Timur Tabi 0 siblings, 1 reply; 37+ messages in thread From: Sergey Senozhatsky @ 2021-01-27 2:11 UTC (permalink / raw) To: John Ogness, Steven Rostedt, Timur Tabi, Vlastimil Babka Cc: Sergey Senozhatsky, Matthew Wilcox, Andrew Morton, linux-kernel, torvalds, Petr Mladek, roman.fietze, keescook, linux-mm, Akinobu Mita 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 ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH 0/2] introduce DUMP_PREFIX_UNHASHED for hex dumps 2021-01-27 2:11 ` Sergey Senozhatsky @ 2021-01-27 3:22 ` Timur Tabi 0 siblings, 0 replies; 37+ messages in thread From: Timur Tabi @ 2021-01-27 3:22 UTC (permalink / raw) To: Sergey Senozhatsky, John Ogness, Steven Rostedt, Vlastimil Babka Cc: Matthew Wilcox, Andrew Morton, linux-kernel, torvalds, Petr Mladek, roman.fietze, keescook, linux-mm, Akinobu Mita 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. ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH 0/2] introduce DUMP_PREFIX_UNHASHED for hex dumps 2021-01-26 17:40 ` Steven Rostedt 2021-01-26 19:23 ` John Ogness @ 2021-01-27 10:11 ` Petr Mladek 2021-01-27 10:38 ` Vlastimil Babka 1 sibling, 1 reply; 37+ messages in thread From: Petr Mladek @ 2021-01-27 10:11 UTC (permalink / raw) To: Steven Rostedt Cc: Timur Tabi, Vlastimil Babka, Sergey Senozhatsky, Matthew Wilcox, Andrew Morton, linux-kernel, torvalds, roman.fietze, keescook, John Ogness, linux-mm, Akinobu Mita 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 ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH 0/2] introduce DUMP_PREFIX_UNHASHED for hex dumps 2021-01-27 10:11 ` Petr Mladek @ 2021-01-27 10:38 ` Vlastimil Babka 0 siblings, 0 replies; 37+ messages in thread From: Vlastimil Babka @ 2021-01-27 10:38 UTC (permalink / raw) To: Petr Mladek, Steven Rostedt Cc: Timur Tabi, Sergey Senozhatsky, Matthew Wilcox, Andrew Morton, linux-kernel, torvalds, roman.fietze, keescook, John Ogness, linux-mm, Akinobu Mita 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 > ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH 0/2] introduce DUMP_PREFIX_UNHASHED for hex dumps 2021-01-19 1:47 ` Matthew Wilcox 2021-01-19 10:38 ` Sergey Senozhatsky @ 2021-01-19 19:45 ` Kees Cook 2021-01-19 19:55 ` Timur Tabi 2021-01-19 20:18 ` Randy Dunlap 1 sibling, 2 replies; 37+ messages in thread From: Kees Cook @ 2021-01-19 19:45 UTC (permalink / raw) To: Matthew Wilcox Cc: Sergey Senozhatsky, Timur Tabi, Andrew Morton, linux-kernel, torvalds, Petr Mladek, roman.fietze, Steven Rostedt, John Ogness, linux-mm, Akinobu Mita 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 ^ permalink raw reply related [flat|nested] 37+ messages in thread
* Re: [PATCH 0/2] introduce DUMP_PREFIX_UNHASHED for hex dumps 2021-01-19 19:45 ` Kees Cook @ 2021-01-19 19:55 ` Timur Tabi 2021-01-19 20:10 ` Steven Rostedt 2021-01-20 9:19 ` Petr Mladek 2021-01-19 20:18 ` Randy Dunlap 1 sibling, 2 replies; 37+ messages in thread From: Timur Tabi @ 2021-01-19 19:55 UTC (permalink / raw) To: Kees Cook, Matthew Wilcox Cc: Sergey Senozhatsky, Andrew Morton, linux-kernel, torvalds, Petr Mladek, roman.fietze, Steven Rostedt, John Ogness, linux-mm, Akinobu Mita 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. ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH 0/2] introduce DUMP_PREFIX_UNHASHED for hex dumps 2021-01-19 19:55 ` Timur Tabi @ 2021-01-19 20:10 ` Steven Rostedt 2021-01-19 20:49 ` Timur Tabi 2021-01-20 9:19 ` Petr Mladek 1 sibling, 1 reply; 37+ messages in thread From: Steven Rostedt @ 2021-01-19 20:10 UTC (permalink / raw) To: Timur Tabi Cc: Kees Cook, Matthew Wilcox, Sergey Senozhatsky, Andrew Morton, linux-kernel, torvalds, Petr Mladek, roman.fietze, John Ogness, linux-mm, Akinobu Mita 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 ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH 0/2] introduce DUMP_PREFIX_UNHASHED for hex dumps 2021-01-19 20:10 ` Steven Rostedt @ 2021-01-19 20:49 ` Timur Tabi 2021-01-19 21:15 ` Steven Rostedt 0 siblings, 1 reply; 37+ messages in thread From: Timur Tabi @ 2021-01-19 20:49 UTC (permalink / raw) To: Steven Rostedt Cc: Kees Cook, Matthew Wilcox, Sergey Senozhatsky, Andrew Morton, linux-kernel, torvalds, Petr Mladek, roman.fietze, John Ogness, linux-mm, Akinobu Mita 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. ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH 0/2] introduce DUMP_PREFIX_UNHASHED for hex dumps 2021-01-19 20:49 ` Timur Tabi @ 2021-01-19 21:15 ` Steven Rostedt 2021-01-19 21:25 ` Timur Tabi 0 siblings, 1 reply; 37+ messages in thread From: Steven Rostedt @ 2021-01-19 21:15 UTC (permalink / raw) To: Timur Tabi Cc: Kees Cook, Matthew Wilcox, Sergey Senozhatsky, Andrew Morton, linux-kernel, torvalds, Petr Mladek, roman.fietze, John Ogness, linux-mm, Akinobu Mita 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 ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH 0/2] introduce DUMP_PREFIX_UNHASHED for hex dumps 2021-01-19 21:15 ` Steven Rostedt @ 2021-01-19 21:25 ` Timur Tabi 0 siblings, 0 replies; 37+ messages in thread From: Timur Tabi @ 2021-01-19 21:25 UTC (permalink / raw) To: Steven Rostedt Cc: Kees Cook, Matthew Wilcox, Sergey Senozhatsky, Andrew Morton, linux-kernel, torvalds, Petr Mladek, roman.fietze, John Ogness, linux-mm, Akinobu Mita 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. ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH 0/2] introduce DUMP_PREFIX_UNHASHED for hex dumps 2021-01-19 19:55 ` Timur Tabi 2021-01-19 20:10 ` Steven Rostedt @ 2021-01-20 9:19 ` Petr Mladek 2021-01-20 12:17 ` Matthew Wilcox 2021-01-20 19:39 ` Linus Torvalds 1 sibling, 2 replies; 37+ messages in thread From: Petr Mladek @ 2021-01-20 9:19 UTC (permalink / raw) To: Timur Tabi Cc: Kees Cook, Matthew Wilcox, Sergey Senozhatsky, Andrew Morton, linux-kernel, torvalds, roman.fietze, Steven Rostedt, John Ogness, linux-mm, Akinobu Mita 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 ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH 0/2] introduce DUMP_PREFIX_UNHASHED for hex dumps 2021-01-20 9:19 ` Petr Mladek @ 2021-01-20 12:17 ` Matthew Wilcox 2021-01-20 19:39 ` Linus Torvalds 1 sibling, 0 replies; 37+ messages in thread From: Matthew Wilcox @ 2021-01-20 12:17 UTC (permalink / raw) To: Petr Mladek Cc: Timur Tabi, Kees Cook, Sergey Senozhatsky, Andrew Morton, linux-kernel, torvalds, roman.fietze, Steven Rostedt, John Ogness, linux-mm, Akinobu Mita 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. ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH 0/2] introduce DUMP_PREFIX_UNHASHED for hex dumps 2021-01-20 9:19 ` Petr Mladek 2021-01-20 12:17 ` Matthew Wilcox @ 2021-01-20 19:39 ` Linus Torvalds 1 sibling, 0 replies; 37+ messages in thread From: Linus Torvalds @ 2021-01-20 19:39 UTC (permalink / raw) To: Petr Mladek Cc: Timur Tabi, Kees Cook, Matthew Wilcox, Sergey Senozhatsky, Andrew Morton, Linux Kernel Mailing List, roman.fietze, Steven Rostedt, John Ogness, Linux-MM, Akinobu Mita 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 ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH 0/2] introduce DUMP_PREFIX_UNHASHED for hex dumps 2021-01-19 19:45 ` Kees Cook 2021-01-19 19:55 ` Timur Tabi @ 2021-01-19 20:18 ` Randy Dunlap 2021-01-20 20:28 ` Kees Cook 1 sibling, 1 reply; 37+ messages in thread From: Randy Dunlap @ 2021-01-19 20:18 UTC (permalink / raw) To: Kees Cook, Matthew Wilcox Cc: Sergey Senozhatsky, Timur Tabi, Andrew Morton, linux-kernel, torvalds, Petr Mladek, roman.fietze, Steven Rostedt, John Ogness, linux-mm, Akinobu Mita 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) ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH 0/2] introduce DUMP_PREFIX_UNHASHED for hex dumps 2021-01-19 20:18 ` Randy Dunlap @ 2021-01-20 20:28 ` Kees Cook 0 siblings, 0 replies; 37+ messages in thread From: Kees Cook @ 2021-01-20 20:28 UTC (permalink / raw) To: Randy Dunlap Cc: Matthew Wilcox, Sergey Senozhatsky, Timur Tabi, Andrew Morton, linux-kernel, torvalds, Petr Mladek, roman.fietze, Steven Rostedt, John Ogness, linux-mm, Akinobu Mita 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 ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH 0/2] introduce DUMP_PREFIX_UNHASHED for hex dumps 2021-01-19 0:53 ` Sergey Senozhatsky 2021-01-19 1:47 ` Matthew Wilcox @ 2021-01-19 2:30 ` Timur Tabi 1 sibling, 0 replies; 37+ messages in thread From: Timur Tabi @ 2021-01-19 2:30 UTC (permalink / raw) To: Sergey Senozhatsky Cc: Matthew Wilcox, Andrew Morton, linux-kernel, torvalds, Petr Mladek, roman.fietze, keescook, Steven Rostedt, John Ogness, linux-mm, Akinobu Mita 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? ^ permalink raw reply [flat|nested] 37+ messages in thread
end of thread, other threads:[~2021-01-27 10:42 UTC | newest] Thread overview: 37+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-01-16 22:09 [PATCH 0/2] introduce DUMP_PREFIX_UNHASHED for hex dumps Timur Tabi 2021-01-16 22:09 ` [PATCH 1/2] [v2] lib/hexdump: introduce DUMP_PREFIX_UNHASHED for unhashed addresses Timur Tabi 2021-01-18 10:03 ` Andy Shevchenko 2021-01-18 15:57 ` Timur Tabi 2021-01-18 17:14 ` Andy Shevchenko 2021-01-18 17:53 ` Timur Tabi 2021-01-16 22:09 ` [PATCH 2/2] mm/page_poison: use unhashed address in hexdump for check_poison_mem() Timur Tabi 2021-01-18 18:26 ` [PATCH 0/2] introduce DUMP_PREFIX_UNHASHED for hex dumps Matthew Wilcox 2021-01-18 19:03 ` Timur Tabi 2021-01-19 0:53 ` Sergey Senozhatsky 2021-01-19 1:47 ` Matthew Wilcox 2021-01-19 10:38 ` Sergey Senozhatsky 2021-01-19 19:45 ` Kees Cook 2021-01-26 16:47 ` Vlastimil Babka 2021-01-26 16:59 ` Timur Tabi 2021-01-26 17:14 ` Steven Rostedt 2021-01-26 17:14 ` Vlastimil Babka 2021-01-26 17:30 ` Timur Tabi 2021-01-26 17:39 ` Steven Rostedt 2021-01-26 17:40 ` Steven Rostedt 2021-01-26 19:23 ` John Ogness 2021-01-27 2:11 ` Sergey Senozhatsky 2021-01-27 3:22 ` Timur Tabi 2021-01-27 10:11 ` Petr Mladek 2021-01-27 10:38 ` Vlastimil Babka 2021-01-19 19:45 ` Kees Cook 2021-01-19 19:55 ` Timur Tabi 2021-01-19 20:10 ` Steven Rostedt 2021-01-19 20:49 ` Timur Tabi 2021-01-19 21:15 ` Steven Rostedt 2021-01-19 21:25 ` Timur Tabi 2021-01-20 9:19 ` Petr Mladek 2021-01-20 12:17 ` Matthew Wilcox 2021-01-20 19:39 ` Linus Torvalds 2021-01-19 20:18 ` Randy Dunlap 2021-01-20 20:28 ` Kees Cook 2021-01-19 2:30 ` Timur Tabi
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).