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

[Jarkko Sakkinen <jarkko@kernel.org>]

On Thu, Oct 29, 2026 at 10:04:13PM +0800, Cen Zhang wrote:
> Hi Jarkko,
> 
> Jarkko Sakkinen <jarkko@kernel.org> 于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.

Yeah, overall I think this is improving and that's why I'm nitpicking
stuff. What I would wish that if you could hold with any other fixes up
until we've got the previous fully addressed.

I mean submitting a fix trial that is not correct is not a problem
or wrong for that matter. The problem is that if you put stuff out in
volumes with patches in various phases, the odds that fix will land
to the mainline tree rise.

This is exactly what I got pissed of last time. Just wanted to this
clear this one up. Also, you can use that on-hold time to re-check
your unsubmitted patches one more time i.e., do your job better
;-) At leas for me that is something that I want to continuously
improve.

Br, Jarkko