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

[Cen Zhang]

Hi Jarkko,

Jarkko Sakkinen <jarkko@xxxxxxxxxx> 于2026年10月30日周五 04:20写道：
>
> On Fri, Oct 30, 2026 at 12:07:08AM +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.
> >
> > 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.
> >
> > Take key->sem before the first access to key->type and hold it until
> > the preparsed payload has been freed. Once a key exists, only the GC
> > changes key->type, and it does so with key->sem held for writing.
> >
> > Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
> > Signed-off-by: Cen Zhang <zzzccc427@xxxxxxxxx>
> > ---
> > Changes in v2:
> > - Refine the commit message.
> > - Remove the code comment in key_update().
> > - Drop the Assisted-by tag.
> >
> > Link to v1: https://lore.kernel.org/r/pm-keys-update-type-removal-20261029-v6-3d92b2f9693854230a02@xxxxxxxxx
> >
> >  security/keys/key.c | 11 ++++++-----
> >  1 file changed, 6 insertions(+), 5 deletions(-)
> >
> > diff --git a/security/keys/key.c b/security/keys/key.c
> > index e88548f1a704..994a34a80c29 100644
> > --- a/security/keys/key.c
> > +++ b/security/keys/key.c
> > @@ -1101,9 +1101,12 @@ int key_update(key_ref_t key_ref, const void *payload, size_t plen)
> >       if (ret < 0)
> >               return ret;
> >
> > +     down_write(&key->sem);
> > +
> >       /* attempt to update it if supported */
> > +     ret = -EOPNOTSUPP;
> >       if (!key->type->update)
> > -             return -EOPNOTSUPP;
> > +             goto out;
> >
> >       memset(&prep, 0, sizeof(prep));
> >       prep.data = payload;
> > @@ -1116,8 +1119,6 @@ int key_update(key_ref_t key_ref, const void *payload, size_t plen)
> >                       goto error;
> >       }
> >
> > -     down_write(&key->sem);
> > -
> >       ret = key->type->update(key, &prep);
> >       if (ret == 0) {
> >               /* Updating a negative key positively instantiates it */
> > @@ -1125,11 +1126,11 @@ int key_update(key_ref_t key_ref, const void *payload, size_t plen)
> >               notify_key(key, NOTIFY_KEY_UPDATED, 0);
> >       }
> >
> > -     up_write(&key->sem);
> > -
> >  error:
> >       if (key->type->preparse)
> >               key->type->free_preparse(&prep);
>
> Hmm.. a nitpick but here should be empty line, or at is least common
> to have that before the next label.
>
> > +out:
> > +     up_write(&key->sem);
>
> And also these labels don't have coherency in naming. Not claiming
> there is overall but it would be good to start improving the
> situation.
>
> So I think this would add more clarity:
>
> out_free_preparse:
>         if (key->type->preparse)
>                 key->type->free_preparse(&prep);
>
> out:
>         up_write(&key->sem);
>         return ret;
> }
> EXPORT_SYMBOL(key_update);
>
> And is more aligned with our defacto conventions.
>

Thanks. I'll refine them in v3.

Best regards,
Cen Zhang