Re: [PATCH] KEYS: do not let an unmatched key decide the search result

[Guixiong Wei]

> From: "Jarkko Sakkinen"<jarkko@xxxxxxxxxx>
> Date:  Mon, Nov 9, 2026, 03:01
> Subject:  Re: [PATCH] KEYS: do not let an unmatched key decide the search result
> To: "Guixiong Wei"<weiguixiong@xxxxxxxxxxxxx>
> Cc: "David Howells"<dhowells@xxxxxxxxxx>, <keyrings@xxxxxxxxxxxxxxx>, "Paul Moore"<paul@xxxxxxxxxxxxxx>, "James Morris"<jmorris@xxxxxxxxx>, "Serge E . Hallyn"<serge@xxxxxxxxxx>, <linux-security-module@xxxxxxxxxxxxxxx>, <linux-kernel@xxxxxxxxxxxxxxx>
> On Sun, Nov 08, 2026 at 09:14:12PM +0800, Guixiong Wei wrote:
> > KEYCTL_SEARCH returns -ENOKEY, -EKEYREVOKED, or -EKEYEXPIRED for the key
> > selected by type and description.  keyring_search_iterator() currently
> > records revoked or expired state before asking the key type whether the key
> > matches.
> 
> I don't understand the "asking the key type or" part here. Please,
> explain what you mean.
> 
I meant calling ctx->match_data.cmp() to check whether a candidate key
matches the search request. For dns_resolver, this calls
dns_resolver_cmp() to compare the candidate's description with the
requested name. I'll make that explicit in v2.

> > 
> > Direct lookup visits the indexed description, so that ordering normally
> 
> "Direct lookup visits the indexed description"
> 
> I don't honestly understand this type of jargon, which is all over the
> place.
> 
I meant that assoc_array_find() looks up a candidate using the requested
type and description. The dns_resolver search instead walks the keys and
checks their descriptions using dns_resolver_cmp().
I'll rewrite the commit message around your foo/bar example.

> > does not matter.  Iterative lookup visits every key of the requested type,
> > however.  The in-tree asymmetric id/ex/dn lookups and dns_resolver use this
> > mode.  A revoked key with a different description can therefore make
> > KEYCTL_SEARCH return -EKEYREVOKED instead of continuing the search.  It can
> > also prevent request_key_and_link() from issuing an upcall because that
> > path only upcalls after -EAGAIN.  An unmatched expired key similarly
> > pollutes searches that do not set KEYRING_SEARCH_SKIP_EXPIRED; the request
> > path itself sets that flag.
> > 
> > Run the type comparator before recording state errors, so only a key that
> > matches the caller's criterion can decide the search result.
> 
> 
> Can you describe instead describe the what goes wrong with an example,
> which does not require much effort:
> 
>         keyctl new_session >/dev/null
>         A=$(printf '127.0.0.1\0' | keyctl padd dns_resolver foo @s)
>         keyctl revoke "$A"
>         keyctl search @s dns_resolver bar
> 
> Results:
> 
>         keyctl_search: Key has been revoked
> 
Thanks for the reproducer. The expected result is -ENOKEY because "bar"
is absent.

When the search encounters "foo", keyring_search_iterator() sets
ctx->result to -EKEYREVOKED and skips that candidate before calling
dns_resolver_cmp(). It therefore never checks whether "foo" matches
"bar".

The walk continues, but in this example it finishes with ctx->result
still set to -EKEYREVOKED. My wording "instead of continuing the search"
was inaccurate.

The patch moves the match check before the state checks, so "foo" is
skipped without changing ctx->result.

> This example alone is not of course enough because it is lacking
> explanation what goes wrong, which is neither clearly described by
> the commit message but you could add my reported-by because at
> least I constructe a real bug report that justifies this fix.
Thanks, I will add your Reported-by.

> 
> 
> > 
> > Fixes: b2a4df200d57 ("KEYS: Expand the capacity of a keyring")
> > Signed-off-by: Guixiong Wei <weiguixiong@xxxxxxxxxxxxx>
> > ---
> >  security/keys/keyring.c | 12 ++++++------
> >  1 file changed, 6 insertions(+), 6 deletions(-)
> > 
> > diff --git a/security/keys/keyring.c b/security/keys/keyring.c
> > index 7143f3079d200..39645b8f9b30c 100644
> > --- a/security/keys/keyring.c
> > +++ b/security/keys/keyring.c
> > @@ -598,6 +598,12 @@ static int keyring_search_iterator(const void *object, void *iterator_data)
> >                  return 0;
> >          }
> >  
> > +        /* A key the caller did not ask for must not decide the result. */
> > +        if (!ctx->match_data.cmp(key, &ctx->match_data)) {
> > +                kleave(" = 0 [!match]");
> > +                return 0;
> > +        }
> > +
> >          /* skip invalidated, revoked and expired keys */
> >          if (ctx->flags & KEYRING_SEARCH_DO_STATE_CHECK) {
> >                  time64_t expiry = READ_ONCE(key->expiry);
> > @@ -617,12 +623,6 @@ static int keyring_search_iterator(const void *object, void *iterator_data)
> >                  }
> >          }
> >  
> > -        /* keys that don't match */
> > -        if (!ctx->match_data.cmp(key, &ctx->match_data)) {
> > -                kleave(" = 0 [!match]");
> > -                return 0;
> > -        }
> > -
> >          /* key must have search permissions */
> >          if (!(ctx->flags & KEYRING_SEARCH_NO_CHECK_PERM) &&
> >              key_task_permission(make_key_ref(key, ctx->possessed),
> > -- 
> > 2.55.0
> 
> Br, Jarkko
> 

Thanks,
Guixiong