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

[Jarkko Sakkinen <jarkko@kernel.org>]

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