Linux kernel -stable discussions
 help / color / mirror / Atom feed
* [PATCH v3 1/3] KEYS: Expand key->{datalen,quotalen} to unsigned int
       [not found] <20261020013903.1244157-1-jarkko@kernel.org>
@ 2026-10-20  1:38 ` Jarkko Sakkinen
  2026-10-20 15:47   ` Eric Biggers
  2026-10-20  1:38 ` [PATCH v3 2/3] KEYS: Reject @desclen values >= KEY_MAX_DESC_SIZE in key_alloc() Jarkko Sakkinen
  2026-10-20  1:38 ` [PATCH v3 3/3] KEYS: Verify the key identity in __key_unlink_begin Jarkko Sakkinen
  2 siblings, 1 reply; 11+ messages in thread
From: Jarkko Sakkinen @ 2026-10-20  1:38 UTC (permalink / raw)
  To: David Howells
  Cc: keyrings, Eric Biggers, Jarkko Sakkinen, stable, Paul Moore,
	James Morris, Serge E. Hallyn, open list,
	open list:SECURITY SUBSYSTEM

In key_alloc() and key_payload_reserve(), key->quotalen and key->datalen
can receive values larger than USHRT_MAX.

Example:

  keyctl new_session >/dev/null
  owner=$(keyctl newring owner @s)
  keeper=$(keyctl newring keeper @s)

  for ((i=0; i<16384; i++)); do
      key=$(keyctl add user "k$i" x "$keeper")
      keyctl link "$key" "$owner"
  done

  awk -v uid="$(id -u):" '$1 == uid {print $NF}' /proc/key-users
  keyctl clear "$owner"
  awk -v uid="$(id -u):" '$1 == uid {print $NF}' /proc/key-users

Because key->datalen wraps around 65535, the quota refunded upon clearing
is truncated, causing quota bytes to leak in /proc/key-users.

Change the types of key->datalen and key->quotalen from unsigned short to
unsigned int so that they can carry the full payload length without
truncation.

Cc: stable@vger.kernel.org
Assisted-by: LLM
Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
Reviewed-by: David Howells <dhowells@redhat.com>
Signed-off-by: Jarkko Sakkinen <jarkko@kernel.org>
---
 include/linux/key.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/include/linux/key.h b/include/linux/key.h
index e40b5267658f..26139c3adf62 100644
--- a/include/linux/key.h
+++ b/include/linux/key.h
@@ -216,8 +216,8 @@ struct key {
 	kuid_t			uid;
 	kgid_t			gid;
 	key_perm_t		perm;		/* access permissions */
-	unsigned short		quotalen;	/* length added to quota */
-	unsigned short		datalen;	/* payload data length
+	unsigned int		quotalen;	/* length added to quota */
+	unsigned int		datalen;	/* payload data length
 						 * - may not match RCU dereferenced payload
 						 * - payload should contain own length
 						 */
-- 
2.47.3


^ permalink raw reply related	[flat|nested] 11+ messages in thread

* [PATCH v3 2/3] KEYS: Reject @desclen values >= KEY_MAX_DESC_SIZE in key_alloc()
       [not found] <20261020013903.1244157-1-jarkko@kernel.org>
  2026-10-20  1:38 ` [PATCH v3 1/3] KEYS: Expand key->{datalen,quotalen} to unsigned int Jarkko Sakkinen
@ 2026-10-20  1:38 ` Jarkko Sakkinen
  2026-10-20  1:47   ` Jarkko Sakkinen
  2026-10-20  1:38 ` [PATCH v3 3/3] KEYS: Verify the key identity in __key_unlink_begin Jarkko Sakkinen
  2 siblings, 1 reply; 11+ messages in thread
From: Jarkko Sakkinen @ 2026-10-20  1:38 UTC (permalink / raw)
  To: David Howells
  Cc: keyrings, Eric Biggers, Jarkko Sakkinen, stable, Paul Moore,
	James Morris, Serge E. Hallyn, open list,
	open list:SECURITY SUBSYSTEM

@desclen in key_alloc() is saved to the 16-bit wide desc_len field in
struct keyring_index_key.

Reject @desclen values at or above KEY_MAX_DESC_SIZE with -EINVAL in order
to match the userspace limit. The constraints inherits from the one byte
required for the NULL character.

Cc: stable@vger.kernel.org
Assisted-by: LLM
Fixes: f771fde82051 ("keys: Simplify key description management")
Reviewed-by: David Howells <dhowells@redhat.com>
Signed-off-by: Jarkko Sakkinen <jarkko@kernel.org>
---
 include/linux/key.h    | 2 ++
 security/keys/key.c    | 2 ++
 security/keys/keyctl.c | 2 --
 3 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/include/linux/key.h b/include/linux/key.h
index 26139c3adf62..fa88de9b5a9b 100644
--- a/include/linux/key.h
+++ b/include/linux/key.h
@@ -37,6 +37,8 @@ struct net;
 
 #undef KEY_DEBUGGING
 
+#define KEY_MAX_DESC_SIZE 4096
+
 #define KEY_POS_VIEW	0x01000000	/* possessor can view a key's attributes */
 #define KEY_POS_READ	0x02000000	/* possessor can read key payload / view keyring */
 #define KEY_POS_WRITE	0x04000000	/* possessor can update key payload / add link to keyring */
diff --git a/security/keys/key.c b/security/keys/key.c
index 6ebb8691551f..e9f8cacf00c8 100644
--- a/security/keys/key.c
+++ b/security/keys/key.c
@@ -245,6 +245,8 @@ struct key *key_alloc(struct key_type *type, const char *desc,
 	}
 
 	desclen = strlen(desc);
+	if (desclen >= KEY_MAX_DESC_SIZE)
+		goto error;
 	quotalen = desclen + 1 + type->def_datalen;
 
 	/* get hold of the key tracking for this user */
diff --git a/security/keys/keyctl.c b/security/keys/keyctl.c
index 36ffba0ebba6..d6b10fd67627 100644
--- a/security/keys/keyctl.c
+++ b/security/keys/keyctl.c
@@ -24,8 +24,6 @@
 #include <keys/request_key_auth-type.h>
 #include "internal.h"
 
-#define KEY_MAX_DESC_SIZE 4096
-
 static const unsigned char keyrings_capabilities[2] = {
 	[0] = (KEYCTL_CAPS0_CAPABILITIES |
 	       (IS_ENABLED(CONFIG_PERSISTENT_KEYRINGS)	? KEYCTL_CAPS0_PERSISTENT_KEYRINGS : 0) |
-- 
2.47.3


^ permalink raw reply related	[flat|nested] 11+ messages in thread

* [PATCH v3 3/3] KEYS: Verify the key identity in __key_unlink_begin
       [not found] <20261020013903.1244157-1-jarkko@kernel.org>
  2026-10-20  1:38 ` [PATCH v3 1/3] KEYS: Expand key->{datalen,quotalen} to unsigned int Jarkko Sakkinen
  2026-10-20  1:38 ` [PATCH v3 2/3] KEYS: Reject @desclen values >= KEY_MAX_DESC_SIZE in key_alloc() Jarkko Sakkinen
@ 2026-10-20  1:38 ` Jarkko Sakkinen
  2 siblings, 0 replies; 11+ messages in thread
From: Jarkko Sakkinen @ 2026-10-20  1:38 UTC (permalink / raw)
  To: David Howells
  Cc: keyrings, Eric Biggers, Jarkko Sakkinen, stable, Paul Moore,
	James Morris, Serge E. Hallyn, open list:SECURITY SUBSYSTEM,
	open list

When two keys have identical type and description, linking the second key
replaces the first in a keyring. However, attempting to unlink the first
key (no longer in the keyring) unlinks the second key.

This happens because __key_unlink_begin() calls assoc_array_delete() with
&key->index_key, matching only on type and description rather than the
key's identity.

Example:

  keyctl new_session >/dev/null
  keyctl newring ring_a @s
  keyctl newring ring_b @s
  A=$(keyctl add user A ss %:ring_a)
  B=$(keyctl add user A qq %:ring_b)
  keyctl newring R @s
  keyctl link $A %:R
  keyctl link $B %:R
  keyctl unlink $A %:R
  keyctl show

Verify the key identity before unlinking, and return -ENOENT on mismatch,
instead of unlinking the key with the wrong identity.

Cc: stable@vger.kernel.org
Assisted-by: LLM
Fixes: b2a4df200d57 ("KEYS: Expand the capacity of a keyring")
Reviewed-by: David Howells <dhowells@redhat.com>
Signed-off-by: Jarkko Sakkinen <jarkko@kernel.org>
---
 security/keys/keyring.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/security/keys/keyring.c b/security/keys/keyring.c
index 7143f3079d20..b025331488c5 100644
--- a/security/keys/keyring.c
+++ b/security/keys/keyring.c
@@ -1501,9 +1501,15 @@ static int __key_unlink_begin(struct key *keyring, struct key *key,
 			      struct assoc_array_edit **_edit)
 {
 	struct assoc_array_edit *edit;
+	const void *object;
 
 	BUG_ON(*_edit != NULL);
 
+	object = assoc_array_find(&keyring->keys, &keyring_assoc_array_ops,
+				  &key->index_key);
+	if (!object || keyring_ptr_to_key(object) != key)
+		return -ENOENT;
+
 	edit = assoc_array_delete(&keyring->keys, &keyring_assoc_array_ops,
 				  &key->index_key);
 	if (IS_ERR(edit))
-- 
2.47.3


^ permalink raw reply related	[flat|nested] 11+ messages in thread

* Re: [PATCH v3 2/3] KEYS: Reject @desclen values >= KEY_MAX_DESC_SIZE in key_alloc()
  2026-10-20  1:38 ` [PATCH v3 2/3] KEYS: Reject @desclen values >= KEY_MAX_DESC_SIZE in key_alloc() Jarkko Sakkinen
@ 2026-10-20  1:47   ` Jarkko Sakkinen
  2026-10-20  2:28     ` Eric Biggers
  0 siblings, 1 reply; 11+ messages in thread
From: Jarkko Sakkinen @ 2026-10-20  1:47 UTC (permalink / raw)
  To: David Howells
  Cc: keyrings, Eric Biggers, stable, Paul Moore, James Morris,
	Serge E. Hallyn, open list, open list:SECURITY SUBSYSTEM

On Tue, Oct 20, 2026 at 04:38:56AM +0300, Jarkko Sakkinen wrote:
> @desclen in key_alloc() is saved to the 16-bit wide desc_len field in
> struct keyring_index_key.
> 
> Reject @desclen values at or above KEY_MAX_DESC_SIZE with -EINVAL in order
> to match the userspace limit. The constraints inherits from the one byte
> required for the NULL character.
> 
> Cc: stable@vger.kernel.org
> Assisted-by: LLM
> Fixes: f771fde82051 ("keys: Simplify key description management")
> Reviewed-by: David Howells <dhowells@redhat.com>
> Signed-off-by: Jarkko Sakkinen <jarkko@kernel.org>

This is now the only one, which does not have keyutils script in the
commit message so in that sense I think these fixes look pretty good.

Br, Jarkko

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v3 2/3] KEYS: Reject @desclen values >= KEY_MAX_DESC_SIZE in key_alloc()
  2026-10-20  1:47   ` Jarkko Sakkinen
@ 2026-10-20  2:28     ` Eric Biggers
  2026-10-20 10:57       ` Jarkko Sakkinen
  0 siblings, 1 reply; 11+ messages in thread
From: Eric Biggers @ 2026-10-20  2:28 UTC (permalink / raw)
  To: Jarkko Sakkinen
  Cc: David Howells, keyrings, stable, Paul Moore, James Morris,
	Serge E. Hallyn, open list, open list:SECURITY SUBSYSTEM

On Tue, Oct 20, 2026 at 04:47:43AM +0300, Jarkko Sakkinen wrote:
> On Tue, Oct 20, 2026 at 04:38:56AM +0300, Jarkko Sakkinen wrote:
> > @desclen in key_alloc() is saved to the 16-bit wide desc_len field in
> > struct keyring_index_key.
> > 
> > Reject @desclen values at or above KEY_MAX_DESC_SIZE with -EINVAL in order
> > to match the userspace limit. The constraints inherits from the one byte
> > required for the NULL character.
> > 
> > Cc: stable@vger.kernel.org
> > Assisted-by: LLM
> > Fixes: f771fde82051 ("keys: Simplify key description management")
> > Reviewed-by: David Howells <dhowells@redhat.com>
> > Signed-off-by: Jarkko Sakkinen <jarkko@kernel.org>
> 
> This is now the only one, which does not have keyutils script in the
> commit message so in that sense I think these fixes look pretty good.

Sure, this fix is still incomplete though, as I mentioned.

Turns out Sashiko found this too; see the comment on
https://sashiko.dev/#/patchset/20261020013903.1244157-1-jarkko%40kernel.org

- Eric

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v3 2/3] KEYS: Reject @desclen values >= KEY_MAX_DESC_SIZE in key_alloc()
  2026-10-20  2:28     ` Eric Biggers
@ 2026-10-20 10:57       ` Jarkko Sakkinen
  2026-10-20 13:00         ` Jarkko Sakkinen
  0 siblings, 1 reply; 11+ messages in thread
From: Jarkko Sakkinen @ 2026-10-20 10:57 UTC (permalink / raw)
  To: Eric Biggers
  Cc: David Howells, keyrings, stable, Paul Moore, James Morris,
	Serge E. Hallyn, open list, open list:SECURITY SUBSYSTEM

On Mon, Oct 19, 2026 at 07:28:32PM -0700, Eric Biggers wrote:
> On Tue, Oct 20, 2026 at 04:47:43AM +0300, Jarkko Sakkinen wrote:
> > On Tue, Oct 20, 2026 at 04:38:56AM +0300, Jarkko Sakkinen wrote:
> > > @desclen in key_alloc() is saved to the 16-bit wide desc_len field in
> > > struct keyring_index_key.
> > > 
> > > Reject @desclen values at or above KEY_MAX_DESC_SIZE with -EINVAL in order
> > > to match the userspace limit. The constraints inherits from the one byte
> > > required for the NULL character.
> > > 
> > > Cc: stable@vger.kernel.org
> > > Assisted-by: LLM
> > > Fixes: f771fde82051 ("keys: Simplify key description management")
> > > Reviewed-by: David Howells <dhowells@redhat.com>
> > > Signed-off-by: Jarkko Sakkinen <jarkko@kernel.org>
> > 
> > This is now the only one, which does not have keyutils script in the
> > commit message so in that sense I think these fixes look pretty good.
> 
> Sure, this fix is still incomplete though, as I mentioned.
> 
> Turns out Sashiko found this too; see the comment on
> https://sashiko.dev/#/patchset/20261020013903.1244157-1-jarkko%40kernel.org

I did not ignore what you said honestly but I overlooked generated
descriptions.

By comparing your v2 response and Sashiko's response and going through the
code this is the list that I have where similar check should be placed:

- key_alloc(): before quota calculation (in place)
- __key_create_or_update():  before assigning index_key.desc_len
- keyring_search(): before match_preparse
- request_key_and_link(): before match_preparse 
- request_key_rcu(): before check_cached_key()

Some sites might require some minor changes I just wrote the around the
ballpark locations.

Did I miss any site that you are aware of?

Because there are so many, despite simple check I also wonder, should
we have a macro for this check?

> 
> - Eric

Br, Jarkko

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v3 2/3] KEYS: Reject @desclen values >= KEY_MAX_DESC_SIZE in key_alloc()
  2026-10-20 10:57       ` Jarkko Sakkinen
@ 2026-10-20 13:00         ` Jarkko Sakkinen
  2026-10-20 15:50           ` Eric Biggers
  0 siblings, 1 reply; 11+ messages in thread
From: Jarkko Sakkinen @ 2026-10-20 13:00 UTC (permalink / raw)
  To: Eric Biggers
  Cc: David Howells, keyrings, stable, Paul Moore, James Morris,
	Serge E. Hallyn, open list, open list:SECURITY SUBSYSTEM

On Tue, Oct 20, 2026 at 01:57:19PM +0300, Jarkko Sakkinen wrote:
> > Sure, this fix is still incomplete though, as I mentioned.
> > 
> > Turns out Sashiko found this too; see the comment on
> > https://sashiko.dev/#/patchset/20261020013903.1244157-1-jarkko%40kernel.org
> 
> I did not ignore what you said honestly but I overlooked generated
> descriptions.
> 
> By comparing your v2 response and Sashiko's response and going through the
> code this is the list that I have where similar check should be placed:
> 
> - key_alloc(): before quota calculation (in place)
> - __key_create_or_update():  before assigning index_key.desc_len
> - keyring_search(): before match_preparse
> - request_key_and_link(): before match_preparse 
> - request_key_rcu(): before check_cached_key()
> 
> Some sites might require some minor changes I just wrote the around the
> ballpark locations.
> 
> Did I miss any site that you are aware of?
> 
> Because there are so many, despite simple check I also wonder, should
> we have a macro for this check?

It's better that I show the diff I'm talking about. With the diff below
I try to address all the sites I mentioned.

Couple of open questions:

- Would it make sense to wrap strlen + check into key_is_valid_desc()?
- Would it make sense as a safe-guard to use strnlen(), which is capped
  to KEY_MAX_DESC_SIZE?

diff --git a/include/linux/key.h b/include/linux/key.h
index 26139c3adf62..3b8af14fb323 100644
--- a/include/linux/key.h
+++ b/include/linux/key.h
@@ -37,6 +37,9 @@ struct net;

 #undef KEY_DEBUGGING

+/* Maximum length of a key description, including the trailing NUL */
+#define KEY_MAX_DESC_SIZE 4096
+
 #define KEY_POS_VIEW	0x01000000	/* possessor can view a key's attributes */
 #define KEY_POS_READ	0x02000000	/* possessor can read key payload / view keyring */
 #define KEY_POS_WRITE	0x04000000	/* possessor can update key payload / add link to keyring */
diff --git a/security/keys/key.c b/security/keys/key.c
index 6ebb8691551f..e88548f1a704 100644
--- a/security/keys/key.c
+++ b/security/keys/key.c
@@ -245,6 +245,8 @@ struct key *key_alloc(struct key_type *type, const char *desc,
 	}

 	desclen = strlen(desc);
+	if (desclen >= KEY_MAX_DESC_SIZE)
+		goto error;
 	quotalen = desclen + 1 + type->def_datalen;

 	/* get hold of the key tracking for this user */
@@ -829,6 +831,7 @@ static key_ref_t __key_create_or_update(key_ref_t keyring_ref,
 	key_ref_t key_ref;
 	int ret;
 	struct key_restriction *restrict_link = NULL;
+	size_t desclen;

 	/* look up the key type to see if it's one of the registered kernel
 	 * types */
@@ -869,7 +872,12 @@ static key_ref_t __key_create_or_update(key_ref_t keyring_ref,
 		if (!index_key.description)
 			goto error_free_prep;
 	}
-	index_key.desc_len = strlen(index_key.description);
+	desclen = strlen(index_key.description);
+	if (desclen >= KEY_MAX_DESC_SIZE) {
+		key_ref = ERR_PTR(-EINVAL);
+		goto error_free_prep;
+	}
+	index_key.desc_len = desclen;
 	key_set_index_key(&index_key);

 	ret = __key_link_lock(keyring, &index_key);
diff --git a/security/keys/keyctl.c b/security/keys/keyctl.c
index 36ffba0ebba6..d6b10fd67627 100644
--- a/security/keys/keyctl.c
+++ b/security/keys/keyctl.c
@@ -24,8 +24,6 @@
 #include <keys/request_key_auth-type.h>
 #include "internal.h"

-#define KEY_MAX_DESC_SIZE 4096
-
 static const unsigned char keyrings_capabilities[2] = {
 	[0] = (KEYCTL_CAPS0_CAPABILITIES |
 	       (IS_ENABLED(CONFIG_PERSISTENT_KEYRINGS)	? KEYCTL_CAPS0_PERSISTENT_KEYRINGS : 0) |
diff --git a/security/keys/keyring.c b/security/keys/keyring.c
index 7143f3079d20..2c54f7f84c55 100644
--- a/security/keys/keyring.c
+++ b/security/keys/keyring.c
@@ -957,7 +957,6 @@ key_ref_t keyring_search(key_ref_t keyring,
 	struct keyring_search_context ctx = {
 		.index_key.type		= type,
 		.index_key.description	= description,
-		.index_key.desc_len	= strlen(description),
 		.cred			= current_cred(),
 		.match_data.cmp		= key_default_cmp,
 		.match_data.raw_data	= description,
@@ -965,8 +964,14 @@ key_ref_t keyring_search(key_ref_t keyring,
 		.flags			= KEYRING_SEARCH_DO_STATE_CHECK,
 	};
 	key_ref_t key;
+	size_t desclen;
 	int ret;

+	desclen = strlen(description);
+	if (desclen >= KEY_MAX_DESC_SIZE)
+		return ERR_PTR(-EINVAL);
+	ctx.index_key.desc_len = desclen;
+
 	if (recurse)
 		ctx.flags |= KEYRING_SEARCH_RECURSE;
 	if (type->match_preparse) {
diff --git a/security/keys/request_key.c b/security/keys/request_key.c
index fa2bb9f2f538..ef00c2b08fe0 100644
--- a/security/keys/request_key.c
+++ b/security/keys/request_key.c
@@ -584,7 +584,6 @@ struct key *request_key_and_link(struct key_type *type,
 		.index_key.type		= type,
 		.index_key.domain_tag	= domain_tag,
 		.index_key.description	= description,
-		.index_key.desc_len	= strlen(description),
 		.cred			= current_cred(),
 		.match_data.cmp		= key_default_cmp,
 		.match_data.raw_data	= description,
@@ -595,8 +594,14 @@ struct key *request_key_and_link(struct key_type *type,
 	};
 	struct key *key;
 	key_ref_t key_ref;
+	size_t desclen;
 	int ret;

+	desclen = strlen(description);
+	if (desclen >= KEY_MAX_DESC_SIZE)
+		return ERR_PTR(-EINVAL);
+	ctx.index_key.desc_len = desclen;
+
 	kenter("%s,%s,%p,%zu,%p,%p,%lx",
 	       ctx.index_key.type->name, ctx.index_key.description,
 	       callout_info, callout_len, aux, dest_keyring, flags);
@@ -787,7 +792,6 @@ struct key *request_key_rcu(struct key_type *type,
 		.index_key.type		= type,
 		.index_key.domain_tag	= domain_tag,
 		.index_key.description	= description,
-		.index_key.desc_len	= strlen(description),
 		.cred			= current_cred(),
 		.match_data.cmp		= key_default_cmp,
 		.match_data.raw_data	= description,
@@ -797,6 +801,12 @@ struct key *request_key_rcu(struct key_type *type,
 	};
 	struct key *key;
 	key_ref_t key_ref;
+	size_t desclen;
+
+	desclen = strlen(description);
+	if (desclen >= KEY_MAX_DESC_SIZE)
+		return ERR_PTR(-EINVAL);
+	ctx.index_key.desc_len = desclen;

 	kenter("%s,%s", type->name, description);

Br, Jarkko

^ permalink raw reply related	[flat|nested] 11+ messages in thread

* Re: [PATCH v3 1/3] KEYS: Expand key->{datalen,quotalen} to unsigned int
  2026-10-20  1:38 ` [PATCH v3 1/3] KEYS: Expand key->{datalen,quotalen} to unsigned int Jarkko Sakkinen
@ 2026-10-20 15:47   ` Eric Biggers
  2026-10-20 15:56     ` Jarkko Sakkinen
  0 siblings, 1 reply; 11+ messages in thread
From: Eric Biggers @ 2026-10-20 15:47 UTC (permalink / raw)
  To: Jarkko Sakkinen
  Cc: David Howells, keyrings, stable, Paul Moore, James Morris,
	Serge E. Hallyn, open list, open list:SECURITY SUBSYSTEM

On Tue, Oct 20, 2026 at 04:38:55AM +0300, Jarkko Sakkinen wrote:
> diff --git a/include/linux/key.h b/include/linux/key.h
> index e40b5267658f..26139c3adf62 100644
> --- a/include/linux/key.h
> +++ b/include/linux/key.h
> @@ -216,8 +216,8 @@ struct key {
>  	kuid_t			uid;
>  	kgid_t			gid;
>  	key_perm_t		perm;		/* access permissions */
> -	unsigned short		quotalen;	/* length added to quota */
> -	unsigned short		datalen;	/* payload data length
> +	unsigned int		quotalen;	/* length added to quota */
> +	unsigned int		datalen;	/* payload data length
>  						 * - may not match RCU dereferenced payload
>  						 * - payload should contain own length
>  						 */

fs/smb/client/cifsacl.c contains some debug logging that prints
key::datalen using the %hu format specifier.  Those need to be updated
to %u to match the new type.

- Eric

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v3 2/3] KEYS: Reject @desclen values >= KEY_MAX_DESC_SIZE in key_alloc()
  2026-10-20 13:00         ` Jarkko Sakkinen
@ 2026-10-20 15:50           ` Eric Biggers
  2026-10-20 16:09             ` Jarkko Sakkinen
  0 siblings, 1 reply; 11+ messages in thread
From: Eric Biggers @ 2026-10-20 15:50 UTC (permalink / raw)
  To: Jarkko Sakkinen
  Cc: David Howells, keyrings, stable, Paul Moore, James Morris,
	Serge E. Hallyn, open list, open list:SECURITY SUBSYSTEM

On Tue, Oct 20, 2026 at 04:00:31PM +0300, Jarkko Sakkinen wrote:
> On Tue, Oct 20, 2026 at 01:57:19PM +0300, Jarkko Sakkinen wrote:
> > > Sure, this fix is still incomplete though, as I mentioned.
> > > 
> > > Turns out Sashiko found this too; see the comment on
> > > https://sashiko.dev/#/patchset/20261020013903.1244157-1-jarkko%40kernel.org
> > 
> > I did not ignore what you said honestly but I overlooked generated
> > descriptions.
> > 
> > By comparing your v2 response and Sashiko's response and going through the
> > code this is the list that I have where similar check should be placed:
> > 
> > - key_alloc(): before quota calculation (in place)
> > - __key_create_or_update():  before assigning index_key.desc_len
> > - keyring_search(): before match_preparse
> > - request_key_and_link(): before match_preparse 
> > - request_key_rcu(): before check_cached_key()
> > 
> > Some sites might require some minor changes I just wrote the around the
> > ballpark locations.
> > 
> > Did I miss any site that you are aware of?

It looks okay to me.

> - Would it make sense to wrap strlen + check into key_is_valid_desc()?

I don't think it would be very helpful.

> - Would it make sense as a safe-guard to use strnlen(), which is capped
>   to KEY_MAX_DESC_SIZE?

Not really needed.  They are NUL-terminated strings anyway.

- Eric

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v3 1/3] KEYS: Expand key->{datalen,quotalen} to unsigned int
  2026-10-20 15:47   ` Eric Biggers
@ 2026-10-20 15:56     ` Jarkko Sakkinen
  0 siblings, 0 replies; 11+ messages in thread
From: Jarkko Sakkinen @ 2026-10-20 15:56 UTC (permalink / raw)
  To: Eric Biggers
  Cc: David Howells, keyrings, stable, Paul Moore, James Morris,
	Serge E. Hallyn, open list, open list:SECURITY SUBSYSTEM

On Tue, Oct 20, 2026 at 03:47:14PM +0000, Eric Biggers wrote:
> On Tue, Oct 20, 2026 at 04:38:55AM +0300, Jarkko Sakkinen wrote:
> > diff --git a/include/linux/key.h b/include/linux/key.h
> > index e40b5267658f..26139c3adf62 100644
> > --- a/include/linux/key.h
> > +++ b/include/linux/key.h
> > @@ -216,8 +216,8 @@ struct key {
> >  	kuid_t			uid;
> >  	kgid_t			gid;
> >  	key_perm_t		perm;		/* access permissions */
> > -	unsigned short		quotalen;	/* length added to quota */
> > -	unsigned short		datalen;	/* payload data length
> > +	unsigned int		quotalen;	/* length added to quota */
> > +	unsigned int		datalen;	/* payload data length
> >  						 * - may not match RCU dereferenced payload
> >  						 * - payload should contain own length
> >  						 */
> 
> fs/smb/client/cifsacl.c contains some debug logging that prints
> key::datalen using the %hu format specifier.  Those need to be updated
> to %u to match the new type.

Ugh, my mistake i.e., we are talking about:

diff --git a/fs/smb/client/cifsacl.c b/fs/smb/client/cifsacl.c
index c5e47a835f99..de2acc7234be 100644
--- a/fs/smb/client/cifsacl.c
+++ b/fs/smb/client/cifsacl.c
@@ -385,7 +385,7 @@ id_to_sid(unsigned int cid, uint sidtype, struct smb_sid *ssid)
 		goto out_revert_creds;
 	} else if (sidkey->datalen < CIFS_SID_BASE_SIZE) {
 		rc = smb_EIO1(smb_eio_trace_malformed_sid_key, sidkey->datalen);
-		cifs_dbg(FYI, "%s: Downcall contained malformed key (datalen=%hu)\n",
+		cifs_dbg(FYI, "%s: Downcall contained malformed key (datalen=%u)\n",
 			 __func__, sidkey->datalen);
 		goto invalidate_key;
 	}
@@ -403,7 +403,7 @@ id_to_sid(unsigned int cid, uint sidtype, struct smb_sid *ssid)
 	if (ksid_size > sidkey->datalen) {
 		rc = smb_EIO2(smb_eio_trace_malformed_ksid_key,
 			      ksid_size, sidkey->datalen);
-		cifs_dbg(FYI, "%s: Downcall contained malformed key (datalen=%hu, ksid_size=%u)\n",
+		cifs_dbg(FYI, "%s: Downcall contained malformed key (datalen=%u, ksid_size=%u)\n",
 			 __func__, sidkey->datalen, ksid_size);
 		goto invalidate_key;
 	}
@@ -498,7 +498,7 @@ sid_to_id(struct cifs_sb_info *cifs_sb, struct smb_sid *psid,
 	 */
 	BUILD_BUG_ON(sizeof(uid_t) != sizeof(gid_t));
 	if (sidkey->datalen != sizeof(uid_t)) {
-		cifs_dbg(FYI, "%s: Downcall contained malformed key (datalen=%hu)\n",
+		cifs_dbg(FYI, "%s: Downcall contained malformed key (datalen=%u)\n",
 			 __func__, sidkey->datalen);
 		key_invalidate(sidkey);
 		goto out_key_put;

I'll include this as part of the patch exactly.

> 
> - Eric

Br, Jarkko

^ permalink raw reply related	[flat|nested] 11+ messages in thread

* Re: [PATCH v3 2/3] KEYS: Reject @desclen values >= KEY_MAX_DESC_SIZE in key_alloc()
  2026-10-20 15:50           ` Eric Biggers
@ 2026-10-20 16:09             ` Jarkko Sakkinen
  0 siblings, 0 replies; 11+ messages in thread
From: Jarkko Sakkinen @ 2026-10-20 16:09 UTC (permalink / raw)
  To: Eric Biggers
  Cc: David Howells, keyrings, stable, Paul Moore, James Morris,
	Serge E. Hallyn, open list, open list:SECURITY SUBSYSTEM

On Tue, Oct 20, 2026 at 03:50:13PM +0000, Eric Biggers wrote:
> On Tue, Oct 20, 2026 at 04:00:31PM +0300, Jarkko Sakkinen wrote:
> > On Tue, Oct 20, 2026 at 01:57:19PM +0300, Jarkko Sakkinen wrote:
> > > > Sure, this fix is still incomplete though, as I mentioned.
> > > > 
> > > > Turns out Sashiko found this too; see the comment on
> > > > https://sashiko.dev/#/patchset/20261020013903.1244157-1-jarkko%40kernel.org
> > > 
> > > I did not ignore what you said honestly but I overlooked generated
> > > descriptions.
> > > 
> > > By comparing your v2 response and Sashiko's response and going through the
> > > code this is the list that I have where similar check should be placed:
> > > 
> > > - key_alloc(): before quota calculation (in place)
> > > - __key_create_or_update():  before assigning index_key.desc_len
> > > - keyring_search(): before match_preparse
> > > - request_key_and_link(): before match_preparse 
> > > - request_key_rcu(): before check_cached_key()
> > > 
> > > Some sites might require some minor changes I just wrote the around the
> > > ballpark locations.
> > > 
> > > Did I miss any site that you are aware of?
> 
> It looks okay to me.
> 
> > - Would it make sense to wrap strlen + check into key_is_valid_desc()?
> 
> I don't think it would be very helpful.

I tend to agree.

> 
> > - Would it make sense as a safe-guard to use strnlen(), which is capped
> >   to KEY_MAX_DESC_SIZE?
> 
> Not really needed.  They are NUL-terminated strings anyway.

This is what I thought based on that in all of the sites I could
determine where the string is create/sourced.

Just wanted to sanity check so that this does not require too
many additional iterations.

> 
> - Eric

Br, Jarkko

^ permalink raw reply	[flat|nested] 11+ messages in thread

end of thread, other threads:[~2026-10-20 16:09 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
     [not found] <20261020013903.1244157-1-jarkko@kernel.org>
2026-10-20  1:38 ` [PATCH v3 1/3] KEYS: Expand key->{datalen,quotalen} to unsigned int Jarkko Sakkinen
2026-10-20 15:47   ` Eric Biggers
2026-10-20 15:56     ` Jarkko Sakkinen
2026-10-20  1:38 ` [PATCH v3 2/3] KEYS: Reject @desclen values >= KEY_MAX_DESC_SIZE in key_alloc() Jarkko Sakkinen
2026-10-20  1:47   ` Jarkko Sakkinen
2026-10-20  2:28     ` Eric Biggers
2026-10-20 10:57       ` Jarkko Sakkinen
2026-10-20 13:00         ` Jarkko Sakkinen
2026-10-20 15:50           ` Eric Biggers
2026-10-20 16:09             ` Jarkko Sakkinen
2026-10-20  1:38 ` [PATCH v3 3/3] KEYS: Verify the key identity in __key_unlink_begin Jarkko Sakkinen

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox