[DISCUSSION] vsprintf: the current state of restricted pointers (%pK)

[DISCUSSION] vsprintf: the current state of restricted pointers (%pK)

[Thomas Weißschuh]

Hi everybody,

as you know, leaking raw kernel pointers to the user is problematic as
they can be used to break KASLR.
Therefore back in 2011 the %pK format specifier was added [0], printing
certain pointers zeroed out or raw depending on the usage context.
Then in 2017 even the default %p format was changed to hash the pointers [1].

Both mechanisms are similar in their intention but have different,
cross-interacting effects and configuration knobs.
The end result is not always obvious. For example:
* "no_hash_pointers" does not work for %pK if kernel.kptr_restrict>=1
* If kernel.kptr_restrict=1, "restricted" pointers are effectively
  less restricted than "normal" pointers.
* For other values of kernel.kptr_restrict %p and %pK have the same
  security properties, but still different string representations.

Additionally the current usage of %pK is incorrect in many cases.
As %pK relies on the current task context for its permission check, it
was only ever meant to be used from procfs/sysfs/debugfs handlers [2].
In reality many callers use it through printk(), leaking addresses
into dmesg. While restricted_pointer() tries to detect some of such
situations at runtime, this check is not and can not be always complete.

File handlers which could use %pK correctly today, often use
kallsyms_show_value() instead. This is similar, but checks explicitly
against the credentials from an opened file instead of the implicit task
credentials. This behavior was the goal for %pK all along [3].

Is it time to inspect the users of %pK and migrate them to either
%p/%px, kallsyms_show_value() or some similar new API?
Then alias %pK to %p, maybe removing it at some point.


A different, but slightly related issue occurs with PREEMPT_RT.
Calling printk("%pK") while holding a raw spinlock will trigger an
invalid wait context and latency spikes if an LSM using sleeping
spinlocks is enabled.
As printk() should be callable from any context this is an issue.
Removing the implicit group check would also avoid this.


Thanks,
Thomas


[0] 455cd5ab305c ("kptr_restrict for hiding kernel pointers from unprivileged users"),
[1] ad67b74d2469 ("printk: hash addresses printed with %p")
[2] Documentation/core-api/printk-formats.rst:
	This modifier is *only* intended when producing content of a file read by
	userspace from e.g. procfs or sysfs, not for dmesg. Please refer to the
	section about %p above for discussion about how to manage hashing pointers
	in printk().
[3] Documentation/admin-guide/sysctl/kernel.rst:
	"The correct long-term solution is to do the permission checks at open() time."
[4] https://lore.kernel.org/lkml/20241217142032.55793-1-acarmina@redhat.com/

Re: [DISCUSSION] vsprintf: the current state of restricted pointers (%pK)

[Andy Shevchenko]

On Mon, Jan 13, 2025 at 05:46:44PM +0100, Thomas Weißschuh wrote:
> Hi everybody,
> 
> as you know, leaking raw kernel pointers to the user is problematic as
> they can be used to break KASLR.
> Therefore back in 2011 the %pK format specifier was added [0], printing
> certain pointers zeroed out or raw depending on the usage context.
> Then in 2017 even the default %p format was changed to hash the pointers [1].
> 
> Both mechanisms are similar in their intention but have different,
> cross-interacting effects and configuration knobs.
> The end result is not always obvious. For example:
> * "no_hash_pointers" does not work for %pK if kernel.kptr_restrict>=1
> * If kernel.kptr_restrict=1, "restricted" pointers are effectively
>   less restricted than "normal" pointers.
> * For other values of kernel.kptr_restrict %p and %pK have the same
>   security properties, but still different string representations.
> 
> Additionally the current usage of %pK is incorrect in many cases.
> As %pK relies on the current task context for its permission check, it
> was only ever meant to be used from procfs/sysfs/debugfs handlers [2].
> In reality many callers use it through printk(), leaking addresses
> into dmesg. While restricted_pointer() tries to detect some of such
> situations at runtime, this check is not and can not be always complete.
> 
> File handlers which could use %pK correctly today, often use
> kallsyms_show_value() instead. This is similar, but checks explicitly
> against the credentials from an opened file instead of the implicit task
> credentials. This behavior was the goal for %pK all along [3].

> Is it time to inspect the users of %pK and migrate them to either
> %p/%px, kallsyms_show_value() or some similar new API?
> Then alias %pK to %p, maybe removing it at some point.

To me this paragraph sounds like a good plan, which I agree on!

> A different, but slightly related issue occurs with PREEMPT_RT.
> Calling printk("%pK") while holding a raw spinlock will trigger an
> invalid wait context and latency spikes if an LSM using sleeping
> spinlocks is enabled.
> As printk() should be callable from any context this is an issue.
> Removing the implicit group check would also avoid this.

> [0] 455cd5ab305c ("kptr_restrict for hiding kernel pointers from unprivileged users"),
> [1] ad67b74d2469 ("printk: hash addresses printed with %p")
> [2] Documentation/core-api/printk-formats.rst:
> 	This modifier is *only* intended when producing content of a file read by
> 	userspace from e.g. procfs or sysfs, not for dmesg. Please refer to the
> 	section about %p above for discussion about how to manage hashing pointers
> 	in printk().
> [3] Documentation/admin-guide/sysctl/kernel.rst:
> 	"The correct long-term solution is to do the permission checks at open() time."
> [4] https://lore.kernel.org/lkml/20241217142032.55793-1-acarmina@redhat.com/

-- 
With Best Regards,
Andy Shevchenko

Re: [DISCUSSION] vsprintf: the current state of restricted pointers (%pK)

[Petr Mladek]

On Tue 2025-01-14 16:35:57, Andy Shevchenko wrote:
> On Mon, Jan 13, 2025 at 05:46:44PM +0100, Thomas Weißschuh wrote:
> > Hi everybody,
> > 
> > as you know, leaking raw kernel pointers to the user is problematic as
> > they can be used to break KASLR.
> > Therefore back in 2011 the %pK format specifier was added [0], printing
> > certain pointers zeroed out or raw depending on the usage context.
> > Then in 2017 even the default %p format was changed to hash the pointers [1].
> > 
> > Both mechanisms are similar in their intention but have different,
> > cross-interacting effects and configuration knobs.
> > The end result is not always obvious. For example:
> > * "no_hash_pointers" does not work for %pK if kernel.kptr_restrict>=1
> > * If kernel.kptr_restrict=1, "restricted" pointers are effectively
> >   less restricted than "normal" pointers.
> > * For other values of kernel.kptr_restrict %p and %pK have the same
> >   security properties, but still different string representations.
> > 
> > Additionally the current usage of %pK is incorrect in many cases.
> > As %pK relies on the current task context for its permission check, it
> > was only ever meant to be used from procfs/sysfs/debugfs handlers [2].
> > In reality many callers use it through printk(), leaking addresses
> > into dmesg. While restricted_pointer() tries to detect some of such
> > situations at runtime, this check is not and can not be always complete.
> > 
> > File handlers which could use %pK correctly today, often use
> > kallsyms_show_value() instead. This is similar, but checks explicitly
> > against the credentials from an opened file instead of the implicit task
> > credentials. This behavior was the goal for %pK all along [3].
> 
> > Is it time to inspect the users of %pK and migrate them to either
> > %p/%px, kallsyms_show_value() or some similar new API?
> > Then alias %pK to %p, maybe removing it at some point.
> 
> To me this paragraph sounds like a good plan, which I agree on!

+1

> > A different, but slightly related issue occurs with PREEMPT_RT.
> > Calling printk("%pK") while holding a raw spinlock will trigger an
> > invalid wait context and latency spikes if an LSM using sleeping
> > spinlocks is enabled.
> > As printk() should be callable from any context this is an issue.
> > Removing the implicit group check would also avoid this.

Good to know.

Best Regards,
Petr