Re: [PATCH] lib/vsprintf: Validate sleepable context during restrictred pointer formatting

[Thomas Weißschuh]

On Wed, Mar 18, 2026 at 02:59:36PM +0100, Sebastian Andrzej Siewior wrote:
> On 2026-03-18 12:03:39 [+0100], Petr Mladek wrote:
> > Now, really with John and Sebastian in Cc.
> > 
> > On Wed 2026-03-18 09:48:06, Thomas Weißschuh wrote:
> > > On Tue, Mar 17, 2026 at 12:41:23PM +0100, Thomas Weißschuh wrote:
> > > > Depending on the system configuration, the restricted pointer formatting
> > > > might call into the security subsystem which might sleep.
> > > > As %pK is intended to be only used from read handlers of virtual files,
> > > > which always run in task context, this should never happen in practice.
> > > > However, developers have used %pK before from atomic context without
> > > > realizing this restriction. While all existing user of %pK through
> > > > printk() have been removed, new ones might be reintroduced accidentally
> > > > in the future.
> > > > 
> > > > Add a might_sleep(), so that misuse of %pK from atomic context is
> > > > detected right away.
> > > > 
> > > > Link: https://lore.kernel.org/lkml/20250113171731-dc10e3c1-da64-4af0-b767-7c7070468023@xxxxxxxxxxxxx/
> > > > Link: https://lore.kernel.org/lkml/20241217142032.55793-1-acarmina@xxxxxxxxxx/
> > > > Signed-off-by: Thomas Weißschuh <thomas.weissschuh@xxxxxxxxxxxxx>
> > > > ---
> > > > This depends on commit 5886cc8f895b ("drm/msm/dpu: Don't use %pK through
> > > > printk (again)"), which was merged in v7.0-rc2.
> > > > ---
> > > >  lib/vsprintf.c | 3 +++
> > > >  1 file changed, 3 insertions(+)
> > > > 
> > > > diff --git a/lib/vsprintf.c b/lib/vsprintf.c
> > > > index 800b8ac49f53..eb9dbb28fb9b 100644
> > > > --- a/lib/vsprintf.c
> > > > +++ b/lib/vsprintf.c
> > > > @@ -862,6 +862,9 @@ static noinline_for_stack
> > > >  char *restricted_pointer(char *buf, char *end, const void *ptr,
> > > >  			 struct printf_spec spec)
> > > >  {
> > > > +	/* Only usable from task context, The call to has_capability_noaudit() might sleep. */
> > > > +	might_sleep();
> > > > +
> > > 
> > > So might_sleep() is not actually the right thing to do here.
> > > Some callers use %pK under a spinlock, which fails the might_sleep() check.
> > > However this is fine to do, as has_capability_noaudit() also only takes a
> > > spinlock.
> > 
> > My understanding is that we want to simulate that we are going
> > to call a normal spin_lock() which might sleep in PREEMPT_RT.
> > So that we trigger the warning when %pK is used under
> > a raw_spin_lock.
> > 
> > It might likely be achieved by adding a "fake" spin_lock and might_lock().
> > I am not sure if there is an easier way by using just
> > a struct lockdep_map.
> > 
> > I am always a bit confused by the lockdep annotation.
> 
> might_sleep() seems reasonable if any usage under spinlock_t can be
> excluded.

Unfortunately it can't be excluded. See net/unix/af_unix.c, which uses
unix_state_lock(), a spinlock, around printk(%pK).

> If the usage is intended to happen only from task context then
> in_task() would make sense. This would already ban any softirq context
> and everything else I know we had in the past.

%pK only makes sense from task context. There should also be an assertion
if this is violated independently of the value of kptr_restrict.

Also in_task() looks like the better replacement for this check in
restricted_pointer():

	if (in_hardirq() || in_serving_softirq() || in_nmi())

> The fake spinlock_t could be achieved by just something like
> might_lock() which would avoid any overhead of real locking.

might_lock() simulates and acquisition and immediate release of the lock.
With an alternative solution which holds the fake lock for the whole
execution of restricted_pointer() we could make sure that lockdep
can detect any incompatible locks being taken in the callees, even if
the actual caller of restrict_pointer() is not currently holding any
spinlocks which would trigger such a warning.
I'm happy about opinions whether this is useful.

> But do we still need %pK? There are far more %p users in seq_printf()
> context as per
> 	git grep -E 'seq_printf.*%p\>'

That would probably be a question for the networking folks who are using it.
Getting rid of it would certainly make various things easier.

> than %pK. According to commit 312b4e226951f ("vsprintf: check real
> user/group id for %pK") it is temporary as of 2013. There should be a
> check at open time instead.

Doing the check at open time requires some place to store the result
of it. The users of %pK I looked at all reused their core datastructures
as state in seq_file, so finding space for the result from open time
would require some non-trivial changes to them.

> > > 
> > > >  	switch (kptr_restrict) {
> > > >  	case 0:
> > > >  		/* Handle as %p, hash and do _not_ leak addresses. */


Thomas