[PATCH] KEYS: request_key: enforce keyring restrictions on constructed keys

[PATCH] KEYS: request_key: enforce keyring restrictions on constructed keys

[Anvik Loitongbam]

construct_alloc_key() pre-links a newly allocated key into the destination
keyring with __key_link(), bypassing __key_link_check_restriction(). If
the request-key helper instantiates the key with ringid 0 (or any keyring
other than the restricted destination), the key becomes positive while the
provisional link in a restricted keyring remains, so the restriction never
sees the payload, potentially resulting in a policy violation, where a
user with write access can inject keys which are restricted on the restricted
keyring.

Restricted destinations lose the provisional-link deduplication while the
key is being constructed. Callers that pass
KEY_ALLOC_BYPASS_RESTRICTION keep the existing provisional-link behavior.

Fixes: 5ac7eace2d00 ("KEYS: Add a check to restrict new links into a keyring")
Cc: stable@vger.kernel.org
Signed-off-by: Anvik Loitongbam <permissiontogallop@gmail.com>
---
 security/keys/request_key.c | 97 ++++++++++++++++++++++++++++++++-----
 1 file changed, 85 insertions(+), 12 deletions(-)

diff --git a/security/keys/request_key.c b/security/keys/request_key.c
index e6ba2d054399..3de6c9160651 100644
--- a/security/keys/request_key.c
+++ b/security/keys/request_key.c
@@ -370,18 +370,21 @@ static int construct_alloc_key(struct keyring_search_context *ctx,
 			       struct key *dest_keyring,
 			       unsigned long flags,
 			       struct key_user *user,
-			       struct key **_key)
+			       struct key **_key,
+			       bool *defer_link)
 {
 	struct assoc_array_edit *edit = NULL;
 	struct key *key;
 	key_perm_t perm;
 	key_ref_t key_ref;
+	bool link_locked = false;
 	int ret;
 
 	kenter("%s,%s,,,",
 	       ctx->index_key.type->name, ctx->index_key.description);
 
 	*_key = NULL;
+	*defer_link = false;
 	mutex_lock(&user->cons_lock);
 
 	perm = KEY_POS_VIEW | KEY_POS_SEARCH | KEY_POS_LINK | KEY_POS_SETATTR;
@@ -404,6 +407,21 @@ static int construct_alloc_key(struct keyring_search_context *ctx,
 		ret = __key_link_lock(dest_keyring, &key->index_key);
 		if (ret < 0)
 			goto link_lock_failed;
+
+		/*
+		 * A restricted keyring should not take an uninstantiated key,
+		 * the restriction has to be evaluated with the payload,
+		 * which is only available after construction. Skip the
+		 * provisional link and add the key with key_link() later,
+		 * unless the caller explicitly asked to bypass restrictions.
+		 */
+		if (dest_keyring->restrict_link &&
+		    !(flags & KEY_ALLOC_BYPASS_RESTRICTION)) {
+			__key_link_end(dest_keyring, &key->index_key, NULL);
+			*defer_link = true;
+		} else {
+			link_locked = true;
+		}
 	}
 
 	/*
@@ -424,7 +442,7 @@ static int construct_alloc_key(struct keyring_search_context *ctx,
 	if (!IS_ERR(key_ref))
 		goto key_already_present;
 
-	if (dest_keyring) {
+	if (dest_keyring && link_locked) {
 		ret = __key_link_begin(dest_keyring, &key->index_key, &edit);
 		if (ret < 0)
 			goto link_alloc_failed;
@@ -432,7 +450,7 @@ static int construct_alloc_key(struct keyring_search_context *ctx,
 	}
 
 	mutex_unlock(&key_construction_mutex);
-	if (dest_keyring)
+	if (link_locked)
 		__key_link_end(dest_keyring, &key->index_key, edit);
 	mutex_unlock(&user->cons_lock);
 	*_key = key;
@@ -446,15 +464,44 @@ static int construct_alloc_key(struct keyring_search_context *ctx,
 	mutex_unlock(&key_construction_mutex);
 	key = key_ref_to_ptr(key_ref);
 	if (dest_keyring) {
-		ret = __key_link_begin(dest_keyring, &key->index_key, &edit);
-		if (ret < 0)
-			goto link_alloc_failed_unlocked;
-		ret = __key_link_check_live_key(dest_keyring, key);
-		if (ret == 0)
-			__key_link(dest_keyring, key, &edit);
-		__key_link_end(dest_keyring, &key->index_key, edit);
-		if (ret < 0)
+		/*
+		 * The outer search checks Link permission on a key it finds.
+		 * The second search must do the same; otherwise a key that
+		 * appeared in the race window could be linked without the
+		 * caller holding Link permission on it.
+		 */
+		ret = key_task_permission(key_ref, ctx->cred, KEY_NEED_LINK);
+		if (ret < 0) {
+			if (link_locked)
+				__key_link_end(dest_keyring, &key->index_key,
+					       NULL);
 			goto link_check_failed;
+		}
+		if (*defer_link) {
+			/*
+			 * The key may still be under construction, in which
+			 * case its payload cannot be checked yet.  Do not add
+			 * a provisional link to a restricted keyring; only an
+			 * already instantiated key can be checked here.
+			 */
+			if (!key_is_positive(key)) {
+				ret = -EPERM;
+				goto link_check_failed;
+			}
+			ret = key_link(dest_keyring, key);
+			if (ret < 0)
+				goto link_check_failed;
+		} else {
+			ret = __key_link_begin(dest_keyring, &key->index_key, &edit);
+			if (ret < 0)
+				goto link_alloc_failed_unlocked;
+			ret = __key_link_check_live_key(dest_keyring, key);
+			if (ret == 0)
+				__key_link(dest_keyring, key, &edit);
+			__key_link_end(dest_keyring, &key->index_key, edit);
+			if (ret < 0)
+				goto link_check_failed;
+		}
 	}
 	mutex_unlock(&user->cons_lock);
 	*_key = key;
@@ -495,6 +542,7 @@ static struct key *construct_key_and_link(struct keyring_search_context *ctx,
 {
 	struct key_user *user;
 	struct key *key;
+	bool defer_link;
 	int ret;
 
 	kenter("");
@@ -512,7 +560,8 @@ static struct key *construct_key_and_link(struct keyring_search_context *ctx,
 		goto error_put_dest_keyring;
 	}
 
-	ret = construct_alloc_key(ctx, dest_keyring, flags, user, &key);
+	ret = construct_alloc_key(ctx, dest_keyring, flags, user, &key,
+				  &defer_link);
 	key_user_put(user);
 
 	if (ret == 0) {
@@ -522,6 +571,30 @@ static struct key *construct_key_and_link(struct keyring_search_context *ctx,
 			kdebug("cons failed");
 			goto construction_failed;
 		}
+
+		/*
+		 * The key was not provisionally linked because the
+		 * destination keyring is restricted. Link it now that the
+		 * payload is available so the restriction can be checked.
+		 */
+		if (defer_link) {
+			/*
+			 * A helper may have negatively instantiated the key
+			 * (e.g. KEYCTL_REJECT) and returned success.  A
+			 * restricted keyring must not cache a negative key;
+			 * key_reject_and_link() rejects those too.
+			 */
+			if (!key_is_positive(key)) {
+				ret = -EPERM;
+				key_put(key);
+				goto error_put_dest_keyring;
+			}
+			ret = key_link(dest_keyring, key);
+			if (ret < 0) {
+				key_put(key);
+				goto error_put_dest_keyring;
+			}
+		}
 	} else if (ret == -EINPROGRESS) {
 		ret = 0;
 	} else {
-- 
2.56.0

Re: [PATCH] KEYS: request_key: enforce keyring restrictions on constructed keys

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] KEYS: request_key_and_link() links uninstantiated key to restricted keyring
--

commit 97697a25e3b571e267998f93eacf1015231cd118
Author: Anvik Loitongbam <permissiontogallop@gmail.com>

KEYS: request_key: enforce keyring restrictions on constructed keys

This commit defers linking a newly allocated key into a restricted destination
keyring during key construction until the key has been instantiated and its
payload can be checked against the keyring's restriction.

> diff --git a/security/keys/request_key.c b/security/keys/request_key.c
> index fa2bb9f2f5381..a47e6a72ded89 100644
> --- a/security/keys/request_key.c
> +++ b/security/keys/request_key.c

[ ... ]

> @@ -446,15 +464,44 @@ static int construct_alloc_key(struct keyring_search_context *ctx,
>  	mutex_unlock(&key_construction_mutex);
>  	key = key_ref_to_ptr(key_ref);
>  	if (dest_keyring) {
> -		ret = __key_link_begin(dest_keyring, &key->index_key, &edit);
> -		if (ret < 0)
> -			goto link_alloc_failed_unlocked;
> -		ret = __key_link_check_live_key(dest_keyring, key);
> -		if (ret == 0)
> -			__key_link(dest_keyring, key, &edit);
> -		__key_link_end(dest_keyring, &key->index_key, edit);
> -		if (ret < 0)
> +		/*
> +		 * The outer search checks Link permission on a key it finds.
> +		 * The second search must do the same; otherwise a key that
> +		 * appeared in the race window could be linked without the
> +		 * caller holding Link permission on it.
> +		 */
> +		ret = key_task_permission(key_ref, ctx->cred, KEY_NEED_LINK);
> +		if (ret < 0) {
> +			if (link_locked)
> +				__key_link_end(dest_keyring, &key->index_key,
> +					       NULL);
>  			goto link_check_failed;
> +		}
> +		if (*defer_link) {
> +			/*
> +			 * The key may still be under construction, in which
> +			 * case its payload cannot be checked yet.  Do not add
> +			 * a provisional link to a restricted keyring; only an
> +			 * already instantiated key can be checked here.
> +			 */
> +			if (!key_is_positive(key)) {
> +				ret = -EPERM;
> +				goto link_check_failed;
> +			}
> +			ret = key_link(dest_keyring, key);
> +			if (ret < 0)
> +				goto link_check_failed;

[Severity: High]
Can an uninstantiated key still be linked into a restricted keyring through
the outer search in request_key_and_link()?

In keyring_search_iterator(), only negative keys (state < 0) are skipped when
KEYRING_SEARCH_DO_STATE_CHECK is set, so search_process_keyrings_rcu() returns
keys that are still in KEY_IS_UNINSTANTIATED (0) state:

security/keys/keyring.c:keyring_search_iterator() {
    ...
	if (ctx->flags & KEYRING_SEARCH_DO_STATE_CHECK) {
		/* we set a different error code if we pass a negative key */
		if (state < 0) {
			ctx->result = ERR_PTR(state);
			kleave(" = %d [neg]", ctx->skipped_ret);
			goto skipped;
		}
	}

	/* Found */
	ctx->result = make_key_ref(key, ctx->possessed);
	kleave(" = 1 [found]");
	return 1;
}

If a task initiates a request_key() call targeting an unrestricted process
keyring (such as the session, process, or thread keyring),
construct_alloc_key() provisionally links the uninstantiated key into that
keyring while the /sbin/request-key upcall is running.

If another thread or helper then calls request_key() for the same key with a
restricted dest_keyring before instantiation completes, the outer
search_process_keyrings_rcu() in request_key_and_link() finds that
uninstantiated key and passes it straight to key_link() before
wait_for_key_construction() is reached:

security/keys/request_key.c:request_key_and_link() {
    ...
	rcu_read_lock();
	key_ref = search_process_keyrings_rcu(&ctx);
	rcu_read_unlock();

	if (!IS_ERR(key_ref)) {
		...
		key = key_ref_to_ptr(key_ref);
		if (dest_keyring) {
			ret = key_link(dest_keyring, key);
    ...
}

Because __key_link_check_restriction() in security/keys/keyring.c also does
not check key_is_positive(key) before passing &key->payload to
keyring->restrict_link->check(), doesn't this cause a few issues?

First, if dest_keyring uses a restriction callback that only validates
key->type and relies on key->type->instantiate() to verify the payload (such
as restrict_link_for_blacklist() in certs/blacklist.c), or if
restrict_link->check is NULL, key_link() will link the uninstantiated key into
the restricted keyring, and the helper can subsequently instantiate or
negatively instantiate the key with ringid 0, leaving an unverified or
negative key linked in the restricted keyring.

Second, if the helper is concurrently instantiating the key with ringid 0 in
__key_instantiate_and_link() (which holds key_construction_mutex, not
dest_keyring->sem), key->type->instantiate() populates key->payload before
mark_key_instantiated() executes smp_store_release(&key->state,
KEY_IS_POSITIVE). Calling key_link() without first checking
key_is_positive(key) (which provides the pairing smp_load_acquire(&key->state))
allows restrict_link->check() to read key->payload concurrently with
instantiate().

Third, in construct_alloc_key() itself, because the first thread drops
user->cons_lock before calling construct_key(), a second thread of the same
user racing on the same key with a restricted dest_keyring can find the
uninstantiated key via the inner search and enter key_already_present while
!key_is_positive(key) is still true, causing it to fail immediately with
-EPERM instead of waiting for construction and then linking.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261030173930.40948-1-permissiontogallop@gmail.com?part=1

Re: [PATCH] KEYS: request_key: enforce keyring restrictions on constructed keys

[Jarkko Sakkinen]

On Fri, Oct 30, 2026 at 11:09:30PM +0530, Anvik Loitongbam wrote:
> construct_alloc_key() pre-links a newly allocated key into the destination
> keyring with __key_link(), bypassing __key_link_check_restriction(). If
> the request-key helper instantiates the key with ringid 0 (or any keyring
> other than the restricted destination), the key becomes positive while the
> provisional link in a restricted keyring remains, so the restriction never
> sees the payload, potentially resulting in a policy violation, where a
> user with write access can inject keys which are restricted on the restricted
> keyring.

What does "key becoming positive" mean here broken down?

Also I have really hard time on reading the second paragrah as it is,
let me count, over five lines long. I honestly cannot say that I
understood what I read. I tried it three times.

If you understand your fix, you should be able to nail it.

> 
> Restricted destinations lose the provisional-link deduplication while the
> key is being constructed. Callers that pass
> KEY_ALLOC_BYPASS_RESTRICTION keep the existing provisional-link behavior.

The second paragraph should be reformatted and there is no rationale
or descriptio for the code change.

Br, Jarkko

Re: [PATCH] KEYS: request_key: enforce keyring restrictions on constructed keys

[Gallop Permission]

Hi, Jarkko

1. you asked: What does "key becoming positive" mean here broken down?
    Here key becoming positive” means the transition from
KEY_IS_UNINSTANTIATED to KEY_IS_POSITIVE
    after the type’s instantiate() succeeded and the payload is present.

2. And on the second para
    For an unrestricted destination, construct_alloc_key() still
pre-links the under-construction key,
    so concurrent request_key() calls for the same type/description
find it and wait instead of
    starting another helper. That visibility is what deduplicates construction.

    For a restricted destination, the patch skips that provisional
link until the key has been
    constructed and checked. A concurrent request for the same key may
therefore fail to find
    the in-progress key and start a second helper. The keyring still
holds at most one link for
    a given type/description; each result is offered through
key_link(), and only a key accepted by
    the restriction is cached. This is a duplicate-work tradeoff, not
duplicate links in the keyring.

    KEY_ALLOC_BYPASS_RESTRICTION callers are excluded from the deferral,
    they keep the existing provisional-link behavior and bypass the
restriction check as before.

3. the rationale for the code change will be to stop an un priviledge
actor with write access to
a restricted keyring from bypassing the restriction checks.

As for the points raised by sashiko ai (concerning
potential outer-search bypasses in request_key_and_link)
im going to need some more time to verify them and fold any necessary
fixes for v2.

Best regards,
Anvik


On Fri, 30 Oct 2026 at 23:48, Jarkko Sakkinen <jarkko@kernel.org> wrote:
>
> On Fri, Oct 30, 2026 at 11:09:30PM +0530, Anvik Loitongbam wrote:
> > construct_alloc_key() pre-links a newly allocated key into the destination
> > keyring with __key_link(), bypassing __key_link_check_restriction(). If
> > the request-key helper instantiates the key with ringid 0 (or any keyring
> > other than the restricted destination), the key becomes positive while the
> > provisional link in a restricted keyring remains, so the restriction never
> > sees the payload, potentially resulting in a policy violation, where a
> > user with write access can inject keys which are restricted on the restricted
> > keyring.
>
> What does "key becoming positive" mean here broken down?
>
> Also I have really hard time on reading the second paragrah as it is,
> let me count, over five lines long. I honestly cannot say that I
> understood what I read. I tried it three times.
>
> If you understand your fix, you should be able to nail it.
>
> >
> > Restricted destinations lose the provisional-link deduplication while the
> > key is being constructed. Callers that pass
> > KEY_ALLOC_BYPASS_RESTRICTION keep the existing provisional-link behavior.
>
> The second paragraph should be reformatted and there is no rationale
> or descriptio for the code change.
>
> Br, Jarkko

Re: [PATCH] KEYS: request_key: enforce keyring restrictions on constructed keys

[Jarkko Sakkinen]

On Sat, Oct 31, 2026 at 10:10:36AM +0530, Gallop Permission wrote:
> Hi, Jarkko
> 
> 1. you asked: What does "key becoming positive" mean here broken down?
>     Here key becoming positive” means the transition from
> KEY_IS_UNINSTANTIATED to KEY_IS_POSITIVE
>     after the type’s instantiate() succeeded and the payload is present.

Great use concrete termininology.

> 
> 2. And on the second para
>     For an unrestricted destination, construct_alloc_key() still
> pre-links the under-construction key,
>     so concurrent request_key() calls for the same type/description
> find it and wait instead of
>     starting another helper. That visibility is what deduplicates construction.
> 
>     For a restricted destination, the patch skips that provisional
> link until the key has been
>     constructed and checked. A concurrent request for the same key may
> therefore fail to find
>     the in-progress key and start a second helper. The keyring still
> holds at most one link for
>     a given type/description; each result is offered through
> key_link(), and only a key accepted by
>     the restriction is cached. This is a duplicate-work tradeoff, not
> duplicate links in the keyring.
> 
>     KEY_ALLOC_BYPASS_RESTRICTION callers are excluded from the deferral,
>     they keep the existing provisional-link behavior and bypass the
> restriction check as before.

Ditto.

> 
> 3. the rationale for the code change will be to stop an un priviledge
> actor with write access to
> a restricted keyring from bypassing the restriction checks.

OK, so I did not mean that. That is motivation to do a code change,
which is I guess what previous paragraph try to answer.

It says nothing about solution per se. And neither I see how this
answer would give anything on that.

> 
> As for the points raised by sashiko ai (concerning
> potential outer-search bypasses in request_key_and_link)
> im going to need some more time to verify them and fold any necessary
> fixes for v2.

Address what I said about commit message by writing a proper commit
message.

> 
> Best regards,
> Anvik
> 
> 
> On Fri, 30 Oct 2026 at 23:48, Jarkko Sakkinen <jarkko@kernel.org> wrote:
> >
> > On Fri, Oct 30, 2026 at 11:09:30PM +0530, Anvik Loitongbam wrote:
> > > construct_alloc_key() pre-links a newly allocated key into the destination
> > > keyring with __key_link(), bypassing __key_link_check_restriction(). If
> > > the request-key helper instantiates the key with ringid 0 (or any keyring
> > > other than the restricted destination), the key becomes positive while the
> > > provisional link in a restricted keyring remains, so the restriction never
> > > sees the payload, potentially resulting in a policy violation, where a
> > > user with write access can inject keys which are restricted on the restricted
> > > keyring.
> >
> > What does "key becoming positive" mean here broken down?
> >
> > Also I have really hard time on reading the second paragrah as it is,
> > let me count, over five lines long. I honestly cannot say that I
> > understood what I read. I tried it three times.
> >
> > If you understand your fix, you should be able to nail it.
> >
> > >
> > > Restricted destinations lose the provisional-link deduplication while the
> > > key is being constructed. Callers that pass
> > > KEY_ALLOC_BYPASS_RESTRICTION keep the existing provisional-link behavior.
> >
> > The second paragraph should be reformatted and there is no rationale
> > or descriptio for the code change.
> >
> > Br, Jarkko

Br, Jarkko

Re: [PATCH] KEYS: request_key: enforce keyring restrictions on constructed keys

[Jarkko Sakkinen]

On Sat, Oct 31, 2026 at 09:53:54AM +0200, Jarkko Sakkinen wrote:
> On Sat, Oct 31, 2026 at 10:10:36AM +0530, Gallop Permission wrote:
> > Hi, Jarkko
> > 
> > 1. you asked: What does "key becoming positive" mean here broken down?
> >     Here key becoming positive” means the transition from
> > KEY_IS_UNINSTANTIATED to KEY_IS_POSITIVE
> >     after the type’s instantiate() succeeded and the payload is present.
> 
> Great use concrete termininology.

Please also read:

https://docs.kernel.org/process/email-clients.html

Especially quoting part. Your responses are somewhat difficult to read
and we don't prefer top-posting.

Br, Jarkko

Re: [PATCH] KEYS: request_key: enforce keyring restrictions on constructed keys

[Jarkko Sakkinen]

On Sat, Oct 31, 2026 at 09:56:03AM +0200, Jarkko Sakkinen wrote:
> On Sat, Oct 31, 2026 at 09:53:54AM +0200, Jarkko Sakkinen wrote:
> > On Sat, Oct 31, 2026 at 10:10:36AM +0530, Gallop Permission wrote:
> > > Hi, Jarkko
> > > 
> > > 1. you asked: What does "key becoming positive" mean here broken down?
> > >     Here key becoming positive” means the transition from
> > > KEY_IS_UNINSTANTIATED to KEY_IS_POSITIVE
> > >     after the type’s instantiate() succeeded and the payload is present.
> > 
> > Great use concrete termininology.
> 
> Please also read:
> 
> https://docs.kernel.org/process/email-clients.html
> 
> Especially quoting part. Your responses are somewhat difficult to read
> and we don't prefer top-posting.

One more thing: the short summary (subject) is incorrect as we already
do enforce restrictions.

I missed this one earlier. Just put something more appropriate, that's
all.

Br, Jarkko

Re: [PATCH] KEYS: request_key: enforce keyring restrictions on constructed keys

[Jarkko Sakkinen]

On Fri, Oct 30, 2026 at 11:09:30PM +0530, Anvik Loitongbam wrote:
> diff --git a/security/keys/request_key.c b/security/keys/request_key.c
> index e6ba2d054399..3de6c9160651 100644
> --- a/security/keys/request_key.c
> +++ b/security/keys/request_key.c
> @@ -370,18 +370,21 @@ static int construct_alloc_key(struct keyring_search_context *ctx,
>  			       struct key *dest_keyring,
>  			       unsigned long flags,
>  			       struct key_user *user,
> -			       struct key **_key)
> +			       struct key **_key,
> +			       bool *defer_link)
>  {
>  	struct assoc_array_edit *edit = NULL;
>  	struct key *key;
>  	key_perm_t perm;
>  	key_ref_t key_ref;
> +	bool link_locked = false;
>  	int ret;
>  
>  	kenter("%s,%s,,,",
>  	       ctx->index_key.type->name, ctx->index_key.description);
>  
>  	*_key = NULL;
> +	*defer_link = false;
>  	mutex_lock(&user->cons_lock);
>  
>  	perm = KEY_POS_VIEW | KEY_POS_SEARCH | KEY_POS_LINK | KEY_POS_SETATTR;
> @@ -404,6 +407,21 @@ static int construct_alloc_key(struct keyring_search_context *ctx,
>  		ret = __key_link_lock(dest_keyring, &key->index_key);
>  		if (ret < 0)
>  			goto link_lock_failed;
> +
> +		/*
> +		 * A restricted keyring should not take an uninstantiated key,
> +		 * the restriction has to be evaluated with the payload,
> +		 * which is only available after construction. Skip the
> +		 * provisional link and add the key with key_link() later,
> +		 * unless the caller explicitly asked to bypass restrictions.
> +		 */
> +		if (dest_keyring->restrict_link &&
> +		    !(flags & KEY_ALLOC_BYPASS_RESTRICTION)) {
> +			__key_link_end(dest_keyring, &key->index_key, NULL);
> +			*defer_link = true;
> +		} else {
> +			link_locked = true;
> +		}
>  	}
>  
>  	/*
> @@ -424,7 +442,7 @@ static int construct_alloc_key(struct keyring_search_context *ctx,
>  	if (!IS_ERR(key_ref))
>  		goto key_already_present;
>  
> -	if (dest_keyring) {
> +	if (dest_keyring && link_locked) {
>  		ret = __key_link_begin(dest_keyring, &key->index_key, &edit);
>  		if (ret < 0)
>  			goto link_alloc_failed;
> @@ -432,7 +450,7 @@ static int construct_alloc_key(struct keyring_search_context *ctx,
>  	}
>  
>  	mutex_unlock(&key_construction_mutex);
> -	if (dest_keyring)
> +	if (link_locked)
>  		__key_link_end(dest_keyring, &key->index_key, edit);
>  	mutex_unlock(&user->cons_lock);
>  	*_key = key;
> @@ -446,15 +464,44 @@ static int construct_alloc_key(struct keyring_search_context *ctx,
>  	mutex_unlock(&key_construction_mutex);
>  	key = key_ref_to_ptr(key_ref);
>  	if (dest_keyring) {
> -		ret = __key_link_begin(dest_keyring, &key->index_key, &edit);
> -		if (ret < 0)
> -			goto link_alloc_failed_unlocked;
> -		ret = __key_link_check_live_key(dest_keyring, key);
> -		if (ret == 0)
> -			__key_link(dest_keyring, key, &edit);
> -		__key_link_end(dest_keyring, &key->index_key, edit);
> -		if (ret < 0)
> +		/*
> +		 * The outer search checks Link permission on a key it finds.
> +		 * The second search must do the same; otherwise a key that
> +		 * appeared in the race window could be linked without the
> +		 * caller holding Link permission on it.
> +		 */
> +		ret = key_task_permission(key_ref, ctx->cred, KEY_NEED_LINK);
> +		if (ret < 0) {
> +			if (link_locked)
> +				__key_link_end(dest_keyring, &key->index_key,
> +					       NULL);
>  			goto link_check_failed;
> +		}
> +		if (*defer_link) {
> +			/*
> +			 * The key may still be under construction, in which
> +			 * case its payload cannot be checked yet.  Do not add
> +			 * a provisional link to a restricted keyring; only an
> +			 * already instantiated key can be checked here.
> +			 */
> +			if (!key_is_positive(key)) {
> +				ret = -EPERM;
> +				goto link_check_failed;
> +			}
> +			ret = key_link(dest_keyring, key);
> +			if (ret < 0)
> +				goto link_check_failed;
> +		} else {
> +			ret = __key_link_begin(dest_keyring, &key->index_key, &edit);
> +			if (ret < 0)
> +				goto link_alloc_failed_unlocked;
> +			ret = __key_link_check_live_key(dest_keyring, key);
> +			if (ret == 0)
> +				__key_link(dest_keyring, key, &edit);
> +			__key_link_end(dest_keyring, &key->index_key, edit);
> +			if (ret < 0)
> +				goto link_check_failed;
> +		}
>  	}
>  	mutex_unlock(&user->cons_lock);
>  	*_key = key;
> @@ -495,6 +542,7 @@ static struct key *construct_key_and_link(struct keyring_search_context *ctx,
>  {
>  	struct key_user *user;
>  	struct key *key;
> +	bool defer_link;
>  	int ret;
>  
>  	kenter("");
> @@ -512,7 +560,8 @@ static struct key *construct_key_and_link(struct keyring_search_context *ctx,
>  		goto error_put_dest_keyring;
>  	}
>  
> -	ret = construct_alloc_key(ctx, dest_keyring, flags, user, &key);
> +	ret = construct_alloc_key(ctx, dest_keyring, flags, user, &key,
> +				  &defer_link);
>  	key_user_put(user);
>  
>  	if (ret == 0) {
> @@ -522,6 +571,30 @@ static struct key *construct_key_and_link(struct keyring_search_context *ctx,
>  			kdebug("cons failed");
>  			goto construction_failed;
>  		}
> +
> +		/*
> +		 * The key was not provisionally linked because the
> +		 * destination keyring is restricted. Link it now that the
> +		 * payload is available so the restriction can be checked.
> +		 */
> +		if (defer_link) {
> +			/*
> +			 * A helper may have negatively instantiated the key
> +			 * (e.g. KEYCTL_REJECT) and returned success.  A
> +			 * restricted keyring must not cache a negative key;
> +			 * key_reject_and_link() rejects those too.
> +			 */
> +			if (!key_is_positive(key)) {
> +				ret = -EPERM;
> +				key_put(key);
> +				goto error_put_dest_keyring;
> +			}
> +			ret = key_link(dest_keyring, key);
> +			if (ret < 0) {
> +				key_put(key);
> +				goto error_put_dest_keyring;
> +			}
> +		}
>  	} else if (ret == -EINPROGRESS) {
>  		ret = 0;
>  	} else {
> -- 
> 2.56.0
> 
> 

I tried to go through the code change now as it is huge and there was no
explanation why you need to do all of this.

It is full out-of-scope changes. Please redo this just as a scoped bug
fix, and don't add any new flags.

Br, Jarkk

Re: [PATCH] KEYS: request_key: enforce keyring restrictions on constructed keys

[Jarkko Sakkinen]

[-- Attachment #1: Type: text/plain, Size: 619 bytes --]

On Sat, Oct 31, 2026 at 10:20:44AM +0200, Jarkko Sakkinen wrote:
> I tried to go through the code change now as it is huge and there was no
> explanation why you need to do all of this.
> 
> It is full out-of-scope changes. Please redo this just as a scoped bug
> fix, and don't add any new flags.

Unfortunately it immediately hit me after sending this how to fix the bug,
and it does not make much sense to ask to write the same fix I would do
myself anyway.

I'll rather go this time with the attached patch and consider this as a
bug report (as marked by reported-by).

Thank you for your contribution.

Br, Jarkko

[-- Attachment #2: 0001-KEYS-Reject-restricted-dest_keyring-in-construct_all.patch --]
[-- Type: text/x-diff, Size: 1580 bytes --]

From 2e37dd3a4fe1c10ebaeec3243e48baa25a6efa1b Mon Sep 17 00:00:00 2001
From: Jarkko Sakkinen <jarkko@kernel.org>
Date: Sat, 31 Oct 2026 11:06:59 +0200
Subject: [PATCH] KEYS: Reject restricted dest_keyring in construct_alloc_key()

construct_alloc_key() links keys into dest_keyring without checking
restrict_link, allowing keys whose key->state reaches KEY_IS_POSITIVE to
remain in a restricted keyring when instantiated without relinking into
dest_keyring. Reject restricted destination keyrings with -EPERM after
__key_link_lock() unless KEY_ALLOC_BYPASS_RESTRICTION is set.

Cc: stable@vger.kernel.org
Fixes: 5ac7eace2d00 ("KEYS: Add a facility to restrict new links into a keyring")
Reported-by: Anvik Loitongbam <permissiontogallop@gmail.com>
Closes: https://lore.kernel.org/keyrings/20261030173930.40948-1-permissiontogallop@gmail.com/
Signed-off-by: Jarkko Sakkinen <jarkko@kernel.org>
---
 security/keys/request_key.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/security/keys/request_key.c b/security/keys/request_key.c
index 22cd66a44da5..715ef3cc51b3 100644
--- a/security/keys/request_key.c
+++ b/security/keys/request_key.c
@@ -404,6 +404,13 @@ static int construct_alloc_key(struct keyring_search_context *ctx,
 		ret = __key_link_lock(dest_keyring, &key->index_key);
 		if (ret < 0)
 			goto link_lock_failed;
+
+		/* The payload is not yet available to check the restriction. */
+		if (dest_keyring->restrict_link &&
+		    !(flags & KEY_ALLOC_BYPASS_RESTRICTION)) {
+			ret = -EPERM;
+			goto link_alloc_failed_unlocked;
+		}
 	}
 
 	/*
-- 
2.47.3