Re: [PATCH] keys: Protect the update method check against type removal

[Cen Zhang]

Hi Jarkko,

Jarkko Sakkinen <jarkko@xxxxxxxxxx> 于2026年10月29日周四 21:14写道：
>
> On Thu, Oct 29, 2026 at 07:58:06PM +0800, Cen Zhang wrote:
> > key_update() reads key->type->update, ->def_datalen and ->preparse
> > before it takes key->sem. A reference on the key does not pin the
> > module that provides its type, so KEYCTL_UPDATE can race with the
> > unloading of that module, for example dns_resolver:
> >
> >   KEYCTL_UPDATE task                  Module unload and key GC
> >   ------------------                  ------------------------
> >   keyctl_update_key()
> >     key_update()
> >       key_permission()
> >       load key->type
> >                                       exit_dns_resolver()
> >                                         unregister_key_type()
> >                                           GC: down_write(&key->sem)
> >                                           GC: retype to key_type_dead
> >                                           GC: up_write(&key->sem)
> >                                         unregister_key_type() returns
> >                                       free_module()
> >       read saved type's update member
> >
> > The final access reads freed module memory. With the updating task
> > held after the key->type load, it faults:
> >
> >   BUG: unable to handle page fault for address: fffffbfff804107f
> >   #PF: supervisor read access in kernel mode
> >   RIP: 0010:key_update+0x12d/0x550
> >   Call Trace:
> >    keyctl_update_key+0x6b/0x110
> >    __do_sys_keyctl+0x21d/0x4f0
> >
> > This is the same problem that commit 925c84e3cc21 ("keys: Protect the
> > read method check against type removal") fixed for KEYCTL_READ.
>
> This is useful bit, thanks.

Thanks for the review.

>
> >
> > The encrypted and trusted types can be built as modules and have an
> > update method. For them there is a second window: the GC can move the
>
> How is the second window related to the fact that encrypted and trusted
> can be built as LKMs and where is "there"?
>
> > key to key_type_dead after the method check and before key_update()
> > takes key->sem. key_update() then calls the NULL update method of
> > key_type_dead.
>
> I don't understand this paragrap overall. Needs a rewrite.

Sorry for that. Perhaps this wording would be better:

For dns_resolver, which has no update method, key_update() returns at
the check, so doing the check with key->sem held would be enough. A
key type that is built as a module and has an update method, like
encrypted or trusted, gets past the check. If its module is unloaded
after the check and before key_update() takes key->sem, the GC has
already moved the key to key_type_dead, and key_update() calls the
update method of key_type_dead, which is NULL. So key->sem has to stay
held from the check until the update is done.

> >
> > Assisted-by: LLM
>
> Please run checkpatch.pl --strict for the next version.

Thanks, I see that the Assisted-by tag has been removed from the
documentation and checkpatch. I will drop it in v2.

>
> > Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
> > Signed-off-by: Cen Zhang <zzzccc427@xxxxxxxxx>
> > ---
> >  security/keys/key.c | 12 +++++++-----
> >  1 file changed, 7 insertions(+), 5 deletions(-)
> >
> > diff --git a/security/keys/key.c b/security/keys/key.c
> > index e88548f1a704..bac2333089fc 100644
> > --- a/security/keys/key.c
> > +++ b/security/keys/key.c
> > @@ -1101,9 +1101,13 @@ int key_update(key_ref_t key_ref, const void *payload, size_t plen)
> >       if (ret < 0)
> >               return ret;
> >
> > +     /* the GC cannot change the key's type while we hold the semaphore */
>
> I don't understand this.

I will remove it in v2.

Best regards,
Cen Zhang