[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