* [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
* 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 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
* [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
* 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 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 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
* [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
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