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

On Mon, Jun 22, 2026 at 09:21:04PM +0100, Ignat Korchagin wrote:
> 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.

BTW I was writing two emails simulatenously this last sentence was for 
private email discussion.

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

And this.

> >
> > BR, Jarkko
> >
> >

+1's were for the discussion. Sorry, I should not multitask with
emails...

BR, Jarkko