On Mon, Feb 24, 2025 at 06:54:47PM +0000, Maciej W. Rozycki wrote:
> On Mon, 24 Feb 2025, Christophe Leroy wrote:
>
> > > Restricted pointers ("%pK") are not meant to be used through printk().
> > > It can unintentionally expose security sensitive, raw pointer values.
> > >
> > > Use regular pointer formatting instead.
> > >
> > > Link:
> > > https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Flore.kernel.org%2Flkml%2F20250113171731-dc10e3c1-da64-4af0-b767-7c7070468023%40linutronix.de%2F&data=05%7C02%7Cchristophe.leroy%40csgroup.eu%7C75a852a0fef54fa43a3608dd4f263f45%7C8b87af7d86474dc78df45f69a2011bb5%7C0%7C0%7C638753747883689862%7CUnknown%7CTWFpbGZsb3d8eyJFbXB0eU1hcGkiOnRydWUsIlYiOiIwLjAuMDAwMCIsIlAiOiJXaW4zMiIsIkFOIjoiTWFpbCIsIldUIjoyfQ%3D%3D%7C0%7C%7C%7C&sdata=aUgq6pXb1ySaQ6e%2FdyM09jfc4MNLE71Njw0%2FnCg%2F6VU%3D&reserved=0
> > > Signed-off-by: Thomas Weißschuh <thomas.weisssc...@linutronix.de>
> >
> > Reviewed-by: Christophe Leroy <christophe.le...@csgroup.eu>
> >
> > > ---
> > > arch/powerpc/kernel/eeh_driver.c | 2 +-
> > > arch/powerpc/perf/hv-24x7.c | 8 ++++----
> > > 2 files changed, 5 insertions(+), 5 deletions(-)
> > >
> > > diff --git a/arch/powerpc/kernel/eeh_driver.c
> > > b/arch/powerpc/kernel/eeh_driver.c
> > > index
> > > 7efe04c68f0fe3fb1c3c13d97d58e79e47cf103b..10ce6b3bd3b7c54f91544ae7f7fd3f32a51ee09a
> > > 100644
> > > --- a/arch/powerpc/kernel/eeh_driver.c
> > > +++ b/arch/powerpc/kernel/eeh_driver.c
> > > @@ -907,7 +907,7 @@ void eeh_handle_normal_event(struct eeh_pe *pe)
> > > /* FIXME: Use the same format as dump_stack() */
> > > pr_err("EEH: Call Trace:\n");
> > > for (i = 0; i < pe->trace_entries; i++)
> > > - pr_err("EEH: [%pK] %pS\n", ptrs[i], ptrs[i]);
> > > + pr_err("EEH: [%p] %pS\n", ptrs[i], ptrs[i]);
> > > pe->trace_entries = 0;
> > > }
>
> But shouldn't this be using `%px' then instead? It would be sad if all
> the address information from error reports such as below:
>
> EEH: Call Trace:
> EEH: [000000008985bc3b] __eeh_send_failure_event+0x78/0x150
> EEH: [000000008c4c5782] eeh_dev_check_failure+0x388/0x6b0
> EEH: [000000001fb766c1] eeh_check_failure+0x98/0x100
> EEH: [000000004b9af8c6] dfx_port_read_long+0xb0/0x130 [defxx]
> EEH: [00000000e23999c1] dfx_interrupt+0x80/0x8c0 [defxx]
> EEH: [00000000c7884fb7] __handle_irq_event_percpu+0x9c/0x2f0
> EEH: [000000008d4e9afd] handle_irq_event_percpu+0x44/0xc0
> EEH: [000000008c39ece4] handle_irq_event+0x74/0xc0
> EEH: [00000000d85114a9] handle_fasteoi_irq+0xd4/0x220
> EEH: [00000000a692ef4e] generic_handle_irq+0x54/0x80
> EEH: [00000000a6db243b] __do_irq+0x68/0x200
> EEH: [0000000040ccff9e] call_do_irq+0x14/0x24
> EEH: [00000000e8e9ddf7] do_IRQ+0x78/0xd0
> EEH: [0000000031916539] replay_soft_interrupts+0x180/0x370
> EEH: [000000001b7e5728] arch_local_irq_restore+0x48/0xc0
> EEH: [00000000088691b7] cpuidle_enter_state+0x108/0x560
> EEH: [00000000e6e26f30] cpuidle_enter+0x50/0x70
> EEH: [000000007c26474c] call_cpuidle+0x4c/0x80
> EEH: [0000000036b8a2fc] do_idle+0x360/0x3b0
> EEH: [0000000048702083] cpu_startup_entry+0x38/0x40
> EEH: [00000000d3b1fb8d] start_secondary+0x62c/0x660
> EEH: [0000000041a9a815] start_secondary_prolog+0x10/0x14
>
> was suddenly lost from the kernel log, the access to which unprivileged
> users can be denied if so desired according to the site policy. Whereas
> running the kernel such as to have all output from plain `%p' exposed just
> to cope with this proposed change, now that seems like a security risk.
Your point makes sense.
*But* the addresses in your example are already hashed,
as indicated by the all-zero upper 32 bits.
By default, when kptr_restrict is set to 0, %pK behaves the same as %p.
The same happened for a bunch of other architectures and nobody seems
to have noticed in the past.
The symbol-relative pointers or pointer formats designed for backtraces,
as notes by Christophe, seem to be enough.
But personally I'm also fine with using %px, as my goal is to remove the
error-prone and confusing %pK.
Thomas
