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

[Jarkko Sakkinen <jarkko@kernel.org>]

On Mon, Nov 09, 2026 at 09:10:08AM +0800, Guixiong Wei wrote:
> 
> > From: "Jarkko Sakkinen"<jarkko@kernel.org>
> > 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@bytedance.com>
> > Cc: "David Howells"<dhowells@redhat.com>, <keyrings@vger.kernel.org>, "Paul Moore"<paul@paul-moore.com>, "James Morris"<jmorris@namei.org>, "Serge E . Hallyn"<serge@hallyn.com>, <linux-security-module@vger.kernel.org>, <linux-kernel@vger.kernel.org>
> > 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.

Yeah, just sorting things out in proper order :-) Do those changes for
v2 and this should be good to go.

Br, Jarkko