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

[David Howells]

Jarkko Sakkinen <jarkko@xxxxxxxxxx> wrote:

> On Mon, Oct 19, 2026 at 10:57:21PM +0800, Cen Zhang wrote:
> > keyctl_read_key() checks key->type->read without holding key->sem.
> > A reference keeps the key alive, but does not pin the module containing
> > its type. A surviving RxRPC key can therefore be read while AF_RXRPC
> > is being unloaded:
> > 
> >   KEYCTL_READ task                    Module unload and key GC
> >   ----------------                   ------------------------
> >   lookup_user_key()
> >   permission / possession checks
> >   load key->type
> >                                      af_rxrpc_exit()
> >                                        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 read member
> > 
> > The final access can read freed module storage. The read invocation in
> > __keyctl_read_key() is already protected by key->sem, but the earlier
> > method check is outside that critical section.
> > 
> > Move the method check into __keyctl_read_key(), after key_validate() and
> > under its existing read hold on key->sem. The check and invocation then
> > use the same protected type for length queries and every read retry.
> > The GC cannot retype the key until the semaphore is released, and type
> > unregistration waits for that GC pass before the module can be freed.
> > 
> > Return -EOPNOTSUPP when validation succeeds and the type has no read
> > method. A temporary buffer may now be allocated before this result;
> > allocation and copying to userspace remain outside the semaphore.
> > 
> > Suggested-by: Jarkko Sakkinen <jarkko@xxxxxxxxxx>
> > Assisted-by: LLM
> > Signed-off-by: Cen Zhang <zzzccc427@xxxxxxxxx>
> 
> I literally described the fix to the reported bug instead of reviewing a
> bug fix. That means by practical means that you only reported a bug.
> This cannot happen again and if it does, next time I'll just do the
> right fix myself and commit to the Git because that matches the reality
> the best.
> 
> 
> Reviewed-by: Jarkko Sakkinen <jarkko@xxxxxxxxxx>

Reviewed-by: David Howells <dhowells@xxxxxxxxxx>