linux-kernel - Re: [PATCH] powerpc: Don't use %pK through printk

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <20250225091250-eac544ad-4e5b-47f7-83fc-5212c720483a@linutronix.de>
Date: Tue, 25 Feb 2025 09:19:17 +0100
From: Thomas Weißschuh <thomas.weissschuh@...utronix.de>
To: "Maciej W. Rozycki" <macro@...am.me.uk>
Cc: Christophe Leroy <christophe.leroy@...roup.eu>, 
	Mahesh J Salgaonkar <mahesh@...ux.ibm.com>, Oliver O'Halloran <oohall@...il.com>, 
	Madhavan Srinivasan <maddy@...ux.ibm.com>, Michael Ellerman <mpe@...erman.id.au>, 
	Nicholas Piggin <npiggin@...il.com>, Naveen N Rao <naveen@...nel.org>, linuxppc-dev@...ts.ozlabs.org, 
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH] powerpc: Don't use %pK through printk

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.weissschuh@...utronix.de>
> > 
> > Reviewed-by: Christophe Leroy <christophe.leroy@...roup.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