[PATCH] powerpc: Don't use %pK through printk

[PATCH] powerpc: Don't use %pK through printk

[Thomas Weißschuh]

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://lore.kernel.org/lkml/20250113171731-dc10e3c1-da64-4af0-b767-7c7070468023@linutronix.de/
Signed-off-by: Thomas Weißschuh <thomas.weissschuh@linutronix.de>
---
 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;
 	}
diff --git a/arch/powerpc/perf/hv-24x7.c b/arch/powerpc/perf/hv-24x7.c
index d400fa391c2765cf201ee4dc754007e655cc74ca..f6d734431b1dcdfec3b9205c3b48577b4fc26b53 100644
--- a/arch/powerpc/perf/hv-24x7.c
+++ b/arch/powerpc/perf/hv-24x7.c
@@ -713,12 +713,12 @@ static ssize_t catalog_event_len_validate(struct hv_24x7_event_data *event,
 	ev_len = be16_to_cpu(event->length);
 
 	if (ev_len % 16)
-		pr_info("event %zu has length %zu not divisible by 16: event=%pK\n",
+		pr_info("event %zu has length %zu not divisible by 16: event=%p\n",
 				event_idx, ev_len, event);
 
 	ev_end = (__u8 *)event + ev_len;
 	if (ev_end > end) {
-		pr_warn("event %zu has .length=%zu, ends after buffer end: ev_end=%pK > end=%pK, offset=%zu\n",
+		pr_warn("event %zu has .length=%zu, ends after buffer end: ev_end=%p > end=%p, offset=%zu\n",
 				event_idx, ev_len, ev_end, end,
 				offset);
 		return -1;
@@ -726,14 +726,14 @@ static ssize_t catalog_event_len_validate(struct hv_24x7_event_data *event,
 
 	calc_ev_end = event_end(event, end);
 	if (!calc_ev_end) {
-		pr_warn("event %zu has a calculated length which exceeds buffer length %zu: event=%pK end=%pK, offset=%zu\n",
+		pr_warn("event %zu has a calculated length which exceeds buffer length %zu: event=%p end=%p, offset=%zu\n",
 			event_idx, event_data_bytes, event, end,
 			offset);
 		return -1;
 	}
 
 	if (calc_ev_end > ev_end) {
-		pr_warn("event %zu exceeds its own length: event=%pK, end=%pK, offset=%zu, calc_ev_end=%pK\n",
+		pr_warn("event %zu exceeds its own length: event=%p, end=%p, offset=%zu, calc_ev_end=%p\n",
 			event_idx, event, ev_end, offset, calc_ev_end);
 		return -1;
 	}

---
base-commit: 0ad2507d5d93f39619fc42372c347d6006b64319
change-id: 20250217-restricted-pointers-powerpc-f11876496464

Best regards,
-- 
Thomas Weißschuh <thomas.weissschuh@linutronix.de>

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

[Christophe Leroy]

Le 17/02/2025 à 08:39, Thomas Weißschuh a écrit :
> 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@linutronix.de>

Reviewed-by: Christophe Leroy <christophe.leroy@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;
>   	}
> diff --git a/arch/powerpc/perf/hv-24x7.c b/arch/powerpc/perf/hv-24x7.c
> index d400fa391c2765cf201ee4dc754007e655cc74ca..f6d734431b1dcdfec3b9205c3b48577b4fc26b53 100644
> --- a/arch/powerpc/perf/hv-24x7.c
> +++ b/arch/powerpc/perf/hv-24x7.c
> @@ -713,12 +713,12 @@ static ssize_t catalog_event_len_validate(struct hv_24x7_event_data *event,
>   	ev_len = be16_to_cpu(event->length);
>   
>   	if (ev_len % 16)
> -		pr_info("event %zu has length %zu not divisible by 16: event=%pK\n",
> +		pr_info("event %zu has length %zu not divisible by 16: event=%p\n",
>   				event_idx, ev_len, event);
>   
>   	ev_end = (__u8 *)event + ev_len;
>   	if (ev_end > end) {
> -		pr_warn("event %zu has .length=%zu, ends after buffer end: ev_end=%pK > end=%pK, offset=%zu\n",
> +		pr_warn("event %zu has .length=%zu, ends after buffer end: ev_end=%p > end=%p, offset=%zu\n",
>   				event_idx, ev_len, ev_end, end,
>   				offset);
>   		return -1;
> @@ -726,14 +726,14 @@ static ssize_t catalog_event_len_validate(struct hv_24x7_event_data *event,
>   
>   	calc_ev_end = event_end(event, end);
>   	if (!calc_ev_end) {
> -		pr_warn("event %zu has a calculated length which exceeds buffer length %zu: event=%pK end=%pK, offset=%zu\n",
> +		pr_warn("event %zu has a calculated length which exceeds buffer length %zu: event=%p end=%p, offset=%zu\n",
>   			event_idx, event_data_bytes, event, end,
>   			offset);
>   		return -1;
>   	}
>   
>   	if (calc_ev_end > ev_end) {
> -		pr_warn("event %zu exceeds its own length: event=%pK, end=%pK, offset=%zu, calc_ev_end=%pK\n",
> +		pr_warn("event %zu exceeds its own length: event=%p, end=%p, offset=%zu, calc_ev_end=%p\n",
>   			event_idx, event, ev_end, offset, calc_ev_end);
>   		return -1;
>   	}
> 
> ---
> base-commit: 0ad2507d5d93f39619fc42372c347d6006b64319
> change-id: 20250217-restricted-pointers-powerpc-f11876496464
> 
> Best regards,

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

[Maciej W. Rozycki]

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

  Maciej

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

[Christophe Leroy]

Le 24/02/2025 à 19:54, Maciej W. Rozycki a écrit :
> ***ATTENTION, Sopra Steria Group cannot confirm the identity of this email sender (SPF record failure). This might be a fake email from an attacker, if you have any doubts report and delete the email.***
> 
> ***ATTENTION, Sopra Steria Group ne peut pas confirmer l’identité de l’émetteur de ce message (SPF record failure). Il pourrait s’agir d’un faux message, à détruire si vous avez un doute ***
> 
> 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%7C9079ef2ec60e4717ec8e08dd5504b718%7C8b87af7d86474dc78df45f69a2011bb5%7C0%7C0%7C638760200949886583%7CUnknown%7CTWFpbGZsb3d8eyJFbXB0eU1hcGkiOnRydWUsIlYiOiIwLjAuMDAwMCIsIlAiOiJXaW4zMiIsIkFOIjoiTWFpbCIsIldUIjoyfQ%3D%3D%7C0%7C%7C%7C&sdata=d2QCtnssTlVmKOKR57rui%2Fq73UsAAoZrim%2FABaz5IFs%3D&reserved=0
>>> Signed-off-by: Thomas Weißschuh <thomas.weissschuh@linutronix.de>
>>
>> Reviewed-by: Christophe Leroy <christophe.leroy@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.

The purpose of hashed addresses is to avoid kernel addresses to leak to 
the kernel log. Here you have function names, if you get real function 
addresses at the same time, then you know everything about kernel 
addresses and for instance KASLR becomes just pointless.

By the way, why do you need the addresses at all in addition to function 
names ? When I look at x86 dump stack, they only print function name, 
using %pBb

Christophe

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

[Thomas Weißschuh]

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

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

[Maciej W. Rozycki]

On Tue, 25 Feb 2025, Christophe Leroy wrote:

> > 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.
> 
> The purpose of hashed addresses is to avoid kernel addresses to leak to the
> kernel log. Here you have function names, if you get real function addresses
> at the same time, then you know everything about kernel addresses and for
> instance KASLR becomes just pointless.

 Why is it so important not to have kernel addresses in the kernel log?  
This defeats the purpose of having diagnostics for such fatal events.

 If your site policy so requires, you can disable access to the kernel log 
for unprivileged users, in which case once you do have one you can poke at 
/dev/mem too and have a thousand other ways to do harm.  And if you are a 
sloppy admin and have /var/log/messages world-readable where you really 
ought not to, then well, what can I say?

 From the description of `%pK' I infer it's been meant for /proc files and 
the like that may be readable to unprivileged users, and I can certainly
understand the security implications here.

> By the way, why do you need the addresses at all in addition to function names
> ? When I look at x86 dump stack, they only print function name, using %pBb

 They can be handed over to GDB, `objdump' and similar debug tools when 
working with `vmlinux'.  Function names do help, giving you assistance to 
make sure you work with matching `vmlinux'.

 NB I don't know what x86 does, I've done little in that area in the 
recent ~20 years, mostly taking care about my legacy 32-bit i486/i586 
stuff.

  Maciej

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

[Maciej W. Rozycki]

On Tue, 25 Feb 2025, Thomas Weißschuh wrote:

> > 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.

 Darn it!

> 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.

 I do hope so.

> But personally I'm also fine with using %px, as my goal is to remove the
> error-prone and confusing %pK.

 It's clear that `%pK' was meant to restrict access to /proc files and the 
like that may be accessible by unprivileged users:

"
kptr_restrict
=============

This toggle indicates whether restrictions are placed on
exposing kernel addresses via ``/proc`` and other interfaces.
"

and not the kernel log, the information in which may come from rare events 
that are difficult to trigger and hard to recover via other means.  Sigh. 
Once you've got access to the kernel log, you may as well wipe the system 
or do any other harm you might like.

  Maciej

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

[Thomas Weißschuh]

On Tue, Feb 25, 2025 at 05:32:12PM +0000, Maciej W. Rozycki wrote:
> On Tue, 25 Feb 2025, Thomas Weißschuh wrote:
> 
> > > 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.
> 
>  Darn it!

Agreed.

> > 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.
> 
>  I do hope so.

As mentioned before, personally I am fine with using %px here.
The values are in the register dumps anyways and security sensitive deployments
will panic on WARN(), making the information disclosure useless.

> > But personally I'm also fine with using %px, as my goal is to remove the
> > error-prone and confusing %pK.
> 
>  It's clear that `%pK' was meant to restrict access to /proc files and the 
> like that may be accessible by unprivileged users:

Then let's stop abusing it. For something that is clear, it is
misunderstood very often.

> "
> kptr_restrict
> =============
> 
> This toggle indicates whether restrictions are placed on
> exposing kernel addresses via ``/proc`` and other interfaces.
> "
> 
> and not the kernel log, the information in which may come from rare events 
> that are difficult to trigger and hard to recover via other means.  Sigh. 
> Once you've got access to the kernel log, you may as well wipe the system 
> or do any other harm you might like.

As I understand it, both the security and printk maintainers don't want the
kernel log in general to be security sensitive and restricted.
My goal here is not to push site-specific policy into the kernel but make life
easier for kernel developers by removing the confusing and error-prone %pK
altogether.
Security is only one aspect.


Thomas

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

[Maciej W. Rozycki]

On Wed, 26 Feb 2025, Thomas Weißschuh wrote:

> > > 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.
> > 
> >  I do hope so.
> 
> As mentioned before, personally I am fine with using %px here.

 Glad to hear!

> The values are in the register dumps anyways and security sensitive deployments
> will panic on WARN(), making the information disclosure useless.

 And even more so, I wasn't aware of this feature.  But this code doesn't 
make use of the WARN() facility, it just prints at the heightened KERN_ERR 
priority.

> > > But personally I'm also fine with using %px, as my goal is to remove the
> > > error-prone and confusing %pK.
> > 
> >  It's clear that `%pK' was meant to restrict access to /proc files and the 
> > like that may be accessible by unprivileged users:
> 
> Then let's stop abusing it. For something that is clear, it is
> misunderstood very often.

 Absolutely, I haven't questioned the removal of `%pK', but the switch to 
`%p' rather than `%px' specifically for this single hunk of your patch.

> > "
> > kptr_restrict
> > =============
> > 
> > This toggle indicates whether restrictions are placed on
> > exposing kernel addresses via ``/proc`` and other interfaces.
> > "
> > 
> > and not the kernel log, the information in which may come from rare events 
> > that are difficult to trigger and hard to recover via other means.  Sigh. 
> > Once you've got access to the kernel log, you may as well wipe the system 
> > or do any other harm you might like.
> 
> As I understand it, both the security and printk maintainers don't want the
> kernel log in general to be security sensitive and restricted.
> My goal here is not to push site-specific policy into the kernel but make life
> easier for kernel developers by removing the confusing and error-prone %pK
> altogether.

 Let me ask a different question then: is your approach to bulk-switch all 
instances of `%pK' to `%p' as the safe default and let other people figure 
out afterwards whether a different conversion specifier ought to be used 
instead on a case-by-case basis and then follow up with another patch, or 
will you consider these alternatives right away?

> Security is only one aspect.

 I think it's important enough though for us to ensure we don't compromise 
it by chance.

  Maciej

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

[Thomas Weißschuh]

On Fri, Feb 28, 2025 at 08:15:02PM +0000, Maciej W. Rozycki wrote:
> On Wed, 26 Feb 2025, Thomas Weißschuh wrote:
> 
> > > > 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.
> > > 
> > >  I do hope so.
> > 
> > As mentioned before, personally I am fine with using %px here.
> 
>  Glad to hear!
> 
> > The values are in the register dumps anyways and security sensitive deployments
> > will panic on WARN(), making the information disclosure useless.
> 
>  And even more so, I wasn't aware of this feature.  But this code doesn't 
> make use of the WARN() facility, it just prints at the heightened KERN_ERR 
> priority.

Indeed, I got confused with some other patches where WARN() is used mostly.
This makes it a bit murkier.

> > > > But personally I'm also fine with using %px, as my goal is to remove the
> > > > error-prone and confusing %pK.
> > > 
> > >  It's clear that `%pK' was meant to restrict access to /proc files and the 
> > > like that may be accessible by unprivileged users:
> > 
> > Then let's stop abusing it. For something that is clear, it is
> > misunderstood very often.
> 
>  Absolutely, I haven't questioned the removal of `%pK', but the switch to 
> `%p' rather than `%px' specifically for this single hunk of your patch.

Sure. It would be great if one of the maintainers could confirm this preference.

> > > "
> > > kptr_restrict
> > > =============
> > > 
> > > This toggle indicates whether restrictions are placed on
> > > exposing kernel addresses via ``/proc`` and other interfaces.
> > > "
> > > 
> > > and not the kernel log, the information in which may come from rare events 
> > > that are difficult to trigger and hard to recover via other means.  Sigh. 
> > > Once you've got access to the kernel log, you may as well wipe the system 
> > > or do any other harm you might like.
> > 
> > As I understand it, both the security and printk maintainers don't want the
> > kernel log in general to be security sensitive and restricted.
> > My goal here is not to push site-specific policy into the kernel but make life
> > easier for kernel developers by removing the confusing and error-prone %pK
> > altogether.
> 
>  Let me ask a different question then: is your approach to bulk-switch all 
> instances of `%pK' to `%p' as the safe default and let other people figure 
> out afterwards whether a different conversion specifier ought to be used 
> instead on a case-by-case basis and then follow up with another patch, or 
> will you consider these alternatives right away?

I am considering on a case-by-case basis. But mostly the decision is that %p is
enough, because by default %pK has been the same as %p anyways.
Also the current wave of replacements does not touch valid users of %pK.
They will stay and later be replaced with a new and better API.

> > Security is only one aspect.
> 
>  I think it's important enough though for us to ensure we don't compromise 
> it by chance.

Agreed.