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

On Mon, Jun 22, 2026 at 3:56 PM Jarkko Sakkinen <[email protected]> wrote:
>
> 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?

As I reread the original submission (somehow I never got the V2) it
seems we're restricting a keyring with a private key?! Which indeed
does not make sense.

> 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
>
>