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

[Chengfeng Ye]

On Thu, Sep 10, 2026 at 6:44 AM Jarkko Sakkinen <jarkko@xxxxxxxxxx> 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@xxxxxxxxx>
> > ---
> > 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@xxxxxxxxx/ [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

Best regards,
Chengfeng