Re: [PATCH v2] keys: Fix key_user use-after-free during ownership changes

[Jarkko Sakkinen <jarkko@kernel.org>]

On Mon, Sep 28, 2026 at 01:19:44AM +0800, Chengfeng Ye wrote:
> On Thu, Sep 10, 2026 at 6:44 AM Jarkko Sakkinen <jarkko@kernel.org> wrote:
> >
> > On Fri, Sep 04, 2026 at 04:09:40PM +0800, Chengfeng Ye wrote:
> > > keyctl_chown_key() replaces key->user and then drops the key's
> > > reference to the previous key_user. Several paths access key->user
> > > without any common synchronization with this replacement, including
> > > the /proc/keys iterators, find_keyring_by_name(), quota accounting, and
> > > key instantiation.
> > >
> > > This allows an ownership change to free the previous key_user while a
> > > reader is still accessing it:
> > >
> > >   CPU 0 (/proc/keys)            CPU 1 (KEYCTL_CHOWN)
> > >   user = key->user
> > >                                 old = key->user
> > >                                 key->user = newowner
> > >                                 key_user_put(old)
> > >                                   kfree(old)
> > >   uid = user->uid
> > >
> > > The same missing synchronization also can corrupt instantiated-key
> > > accounting. KEY_LOOKUP_PARTIAL allows keyctl_chown_key() to change the
> > > owner of a key while it is still being instantiated:
> > >
> > >   CPU 0 (instantiate)           CPU 1 (KEYCTL_CHOWN)
> > >   atomic_inc(old->nikeys)
> > >                                 observe KEY_IS_UNINSTANTIATED
> > >                                 skip the nikeys transfer
> > >                                 key->user = newowner
> > >   mark key instantiated
> > >
> > > The increment remains charged to the previous owner. Quota reservation
> > > can similarly select one owner for its quota limit and another owner
> > > for the usage update, or access an owner that has already been freed.
> > >
> > > The existing locks do not provide common exclusion for these
> > > operations. keyctl_chown_key() holds key->sem, key construction uses
> > > key_construction_mutex, and the affected readers hold their respective
> > > tree or list locks.
> > >
> > > Use key_user_lock to serialize access to key ownership. Hold it across
> > > the accounting transfer and key->user replacement in keyctl_chown_key().
> > > Use the same lock while readers copy the owner's UID, while quota usage
> > > is updated, and while the instantiated-key count and key state are
> > > committed.
> > >
> > > key_user_lock already serializes final key_user removal, so an old
> > > owner cannot be freed while one of these readers is accessing it.
> > >
> > > Fixes: 5801649d8b83 ("[PATCH] keys: let keyctl_chown() change a key's owner")
> > > Signed-off-by: Chengfeng Ye <nicoyip.dev@gmail.com>
> > > ---
> > > Changes in v2:
> > > - Audit every user->uid occurrence and all direct key->user accesses.
> > > - Take key_user_lock explicitly at each namespace-filtering reader instead
> > >   of hiding the lock acquisition in an accessor.
> > > - Serialize quota reservation and construction accounting with ownership
> > >   changes while preserving the existing direct key->user accesses.
> > > - Use the commit that introduced ownership changes as the Fixes target.
> > >
> > > Link: https://lore.kernel.org/keyrings/20260823170448.3856516-1-nicoyip.dev@gmail.com/ [v1]
> >
> > https://www.kernel.org/doc/html/latest/process/submitting-patches.html#separate-your-changes
> >
> > I.e. no bundle commits. This unreviewable.
> 
> Thanks for the review. I have split the changes into two patches
> and sent them as v3:
> 
> https://lists.openwall.net/linux-kernel/2026/09/27/734

Thank you, it is just what SubmittingPatches says about commits :-)

> 
> Best regards,
> Chengfeng

Br, Jarkko