Re: [PATCH v2] asymmetric_keys: check asymmetric_key_ids() for NULL before dereference

On Mon, Jun 22, 2026 at 12:16:07PM +0800, Herbert Xu wrote:
> On Sat, May 02, 2026 at 09:33:29AM -0700, Weiming Shi wrote:
> >
> > diff --git a/crypto/asymmetric_keys/asymmetric_type.c b/crypto/asymmetric_keys/asymmetric_type.c
> > index 16a7ae16593c..22f04656d529 100644
> > --- a/crypto/asymmetric_keys/asymmetric_type.c
> > +++ b/crypto/asymmetric_keys/asymmetric_type.c
> > @@ -109,6 +109,8 @@ struct key *find_asymmetric_key(struct key *keyring,
> >  	if (id_0 && id_1) {
> >  		const struct asymmetric_key_ids *kids = asymmetric_key_ids(key);
> >  
> > +		if (!kids)
> > +			goto reject;
> 
> This check is actually unnecessary because we've already matched
> the key against the kid so it must be present.
> 
> I'd get rid of this check or perhaps add a comment instead.

+1

> 
> >  		if (!kids->id[1]) {
> >  			pr_debug("First ID matches, but second is missing\n");
> >  			goto reject;
> > diff --git a/crypto/asymmetric_keys/restrict.c b/crypto/asymmetric_keys/restrict.c
> > index 86292965f493..ccf1084f720e 100644
> > --- a/crypto/asymmetric_keys/restrict.c
> > +++ b/crypto/asymmetric_keys/restrict.c
> > @@ -243,10 +243,14 @@ static int key_or_keyring_common(struct key *dest_keyring,
> >  			if (IS_ERR(key))
> >  				key = NULL;
> >  		} else if (trusted->type == &key_type_asymmetric) {
> > +			const struct asymmetric_key_ids *kids;
> >  			const struct asymmetric_key_id **signer_ids;
> >  
> > -			signer_ids = (const struct asymmetric_key_id **)
> > -				asymmetric_key_ids(trusted)->id;
> > +			kids = asymmetric_key_ids(trusted);
> > +			if (!kids)
> > +				goto skip_trusted;
> 
> Yes this is definitely buggy.
> 
> I think it was introduced by these two commits:
> 
> commit 3c58b2362ba828ee2970c66c6a6fd7b04fde4413
> Author: David Howells <[email protected]>
> Date:   Tue Oct 9 17:47:46 2018 +0100
> 
>     KEYS: Implement PKCS#8 RSA Private Key parser [ver #2]
> 
> and
> 
> commit 7e3c4d22083f6e7316c5229b6197ca2d5335aa35
> Author: Mat Martineau <[email protected]>
> Date:   Mon Jun 27 16:45:16 2016 -0700
> 
>     KEYS: Restrict asymmetric key linkage using a specific keychain
> 
> So the Fixes header should point to them.

+1

> 
> > @@ -290,6 +294,7 @@ static int key_or_keyring_common(struct key *dest_keyring,
> >  		}
> >  	}
> >  
> > +skip_trusted:
> >  	if (check_dest && !key) {
> >  		/* See if the destination has a key that signed this one. */
> >  		key = find_asymmetric_key(dest_keyring, sig->auth_ids[0],
> 
> I'm not sure continuing here is a good idea.  Having a private key
> here makes no sense whatsoever and we should just bail out right
> away.
> 
> I would recommend returning an error of some sort if kids is NULL.
> 
> David/Lukas/Ignat, any opinions?

I think with a quick skim that you are right. I'll work on this area
for the next version.

> 
> Thanks,
> -- 
> Email: Herbert Xu <[email protected]>
> Home Page: http://gondor.apana.org.au/~herbert/
> PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Thanks for the review!

BR, Jarkko