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@xxxxxxxxxx/

-- 
With Best Regards,
Andy Shevchenko