[PATCH 1/2] crypto: asymmetric_keys - prevent overflow in asymmetric_key_generate_id

[PATCH 1/2] crypto: asymmetric_keys - prevent overflow in asymmetric_key_generate_id

[Thorsten Blum]

Use size_add() to prevent a potential integer overflow when adding the
binary blob lengths in asymmetric_key_generate_id(), which could cause a
buffer overflow when copying the data using memcpy().

Use struct_size() to calculate the number of bytes to allocate for the
new asymmetric key id.

No functional changes.

Signed-off-by: Thorsten Blum <thorsten.blum@linux.dev>
---
 crypto/asymmetric_keys/asymmetric_type.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/crypto/asymmetric_keys/asymmetric_type.c b/crypto/asymmetric_keys/asymmetric_type.c
index ba2d9d1ea235..aea925c88973 100644
--- a/crypto/asymmetric_keys/asymmetric_type.c
+++ b/crypto/asymmetric_keys/asymmetric_type.c
@@ -11,6 +11,7 @@
 #include <crypto/public_key.h>
 #include <linux/seq_file.h>
 #include <linux/module.h>
+#include <linux/overflow.h>
 #include <linux/slab.h>
 #include <linux/ctype.h>
 #include <keys/system_keyring.h>
@@ -141,12 +142,13 @@ struct asymmetric_key_id *asymmetric_key_generate_id(const void *val_1,
 						     size_t len_2)
 {
 	struct asymmetric_key_id *kid;
+	size_t len;
 
-	kid = kmalloc(sizeof(struct asymmetric_key_id) + len_1 + len_2,
-		      GFP_KERNEL);
+	len = size_add(len_1, len_2);
+	kid = kmalloc(struct_size(kid, data, len), GFP_KERNEL);
 	if (!kid)
 		return ERR_PTR(-ENOMEM);
-	kid->len = len_1 + len_2;
+	kid->len = len;
 	memcpy(kid->data, val_1, len_1);
 	memcpy(kid->data + len_1, val_2, len_2);
 	return kid;
-- 
2.51.0

[PATCH 2/2] crypto: asymmetric_keys - simplify asymmetric_key_hex_to_key_id

[Thorsten Blum]

Use struct_size() to calculate the number of bytes to allocate for the
asymmetric key id.  Add a local variable to store the hex data length
instead of recalculating it.

The variable 'ret', whose name implies a return variable, is only used
to temporarily store the result of __asymmetric_key_hex_to_key_id(). Use
the result directly and remove the local variable.

No functional changes.

Signed-off-by: Thorsten Blum <thorsten.blum@linux.dev>
---
 crypto/asymmetric_keys/asymmetric_type.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/crypto/asymmetric_keys/asymmetric_type.c b/crypto/asymmetric_keys/asymmetric_type.c
index aea925c88973..b73b8b983bfa 100644
--- a/crypto/asymmetric_keys/asymmetric_type.c
+++ b/crypto/asymmetric_keys/asymmetric_type.c
@@ -228,7 +228,7 @@ struct asymmetric_key_id *asymmetric_key_hex_to_key_id(const char *id)
 {
 	struct asymmetric_key_id *match_id;
 	size_t asciihexlen;
-	int ret;
+	size_t hexlen;
 
 	if (!*id)
 		return ERR_PTR(-EINVAL);
@@ -236,12 +236,11 @@ struct asymmetric_key_id *asymmetric_key_hex_to_key_id(const char *id)
 	if (asciihexlen & 1)
 		return ERR_PTR(-EINVAL);
 
-	match_id = kmalloc(sizeof(struct asymmetric_key_id) + asciihexlen / 2,
-			   GFP_KERNEL);
+	hexlen = asciihexlen / 2;
+	match_id = kmalloc(struct_size(match_id, data, hexlen), GFP_KERNEL);
 	if (!match_id)
 		return ERR_PTR(-ENOMEM);
-	ret = __asymmetric_key_hex_to_key_id(id, match_id, asciihexlen / 2);
-	if (ret < 0) {
+	if (__asymmetric_key_hex_to_key_id(id, match_id, hexlen) < 0) {
 		kfree(match_id);
 		return ERR_PTR(-EINVAL);
 	}
-- 
2.51.0

Re: [PATCH 2/2] crypto: asymmetric_keys - simplify asymmetric_key_hex_to_key_id

[Lukas Wunner]

On Tue, Oct 07, 2025 at 08:52:21PM +0200, Thorsten Blum wrote:
> Use struct_size() to calculate the number of bytes to allocate for the
> asymmetric key id.

Why?  To what end?  To guard against an overflow?

I've gone through the callers of asymmetric_key_hex_to_key_id() and
it seems they all limit the length of the keyid.  Guarding against
an overflow in asymmetric_key_hex_to_key_id() could thus only be
justified as a defense-in-depth measure, but I doubt that's worth it.

Callers I've found:

- keyctl_keyring_search() [security/keys/keyctl.c]
    keyring_search()
      type->match_preparse() == asymmetric_key_match_preparse()
        asymmetric_key_hex_to_key_id()

  Here the size of the key id is constrained to 4096 bytes
  (KEY_MAX_DESC_SIZE) by keyctl_keyring_search().

- request_key() [security/keys/keyctl.c]
    request_key_and_link()
        type->match_preparse() == asymmetric_key_match_preparse()
          asymmetric_key_hex_to_key_id()

  Same here.

- asymmetric_verify() [security/integrity/digsig_asymmetric.c]
    request_asymmetric_key()
      find_asymmetric_key()
        keyring_search()
          type->match_preparse() == asymmetric_key_match_preparse()
            asymmetric_key_hex_to_key_id()

  Here the size of the key id is a 32-bit integer.

- pkcs7_validate_trust_one() [crypto/asymmetric_keys/pkcs7_trust.c]
    find_asymmetric_key()
      keyring_search()
        type->match_preparse() == asymmetric_key_match_preparse()
          asymmetric_key_hex_to_key_id()

  Here the key id in hexadecimal is constructed from its binary form,
  asymmetric_key_hex_to_key_id() then converts that back.  Naturally
  the back conversion shouldn't overflow.

- restrict_link_by_signature() [crypto/asymmetric_keys/restrict.c]
    find_asymmetric_key()
      keyring_search()
        type->match_preparse() == asymmetric_key_match_preparse()
          asymmetric_key_hex_to_key_id()

  Same here.

> +++ b/crypto/asymmetric_keys/asymmetric_type.c
> @@ -236,12 +236,11 @@ struct asymmetric_key_id *asymmetric_key_hex_to_key_id(const char *id)
>  	if (asciihexlen & 1)
>  		return ERR_PTR(-EINVAL);
>  
> -	match_id = kmalloc(sizeof(struct asymmetric_key_id) + asciihexlen / 2,
> -			   GFP_KERNEL);
> +	hexlen = asciihexlen / 2;
> +	match_id = kmalloc(struct_size(match_id, data, hexlen), GFP_KERNEL);
>  	if (!match_id)
>  		return ERR_PTR(-ENOMEM);

This doesn't look more readable to be honest.

> -	ret = __asymmetric_key_hex_to_key_id(id, match_id, asciihexlen / 2);
> -	if (ret < 0) {
> +	if (__asymmetric_key_hex_to_key_id(id, match_id, hexlen) < 0) {
>  		kfree(match_id);
>  		return ERR_PTR(-EINVAL);
>  	}

If anything, return ret instead of removing the ret variable.
The only negative return value of __asymmetric_key_hex_to_key_id()
is -EINVAL, hence that's returned directly here.

Thanks,

Lukas

Re: [PATCH 2/2] crypto: asymmetric_keys - simplify asymmetric_key_hex_to_key_id

[Thorsten Blum]

On 12. Oct 2025, at 14:10, Lukas Wunner wrote:
> On Tue, Oct 07, 2025 at 08:52:21PM +0200, Thorsten Blum wrote:
>> Use struct_size() to calculate the number of bytes to allocate for the
>> asymmetric key id.
> 
> Why?  To what end?  To guard against an overflow?

I find struct_size() to be more readable because it explicitly
communicates the relationship between the flexible array member 'data'
and 'asciihexlen / 2', which the open-coded version doesn't.

'sizeof(struct asymmetric_key_id) + asciihexlen / 2' works because the
flexible array 'data' is an unsigned char (1 byte). This will probably
never change, but struct_size() would still work even if it did change
to a data type that isn't exactly 1 byte.

Additionally, struct_size() has some extra compile-time checks (e.g.,
__must_be_array()).

>> -	ret = __asymmetric_key_hex_to_key_id(id, match_id, asciihexlen / 2);
>> -	if (ret < 0) {
>> +	if (__asymmetric_key_hex_to_key_id(id, match_id, hexlen) < 0) {
>> 		kfree(match_id);
>> 		return ERR_PTR(-EINVAL);
>> 	}
> 
> If anything, return ret instead of removing the ret variable.
> The only negative return value of __asymmetric_key_hex_to_key_id()
> is -EINVAL, hence that's returned directly here.

Ok, I'll change this in v2.

Thanks,
Thorsten

Re: [PATCH 2/2] crypto: asymmetric_keys - simplify asymmetric_key_hex_to_key_id

[Lukas Wunner]

On Sun, Oct 12, 2025 at 03:23:02PM +0200, Thorsten Blum wrote:
> On 12. Oct 2025, at 14:10, Lukas Wunner wrote:
> > On Tue, Oct 07, 2025 at 08:52:21PM +0200, Thorsten Blum wrote:
> > > Use struct_size() to calculate the number of bytes to allocate for the
> > > asymmetric key id.
> > 
> > Why?  To what end?  To guard against an overflow?
> 
> I find struct_size() to be more readable because it explicitly
> communicates the relationship between the flexible array member 'data'
> and 'asciihexlen / 2', which the open-coded version doesn't.
> 
> 'sizeof(struct asymmetric_key_id) + asciihexlen / 2' works because the
> flexible array 'data' is an unsigned char (1 byte). This will probably
> never change, but struct_size() would still work even if it did change
> to a data type that isn't exactly 1 byte.
> 
> Additionally, struct_size() has some extra compile-time checks (e.g.,
> __must_be_array()).

My view is that mere readability improvements (which are subjective)
and theoretical safety gains are not sufficient to justify a change.

In general submission of many small treewide refactoring changes in a
shotgun fashion is problematic because they occupy reviewers' and
maintainers' time (which is a scarce resource) and they mess up the
git history (complicating git blame), often for little to no gain.
I've heard one x86 maintainer say he considers them "more harm than good
nowadays".

In this particular case, the struct size calculation can never overflow
because even if asciihexlen has SIZE_MAX, the division by 2 ensures that
adding sizeof(struct asymmetric_key_id) doesn't cause an overflow.
So this patch doesn't seem to solve an actual problem.

Thanks,

Lukas

Re: [PATCH 1/2] crypto: asymmetric_keys - prevent overflow in asymmetric_key_generate_id

[Lukas Wunner]

On Tue, Oct 07, 2025 at 08:52:20PM +0200, Thorsten Blum wrote:
> +++ b/crypto/asymmetric_keys/asymmetric_type.c
> @@ -141,12 +142,13 @@ struct asymmetric_key_id *asymmetric_key_generate_id(const void *val_1,
>  						     size_t len_2)
>  {
>  	struct asymmetric_key_id *kid;
> +	size_t len;
>  
> -	kid = kmalloc(sizeof(struct asymmetric_key_id) + len_1 + len_2,
> -		      GFP_KERNEL);
> +	len = size_add(len_1, len_2);
> +	kid = kmalloc(struct_size(kid, data, len), GFP_KERNEL);
>  	if (!kid)
>  		return ERR_PTR(-ENOMEM);

This should error out on overflow, rather than continuing with a
SIZE_MAX length.  So how about using check_add_overflow() instead
of size_add() and returning -EOVERFLOW if that returns true?

asymmetric_key_generate_id() is invoked, among other things, with
the raw serial number from the X.509 certificate, which is an
ASN.1 INTEGER, which can be arbitrarily large.  (You may want to
mention that in the commit message.)  So checking for overflows
does seem to make sense to guard against maliciously crafted
certificates.

Thanks,

Lukas