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

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

[Guixiong Wei]

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.

Direct lookup visits the indexed description, so that ordering normally
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.

Fixes: b2a4df200d57 ("KEYS: Expand the capacity of a keyring")
Signed-off-by: Guixiong Wei <weiguixiong@bytedance.com>
---
 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

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

[Jarkko Sakkinen]

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.

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

> 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

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.


> 
> Fixes: b2a4df200d57 ("KEYS: Expand the capacity of a keyring")
> Signed-off-by: Guixiong Wei <weiguixiong@bytedance.com>
> ---
>  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

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

[Guixiong Wei]

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

> 
> 
> > 
> > Fixes: b2a4df200d57 ("KEYS: Expand the capacity of a keyring")
> > Signed-off-by: Guixiong Wei <weiguixiong@bytedance.com>
> > ---
> >  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

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

[Jarkko Sakkinen]

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