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

[Sebastian Andrzej Siewior]

On 2026-03-18 15:45:06 [+0100], Thomas Weißschuh wrote:
> > 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).

So if the security code can't use a mutex_t internally as locking then
we can't do might_sleep() then.

> > 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())

exactly.

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

might_lock() just grabs and releases it. You could acquire it at the top
and drop it at the end. In order to simulate a lock+unlock behaviour
without an actual lock.

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

Lets see what Kees says, then maybe poke the netdev unless Kees explains
why this need to remain.

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

You could go for the easy solution and return early with -EACCES or so.
That was the intention according to my understanding.

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

Sebastian