[PATCH v2] crypto: sa2ul - Fix stack overflow in sa_prepare_iopads

[PATCH v2] crypto: sa2ul - Fix stack overflow in sa_prepare_iopads

[Siddharth Vadapalli]

Using sa2ul for IPSec results in the following KASAN report:
  BUG: KASAN: stack-out-of-bounds in __crypto_sha256_export.isra.0+0x1ac/0x1c4
  Write of size 1 at addr ffff80008df96b08 by task charon-systemd/577
  [...]
  Call trace:
   [...]
   __crypto_sha256_export.isra.0+0x1ac/0x1c4
   crypto_sha256_export+0x14/0x24
   crypto_shash_export+0xe8/0x2e0
   sa_export_shash+0x40/0x110 [sa2ul]
   sa_prepare_iopads+0x264/0x4cc [sa2ul]
   sa_init_sc+0x838/0xa78 [sa2ul]
   sa_aead_setkey.constprop.0+0x3a8/0x6e8 [sa2ul]
   sa_aead_cbc_sha256_setkey+0xac/0xec [sa2ul]
   crypto_aead_setkey+0xa8/0x22c
   aead_geniv_setkey+0x34/0x60
   crypto_aead_setkey+0xa8/0x22c
   esp_init_authenc.constprop.0+0x4c0/0x810
   esp_init_state+0x27c/0x3e0
   [...]
  The buggy address belongs to stack of task charon-systemd/577
   and is located at offset 152 in frame:
   sa_prepare_iopads+0x0/0x4cc [sa2ul]
  This frame has 2 objects:
   [48, 152) 'sha'
   [192, 569) '__shash_desc'

Commit 3bf533787910 ("crypto: sha256 - Use the partial block API") added a
real ".export" function to the arch sha256 shash algorithm, changing its
export format to write "sizeof(struct __sha256_ctx) + 1" (105 bytes).

On the other hand, sa_prepare_iopads() passes a stack-allocated union as
the export destination buffer:
  union {
          struct sha1_state sha1;     /* 92 bytes */
	  struct sha256_state sha256; /* 104 bytes */
	  u8 k_pad[SHA1_BLOCK_SIZE];  /* 64 bytes */
  } sha;                              /* 104 bytes total */
with the size of the union being 104 bytes.

Since crypto_shash_export() writes 105 bytes into this 104-byte union, it
overflows by one byte into the adjacent stack frame.

Hence, fix this by replacing the fixed-size stack union with a heap
allocation of crypto_shash_statesize() bytes. Since the contents of the
allocated heap are written-to before they are read, a kmalloc() is safe.

While at it, verify that the size of the authentication key is within the
expected limit.

Fixes: 3bf533787910 ("crypto: sha256 - Use the partial block API")
Cc: <stable@vger.kernel.org>
Signed-off-by: Siddharth Vadapalli <s-vadapalli@ti.com>
---

Patch is based on commit
cee9395acd80 Linux 7.3-rc1
of Mainline Linux.

v1 of this patch is at:
https://lore.kernel.org/r/20260819142357.3950463-1-s-vadapalli@ti.com/

Changes since v1:
- Propagated error from sa_prepare_iopads throughout the call chain.
- Added missing check for authentication key size being within limits.
Above changes are based on Sashiko review of the v1 patch at:
https://sashiko.dev/#/patchset/20260819142357.3950463-1-s-vadapalli%40ti.com

Regards,
Siddharth.

 drivers/crypto/sa2ul.c | 63 +++++++++++++++++++++++++-----------------
 1 file changed, 37 insertions(+), 26 deletions(-)

diff --git a/drivers/crypto/sa2ul.c b/drivers/crypto/sa2ul.c
index 9846cbeb3449..13d54f269485 100644
--- a/drivers/crypto/sa2ul.c
+++ b/drivers/crypto/sa2ul.c
@@ -143,8 +143,8 @@ struct algo_data {
 	bool inv_key;
 	struct sa_tfm_ctx *ctx;
 	bool keyed_mac;
-	void (*prep_iopad)(struct algo_data *algo, const u8 *key,
-			   u16 key_sz, __be32 *ipad, __be32 *opad);
+	int (*prep_iopad)(struct algo_data *algo, const u8 *key,
+			  u16 key_sz, __be32 *ipad, __be32 *opad);
 };
 
 /**
@@ -433,34 +433,42 @@ static void sa_export_shash(void *state, struct shash_desc *hash,
 	cpu_to_be32_array(out, result, digest_size / 4);
 }
 
-static void sa_prepare_iopads(struct algo_data *data, const u8 *key,
-			      u16 key_sz, __be32 *ipad, __be32 *opad)
+static int sa_prepare_iopads(struct algo_data *data, const u8 *key,
+			     u16 key_sz, __be32 *ipad, __be32 *opad)
 {
 	SHASH_DESC_ON_STACK(shash, data->ctx->shash);
 	int block_size = crypto_shash_blocksize(data->ctx->shash);
 	int digest_size = crypto_shash_digestsize(data->ctx->shash);
-	union {
-		struct sha1_state sha1;
-		struct sha256_state sha256;
-		u8 k_pad[SHA1_BLOCK_SIZE];
-	} sha;
+	int state_size = crypto_shash_statesize(data->ctx->shash);
+	u8 *sha;
+
+	if (key_sz > block_size) {
+		dev_err(sa_k3_dev, "%s: key size: %u exceeds block size: %d\n",
+			__func__, key_sz, block_size);
+		return -EINVAL;
+	}
+
+	sha = kmalloc(state_size, GFP_KERNEL);
+	if (!sha)
+		return -ENOMEM;
 
 	shash->tfm = data->ctx->shash;
 
-	prepare_kipad(sha.k_pad, key, key_sz);
+	prepare_kipad(sha, key, key_sz);
 
 	crypto_shash_init(shash);
-	crypto_shash_update(shash, sha.k_pad, block_size);
-	sa_export_shash(&sha, shash, digest_size, ipad);
+	crypto_shash_update(shash, sha, block_size);
+	sa_export_shash(sha, shash, digest_size, ipad);
 
-	prepare_kopad(sha.k_pad, key, key_sz);
+	prepare_kopad(sha, key, key_sz);
 
 	crypto_shash_init(shash);
-	crypto_shash_update(shash, sha.k_pad, block_size);
+	crypto_shash_update(shash, sha, block_size);
+	sa_export_shash(sha, shash, digest_size, opad);
 
-	sa_export_shash(&sha, shash, digest_size, opad);
+	kfree_sensitive(sha);
 
-	memzero_explicit(&sha, sizeof(sha));
+	return 0;
 }
 
 /* Derive the inverse key used in AES-CBC decryption operation */
@@ -530,8 +538,8 @@ static int sa_set_sc_enc(struct algo_data *ad, const u8 *key, u16 key_sz,
 }
 
 /* Set Security context for the authentication engine */
-static void sa_set_sc_auth(struct algo_data *ad, const u8 *key, u16 key_sz,
-			   u8 *sc_buf)
+static int sa_set_sc_auth(struct algo_data *ad, const u8 *key, u16 key_sz,
+			  u8 *sc_buf)
 {
 	__be32 *ipad = (void *)(sc_buf + 32);
 	__be32 *opad = (void *)(sc_buf + 64);
@@ -544,11 +552,12 @@ static void sa_set_sc_auth(struct algo_data *ad, const u8 *key, u16 key_sz,
 
 	/* Copy the keys or ipad/opad */
 	if (ad->keyed_mac)
-		ad->prep_iopad(ad, key, key_sz, ipad, opad);
-	else {
-		/* basic hash */
-		sc_buf[1] |= SA_BASIC_HASH;
-	}
+		return ad->prep_iopad(ad, key, key_sz, ipad, opad);
+
+	/* basic hash */
+	sc_buf[1] |= SA_BASIC_HASH;
+
+	return 0;
 }
 
 static inline void sa_copy_iv(__be32 *out, const u8 *iv, bool size16)
@@ -763,9 +772,11 @@ int sa_init_sc(struct sa_ctx_info *ctx, const struct sa_match_data *match_data,
 	}
 
 	/* Prepare context for authentication engine */
-	if (ad->auth_eng.sc_size)
-		sa_set_sc_auth(ad, auth_key, auth_key_sz,
-			       &sc_buf[auth_sc_offset]);
+	if (ad->auth_eng.sc_size) {
+		if (sa_set_sc_auth(ad, auth_key, auth_key_sz,
+				   &sc_buf[auth_sc_offset]))
+			return -EINVAL;
+	}
 
 	/* Set the ownership of context to CP_ACE */
 	sc_buf[SA_CTX_SCCTL_OWNER_OFFSET] = 0x80;
-- 
2.51.1

Re: [PATCH v2] crypto: sa2ul - Fix stack overflow in sa_prepare_iopads

[Herbert Xu]

On Mon, Aug 31, 2026 at 11:21:38AM +0530, Siddharth Vadapalli wrote:
>
> Since crypto_shash_export() writes 105 bytes into this 104-byte union, it
> overflows by one byte into the adjacent stack frame.

This should be converted to crypto_shash_export_core which exports
just the hash state without the partial block buffer.
 
> Changes since v1:
> - Propagated error from sa_prepare_iopads throughout the call chain.
> - Added missing check for authentication key size being within limits.
> Above changes are based on Sashiko review of the v1 patch at:
> https://sashiko.dev/#/patchset/20260819142357.3950463-1-s-vadapalli%40ti.com

Looks like Sashiko is still not happy:

https://sashiko.dev/#/patchset/20260831055141.1632832-1-s-vadapalli%40ti.com

Let's split this up.  Just do a one-liner to switch shash_export
to shash_export_core, that should fix the stack overrun.  Then
fix the long key issue in a separate patch.

I think you should support long keys properly by hashing them as
crypto/hmac.c does.

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 v2] crypto: sa2ul - Fix stack overflow in sa_prepare_iopads

[Siddharth Vadapalli]

On 9/16/26 2:30 PM, Herbert Xu wrote:
> On Mon, Aug 31, 2026 at 11:21:38AM +0530, Siddharth Vadapalli wrote:
>>
>> Since crypto_shash_export() writes 105 bytes into this 104-byte union, it
>> overflows by one byte into the adjacent stack frame.
> 
> This should be converted to crypto_shash_export_core which exports
> just the hash state without the partial block buffer.
>   
>> Changes since v1:
>> - Propagated error from sa_prepare_iopads throughout the call chain.
>> - Added missing check for authentication key size being within limits.
>> Above changes are based on Sashiko review of the v1 patch at:
>> https://sashiko.dev/#/patchset/20260819142357.3950463-1-s-vadapalli%40ti.com
> 
> Looks like Sashiko is still not happy:
> 
> https://sashiko.dev/#/patchset/20260831055141.1632832-1-s-vadapalli%40ti.com
> 
> Let's split this up.  Just do a one-liner to switch shash_export
> to shash_export_core, that should fix the stack overrun.  Then
> fix the long key issue in a separate patch.
> 
> I think you should support long keys properly by hashing them as
> crypto/hmac.c does.

Thank you for reviewing the patch and providing your valuable feedback.

I will split the patch as you have suggested with the one-line fix patch 
posted first followed by an independent patch posted later for handling 
long keys.

Regards,
Siddharth.

Re: [PATCH v2] crypto: sa2ul - Fix stack overflow in sa_prepare_iopads

[Eric Biggers]

On Tue, Sep 22, 2026 at 06:33:51PM +0530, Siddharth Vadapalli wrote:
> On 9/16/26 2:30 PM, Herbert Xu wrote:
> > On Mon, Aug 31, 2026 at 11:21:38AM +0530, Siddharth Vadapalli wrote:
> > > 
> > > Since crypto_shash_export() writes 105 bytes into this 104-byte union, it
> > > overflows by one byte into the adjacent stack frame.
> > 
> > This should be converted to crypto_shash_export_core which exports
> > just the hash state without the partial block buffer.
> > > Changes since v1:
> > > - Propagated error from sa_prepare_iopads throughout the call chain.
> > > - Added missing check for authentication key size being within limits.
> > > Above changes are based on Sashiko review of the v1 patch at:
> > > https://sashiko.dev/#/patchset/20260819142357.3950463-1-s-vadapalli%40ti.com
> > 
> > Looks like Sashiko is still not happy:
> > 
> > https://sashiko.dev/#/patchset/20260831055141.1632832-1-s-vadapalli%40ti.com
> > 
> > Let's split this up.  Just do a one-liner to switch shash_export
> > to shash_export_core, that should fix the stack overrun.  Then
> > fix the long key issue in a separate patch.
> > 
> > I think you should support long keys properly by hashing them as
> > crypto/hmac.c does.
> 
> Thank you for reviewing the patch and providing your valuable feedback.
> 
> I will split the patch as you have suggested with the one-line fix patch
> posted first followed by an independent patch posted later for handling long
> keys.

https://lore.kernel.org/linux-crypto/20260923051116.92741-1-ebiggers@kernel.org
fixes both bugs properly by using the crypto library instead.  There's
really no need to even consider workarounds using crypto_shash here.

- Eric