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

[Cen Zhang]

Hi Jarkko,

Jarkko Sakkinen <jarkko@xxxxxxxxxx> 于2026年10月18日周日 10:16写道：

>
> On Sun, Oct 18, 2026 at 05:00:14AM +0300, Jarkko Sakkinen wrote:
> > On Sat, Oct 17, 2026 at 03:30:56PM +0800, Cen Zhang wrote:
> > > The key type must remain alive while its read method is checked.
> > > keyctl_read_key() holds a reference to the key, but checks
> > > key->type->read without holding key->sem. The reference keeps the key
> > > alive, but does not pin the module containing its type.
> > >
> > > With AF_RXRPC built as a module, a positive RxRPC key can pass
> > > key_read_state() and the Read permission or possession checks while
> > > another task unloads the module. Unload is possible once the module
> > > refcount reaches zero, e.g. when no RxRPC socket remains open. The
> > > reader can be delayed between loading key->type and reading its read
> > > member, allowing this order:
> > >
> > >   KEYCTL_READ task                  Module unload and key GC
> > >   lookup_user_key()
> > >   key_read_state()
> > >   key_permission() / is_key_possessed()
> > >   load key->type
> > >                                     af_rxrpc_exit()
> > >                                     unregister_key_type()
> > >                                     key_gc_keytype() schedules the GC
> > >                                       pass and waits for it
> > >                                     GC takes key->sem for writing
> > >                                     GC retypes key to key_type_dead
> > >                                     GC releases key->sem
> > >                                     unregister_key_type() returns
> > >                                     free_module() frees type storage
> > >   read saved type's read member
> > >
> > > The last access reads freed module storage and can fault in
> > > keyctl_read_key(). The later read invocation is already protected by
> > > key->sem and key_validate() in __keyctl_read_key(), but neither covers
> > > this earlier check.
> > >
> > > Take key->sem for reading around the read-method check. The garbage
> > > collector retypes the key under a write hold on key->sem, so it must
> > > wait for the check to finish; key_gc_keytype() in turn waits for that
> > > GC pass before unregister_key_type() returns, so the module text cannot
> > > be freed until the check has completed. If retyping happens first, the
> > > check sees key_type_dead, which has no read method. Release the
> > > semaphore before returning -EOPNOTSUPP or proceeding to buffer
> > > allocation, preserving the early unsupported-operation result and
> > > keeping user copies outside the semaphore.
> > >
> > > Oops report as below:
> > >
> > >     BUG: unable to handle page fault for address: fffffbfff8048bc5
> > >     #PF: supervisor read access in kernel mode
> > >     #PF: error_code(0x0000) - not-present page
> > >     RIP: 0010:keyctl_read_key+0x1aa/0x600
> > >     Call Trace:
> > >      __do_sys_keyctl+0x2a7/0x4f0
> > >
> > > Signed-off-by: Cen Zhang <zzzccc427@xxxxxxxxx>
> > > Assisted-by: LLM
> >
> > Would be better to finish off the work unfinished and fix the patches
> > under circulation instead of spamming new ones while at it. I.e. fix
> > them first and don't starvate us with unfinished patches.
> >
> > If this was merge ready this would be acceptable, but in this even the
> > tags above are in the wrong order. This is not a lottery machine where
> > you dump whatever LLM gives you without your attention, so please don't
> > do that.
> >
> > >
> > > ---
> > > diff --git a/security/keys/keyctl.c b/security/keys/keyctl.c
> > > index 5a5efa26ed0e..756368422b7c 100644
> > > --- a/security/keys/keyctl.c
> > > +++ b/security/keys/keyctl.c
> > > @@ -863,10 +863,13 @@ long keyctl_read_key(key_serial_t keyid, char __user *buffer, size_t buflen)
> > >
> > >     /* the key is probably readable - now try to read it */
> > >  can_read_key:
> > > -   if (!key->type->read) {
> > > -           ret = -EOPNOTSUPP;
> > > +   down_read(&key->sem);
> > > +   ret = -EOPNOTSUPP;
> > > +   if (key->type->read)
> > > +           ret = 0;
> > > +   up_read(&key->sem);
> > > +   if (ret)
> > >             goto key_put_out;
> > > -   }
> > >
> > >     if (!buffer || !buflen) {
> > >             /* Get the key length from the read method */
> >
> > Why did you chose this over embedding the check inside __keyctl_read_key()?
> >
> > I'm assuming you have considered both options.
>
> I.e. to reformalize: how does that change guarantee key->type->read
> validity for the __keyctl_read_key() that follow?
>
> My first idea would be put the check inside __keyctl_read_key()
> while key->sem is taken.
>
> Please explain because I'm not fully following with that change.
> I could get something wrong but it is against the assumed most
> common sense fix imaginable.
>
> Br, Jarkko

Sadly, I focused too narrowly on protecting the early dereference
and overlooked the approach you suggested. It does make more
sense.

I've removed the outer check in v2 and moved it into __keyctl_read_key(),
after key_validate() succeeds and while key->sem remains held. The check
and call now run under the same read hold on key->sem, so GC cannot
replace key->type between them.

I've also corrected the trailer order.

Best regards,
Cen Zhang