crypto: asymmetric_keys - prevent overflow in asymmetric_key_generate_id

[PATCH v3] crypto: asymmetric_keys - prevent overflow in asymmetric_key_generate_id

Posted by Thorsten Blum 3 months, 3 weeks ago

Use check_add_overflow() to guard against potential integer overflows
when adding the binary blob lengths and the size of an asymmetric_key_id
structure and return ERR_PTR(-EOVERFLOW) accordingly. This prevents a
possible buffer overflow when copying data from potentially malicious
X.509 certificate fields that can be arbitrarily large, such as ASN.1
INTEGER serial numbers, issuer names, etc.

Fixes: 7901c1a8effb ("KEYS: Implement binary asymmetric key ID handling")
Signed-off-by: Thorsten Blum <thorsten.blum@linux.dev>
---
Changes in v3:
- Also use check_add_overflow() for adding the struct size itself as
  suggested by Lukas
- Drop struct_size()
- Update patch description
- Drop patch 2/2
- Link to v2: https://lore.kernel.org/lkml/20251012203841.60230-1-thorsten.blum@linux.dev/

Changes in v2:
- Use check_add_overflow() and error out as suggested by Lukas
- Update patch description
- Add Fixes: tag and @stable for backporting
- Link to v1: https://lore.kernel.org/lkml/20251007185220.234611-2-thorsten.blum@linux.dev/
---
 crypto/asymmetric_keys/asymmetric_type.c | 14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)

diff --git a/crypto/asymmetric_keys/asymmetric_type.c b/crypto/asymmetric_keys/asymmetric_type.c
index ba2d9d1ea235..348966ea2175 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,17 @@ struct asymmetric_key_id *asymmetric_key_generate_id(const void *val_1,
 						     size_t len_2)
 {
 	struct asymmetric_key_id *kid;
-
-	kid = kmalloc(sizeof(struct asymmetric_key_id) + len_1 + len_2,
-		      GFP_KERNEL);
+	size_t kid_sz;
+	size_t len;
+
+	if (check_add_overflow(len_1, len_2, &len))
+		return ERR_PTR(-EOVERFLOW);
+	if (check_add_overflow(sizeof(struct asymmetric_key_id), len, &kid_sz))
+		return ERR_PTR(-EOVERFLOW);
+	kid = kmalloc(kid_sz, 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

Re: [PATCH v3] crypto: asymmetric_keys - prevent overflow in asymmetric_key_generate_id

Posted by Herbert Xu 3 months, 2 weeks ago

On Mon, Oct 13, 2025 at 01:40:10PM +0200, Thorsten Blum wrote:
> Use check_add_overflow() to guard against potential integer overflows
> when adding the binary blob lengths and the size of an asymmetric_key_id
> structure and return ERR_PTR(-EOVERFLOW) accordingly. This prevents a
> possible buffer overflow when copying data from potentially malicious
> X.509 certificate fields that can be arbitrarily large, such as ASN.1
> INTEGER serial numbers, issuer names, etc.
> 
> Fixes: 7901c1a8effb ("KEYS: Implement binary asymmetric key ID handling")
> Signed-off-by: Thorsten Blum <thorsten.blum@linux.dev>
> ---
> Changes in v3:
> - Also use check_add_overflow() for adding the struct size itself as
>   suggested by Lukas
> - Drop struct_size()
> - Update patch description
> - Drop patch 2/2
> - Link to v2: https://lore.kernel.org/lkml/20251012203841.60230-1-thorsten.blum@linux.dev/
> 
> Changes in v2:
> - Use check_add_overflow() and error out as suggested by Lukas
> - Update patch description
> - Add Fixes: tag and @stable for backporting
> - Link to v1: https://lore.kernel.org/lkml/20251007185220.234611-2-thorsten.blum@linux.dev/
> ---
>  crypto/asymmetric_keys/asymmetric_type.c | 14 ++++++++++----
>  1 file changed, 10 insertions(+), 4 deletions(-)

Patch applied.  Thanks.
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCH v3] crypto: asymmetric_keys - prevent overflow in asymmetric_key_generate_id

Posted by Lukas Wunner 3 months, 3 weeks ago

On Mon, Oct 13, 2025 at 01:40:10PM +0200, Thorsten Blum wrote:
> Use check_add_overflow() to guard against potential integer overflows
> when adding the binary blob lengths and the size of an asymmetric_key_id
> structure and return ERR_PTR(-EOVERFLOW) accordingly. This prevents a
> possible buffer overflow when copying data from potentially malicious
> X.509 certificate fields that can be arbitrarily large, such as ASN.1
> INTEGER serial numbers, issuer names, etc.
> 
> Fixes: 7901c1a8effb ("KEYS: Implement binary asymmetric key ID handling")
> Signed-off-by: Thorsten Blum <thorsten.blum@linux.dev>

Reviewed-by: Lukas Wunner <lukas@wunner.de>

Re: [PATCH v3] crypto: asymmetric_keys - prevent overflow in asymmetric_key_generate_id

Posted by Thorsten Blum 3 months, 2 weeks ago

Hi Lukas,

On 13. Oct 2025, at 17:39, Lukas Wunner wrote:
> On Mon, Oct 13, 2025 at 01:40:10PM +0200, Thorsten Blum wrote:
>> Use check_add_overflow() to guard against potential integer overflows
>> when adding the binary blob lengths and the size of an asymmetric_key_id
>> structure and return ERR_PTR(-EOVERFLOW) accordingly. This prevents a
>> possible buffer overflow when copying data from potentially malicious
>> X.509 certificate fields that can be arbitrarily large, such as ASN.1
>> INTEGER serial numbers, issuer names, etc.
>> 
>> Fixes: 7901c1a8effb ("KEYS: Implement binary asymmetric key ID handling")
>> Signed-off-by: Thorsten Blum <thorsten.blum@linux.dev>
> 
> Reviewed-by: Lukas Wunner <lukas@wunner.de>

Thank you for your review.

I removed stable@ after your feedback to v2, but shouldn't v3 be applied
to stable as well?

Best,
Thorsten

Re: [PATCH v3] crypto: asymmetric_keys - prevent overflow in asymmetric_key_generate_id

Posted by Lukas Wunner 3 months, 2 weeks ago

On Wed, Oct 22, 2025 at 02:23:02PM +0200, Thorsten Blum wrote:
> On 13. Oct 2025, at 17:39, Lukas Wunner wrote:
> > On Mon, Oct 13, 2025 at 01:40:10PM +0200, Thorsten Blum wrote:
> >> Use check_add_overflow() to guard against potential integer overflows
> >> when adding the binary blob lengths and the size of an asymmetric_key_id
> >> structure and return ERR_PTR(-EOVERFLOW) accordingly. This prevents a
> >> possible buffer overflow when copying data from potentially malicious
> >> X.509 certificate fields that can be arbitrarily large, such as ASN.1
> >> INTEGER serial numbers, issuer names, etc.
> >> 
> >> Fixes: 7901c1a8effb ("KEYS: Implement binary asymmetric key ID handling")
> >> Signed-off-by: Thorsten Blum <thorsten.blum@linux.dev>
> 
> I removed stable@ after your feedback to v2, but shouldn't v3 be applied
> to stable as well?

The Fixes tag you included implicitly serves as a stable tag.
It's usually sufficient reason for stable maintainers to select
the patch for backporting to stable kernels.

I'm always a bit cautious with stable designations because
if the patch turns out to be buggy, we broke the stable kernels as well,
which is bad and embarrassing.

In this particular case, the patch is fine but the bug doesn't look
easy to trigger.  One would have to craft an extremely large certificate.
Possible, but not very common.  Hence it doesn't seem super important
to get this fixed in stable kernels and for this reason I wouldn't have
included a Fixes tag if this was my patch.

Thanks,

Lukas