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

[Jarkko Sakkinen <jarkko@kernel.org>]

On Mon, Oct 19, 2026 at 10:50:12PM +0800, Cen Zhang wrote:
> Hi Jarkko,
> 
> Jarkko Sakkinen <jarkko@kernel.org> 于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@gmail.com>
> > > > 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.

Please don't frame ignorance as a problem with focus because this is
ignorance. And there is no "more" here as your fix was plain dead
wrong. You cannot compare a working fix and a code change that
does nothing useful. For me this appears as proxying LLM output
more than anything else. If doing right thing right is a problem
for you, stop sending kernel patches.

I hope this changes the course.

Add my suggested-by tag.

> 
> Best regards,
> Cen Zhang

Br, Jarkko

Br, Jarkko