Re: [PATCH v4 1/2] keys: Protect quota-owner reads in namespace filters

[Jarkko Sakkinen <jarkko@kernel.org>]

On Thu, Oct 15, 2026 at 01:52:04AM +0800, Chengfeng Ye wrote:
> Dear Jarkko,
> 
> On Wed, Oct 14, 2026 at 7:34 PM Jarkko Sakkinen <jarkko@kernel.org> wrote:
> >
> > On Tue, Oct 13, 2026 at 05:03:57PM +0800, Chengfeng Ye wrote:
> > > keyctl_chown_key() replaces key->user under key->sem and drops the
> > > reference to the previous owner after releasing the semaphore. Readers
> > > which do not hold that semaphore can still be using the previous owner
> > > when key_user_put() frees it.
> > >
> > > For example, namespace filtering in /proc/keys can race with chown:
> > >
> > >   /proc/keys reader                 keyctl_chown_key()
> > >   user = key->user
> > >                                     key->user = newowner
> > >                                     key_user_put(old)
> > >                                       kfree(old)
> > >   read user->uid
> > >
> > > An earlier instrumented run reported:
> > >
> > >   BUG: KASAN: slab-use-after-free in proc_keys_start+0x353/0x440
> > >   Read of size 4 at addr ffff8881128abbc0 by task poc/88
> > >   Call Trace:
> > >    proc_keys_start+0x353/0x440
> > >    seq_read_iter+0x25d/0x1190
> > >    proc_reg_read_iter+0x19e/0x260
> > >   Allocated by task 86:
> > >    key_user_lookup+0x1b4/0x540
> > >    keyctl_chown_key+0x3cc/0xbf0
> > >   Freed by task 87:
> > >    kfree+0x149/0x330
> > >    keyctl_chown_key+0x7b0/0xbf0
> >
> > Last night I missed the text below.
> >
> > It is somewhat messy to the point that frankly I could not read it, or
> > at least I did not understand it. Have you read it yourself?
> >
> > There is a whole eassay and I can only see that key->user can change
> > unless key_user_lock is taken, and READ_ONCE() is required to pair with
> > the writers.
> 
> That READ_ONCE() should be unnecessary, since key_user_lock now
> serializes both readers and writers. I will double-check whether to
> drop it.
> 
> > I don't understand what is going on down there, and what's your goal
> > with it?
> 
> Yes, the messy commit message should be cleaned up before sending...
> Sorry for that. Basically, it describes why the three `key->user` use
> sites should be protected by the lock (because there's no
> synchronization) and why the remaining access at other sites doesn't
> need it (because other caller sites are synchronized). I will rephrase
> the commit message to make it human-readable.

Maybe in the case of these bugs do this:

1. From both: drop the tail.
2. Just think yourself is there a remark that is good to keep
   in my. Put those. Not more.
3. Put exactly what makes sense to you, not for me. Don't
   put if you don't want to.

E.g., maybe in the case of this patch it could be that you just
take the head and add remark about READ_ONCE(). Perhaps did not
think whole a lot.

Now it feels maybe that the tail is some kind of "just in case
list of things". 

Hope these instructions help you out. In this case I don't
believe in zero effort, or actually I believe that you do
check up stuff properly it shows from the first half of
the commit messages and from your responses.

Just wanted to state that explicitly so that it is clear.  Commit
messages are in this case exactly the glitch. Otherwise, I don't have
any issues, fears or doubts.

Br, Jarkko