OOPs in 6.16-rc2 crypto_shash_export due to partial block handling

PATCH In review Milan Broz 4 participants 9 messages Jun 19, 2025 – Jun 24, 2025

Subsystems (2)

Crypto API Device mapper

lore

Share & tools

lore

Milan Broz

author

2025-06-19 21:17 UTC #0 lore raw

Hi Herbert,

there is an apparent regression in recent 6.16-rc2.

I can easily crash the kernel on 32bit machine with this OOPS:

: Oops: Oops: 0000 [#1] SMP
: CPU: 1 UID: 0 PID: 12 Comm: kworker/u16:0 Not tainted 6.16.0-rc2+ #993 PREEMPT(full)
: Hardware name: VMware, Inc. VMware Virtual Platform/440BX Desktop Reference Platform, BIOS 6.00 11/12/2020
: Workqueue: kcryptd-254:0-1 kcryptd_crypt [dm_crypt]
: EIP: __crypto_shash_export+0xf/0x90
: Code: 4a c1 c7 40 20 a0 b4 4a c1 81 cf 0e 00 04 08 89 78 50 e9 2b ff ff ff 8d 74 26 00 55 89 e5 57 56 53 89 c3 89 d6 8b 00 8b 40 14 <8b> 50 fc f6 40 13 01 74 04 4a 2b 50 14 85 c9 74 10 89 f2 89 d8 ff : EAX: 303a3435 EBX: c3007c90 ECX: 00000000 EDX: c3007c38
: ESI: c3007c38 EDI: c3007c90 EBP: c3007bfc ESP: c3007bf0
: DS: 007b ES: 007b FS: 00d8 GS: 0000 SS: 0068 EFLAGS: 00010216
: CR0: 80050033 CR2: 303a3431 CR3: 04fbe000 CR4: 00350e90
: Call Trace:
: crypto_shash_export+0x65/0xc0
: crypt_iv_lmk_one+0x106/0x1a0 [dm_crypt]
...

The bisect points to

commit 8cf4c341f1931c20c564ab2ee0f9eb990a606cac

Author: Herbert Xu <[email protected]>
Date:   Fri Apr 18 10:59:04 2025 +0800

crypto: md5-generic - Use API partial block handling

Use the Crypto API partial block handling.

I think there is a buffer overflow in crypto_shash_export, it does not crash on 64bit perhaps because of different alignment, but I can be mistaken.

As plen is blocksize + 1, this line in crypto_shash_export seems write out of m5 state:

  unsigned int plen = crypto_shash_blocksize(tfm) + 1;
  ...
  memcpy(out + ss - plen, buf + descsize - plen, plen);

It is easily reproducible with cryptsetup testuite script tests/loopaes-test (on 32bit system).

Let me know if you need more info.

Milan

Herbert Xu

maintainer

2025-06-20 04:09 UTC #1 lore raw

dm-crypt: Extend state buffer size in crypt_iv_lmk_one

Quoted 4 lines · Milan Broz

: Call Trace:
: crypto_shash_export+0x65/0xc0
: crypt_iv_lmk_one+0x106/0x1a0 [dm_crypt]

Thanks for the report. The problem is that crypt_iv_lmk_one is calling crypto_shash_export using a buffer that is less than the size as returned by crypto_shash_statesize.

The easiest fix is to expand the buffer to HASH_MAX_STATESIZE:

---8<---
The output buffer size of of crypto_shash_export is returned by crypto_shash_statesize. Alternatively HASH_MAX_STATESIZE may be used for stack buffers.

Fixes 8cf4c341f193 ("crypto: md5-generic - Use API partial block handling")

Reported-by Milan Broz <[email protected]>

Signed-off-by Herbert Xu <[email protected]>

drivers/md/dm-crypt.c +7 -4

index 9dfdb63220d7..cb4617df7356 100644
@@ -517,7 +517,10 @@ static int crypt_iv_lmk_one(struct crypt_config *cc, u8 *iv,
 {
 	struct iv_lmk_private *lmk = &cc->iv_gen_private.lmk;
 	SHASH_DESC_ON_STACK(desc, lmk->hash_tfm);
-	struct md5_state md5state;
+	union {
+		struct md5_state md5state;
+		u8 state[HASH_MAX_STATESIZE];
+	} u;
 	__le32 buf[4];
 	int i, r;

@@ -548,13 +551,13 @@ static int crypt_iv_lmk_one(struct crypt_config *cc, u8 *iv,
 		return r;
 
 	/* No MD5 padding here */
-	r = crypto_shash_export(desc, &md5state);
+	r = crypto_shash_export(desc, &u.md5state);
 	if (r)
 		return r;
 
 	for (i = 0; i < MD5_HASH_WORDS; i++)
-		__cpu_to_le32s(&md5state.hash[i]);
-	memcpy(iv, &md5state.hash, cc->iv_size);
+		__cpu_to_le32s(&u.md5state.hash[i]);
+	memcpy(iv, &u.md5state.hash, cc->iv_size);
 
 	return 0;
 }

Signature · 3 lines

Email: Herbert Xu <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Milan Broz

author Tested-by

2025-06-20 08:04 UTC #2 lore raw

Re: dm-crypt: Extend state buffer size in crypt_iv_lmk_one

Hi,

Quoted 7 lines · Herbert Xu

The output buffer size of of crypto_shash_export is returned by crypto_shash_statesize. Alternatively HASH_MAX_STATESIZE may be used for stack buffers.
Fixes 8cf4c341f193 ("crypto: md5-generic - Use API partial block handling")
Reported-by Milan Broz <[email protected]>
Signed-off-by Herbert Xu <[email protected]>

Yes, that fixes the issue, thanks!

Tested-by Milan Broz <[email protected]>

Mikulas, I think this should go through DM tree, could you send it for 6.16? The full patch is here
https://lore.kernel.org/linux-crypto/[email protected]/T/#u

drivers/md/dm-crypt.c static int crypt_iv_lmk_one(struct crypt_config *cc, u8 *iv, · line 523

Quoted 13 lines

drivers/md/dm-crypt.c +4 -1

index 9dfdb63220d7..cb4617df7356 100644
@@ -517,7 +517,10 @@ static int crypt_iv_lmk_one(struct crypt_config *cc, u8 *iv,
  {
  	struct iv_lmk_private *lmk = &cc->iv_gen_private.lmk;
  	SHASH_DESC_ON_STACK(desc, lmk->hash_tfm);
-	struct md5_state md5state;
+	union {
+		struct md5_state md5state;
+		u8 state[HASH_MAX_STATESIZE];
+	} u;

I am ok with it, as this is an obscure IV case for old devices compatibility, but I would kind of expected the algorithm state struct is self-contained...

...

Thanks,
Milan

Mikulas Patocka

maintainer

2025-06-23 09:40 UTC #3 lore raw

Re: dm-crypt: Extend state buffer size in crypt_iv_lmk_one

Quoted 19 lines · Milan Broz

Hi,
Quoted 7 lines · Herbert Xu
The output buffer size of of crypto_shash_export is returned by crypto_shash_statesize. Alternatively HASH_MAX_STATESIZE may be used for stack buffers.
Fixes 8cf4c341f193 ("crypto: md5-generic - Use API partial block handling")
Reported-by Milan Broz <[email protected]>
Signed-off-by Herbert Xu <[email protected]>
Yes, that fixes the issue, thanks!
Tested-by Milan Broz <[email protected]>
Mikulas, I think this should go through DM tree, could you send it for 6.16? The full patch is here
https://lore.kernel.org/linux-crypto/[email protected]/T/#u

Quoted 14 lines

Quoted 5 lines

drivers/md/dm-crypt.c +0 -0
index 9dfdb63220d7..cb4617df7356 100644

drivers/md/dm-crypt.c static int crypt_iv_lmk_one(struct crypt_config *cc, u8 · line 524

Quoted 9 lines

drivers/md/dm-crypt.c +4 -1

@@ -517,7 +517,10 @@ static int crypt_iv_lmk_one(struct crypt_config *cc, u8
 *iv,
  {
  	struct iv_lmk_private *lmk = &cc->iv_gen_private.lmk;
  	SHASH_DESC_ON_STACK(desc, lmk->hash_tfm);
-	struct md5_state md5state;
+	union {
+		struct md5_state md5state;
+		u8 state[HASH_MAX_STATESIZE];
+	} u;

345 bytes on the stack - I think it's too much, given the fact that it already uses 345 bytes (from SHASH_DESC_ON_STACK) and it may be called in a tasklet context. I'd prefer a solution that allocates less bytes.

I don't see the beginning of this thread, so I'd like to ask what's the problem here, what algorithm other than md5 is used here that causes the buffer overflow?

Mikulas

Herbert Xu

maintainer

2025-06-23 11:11 UTC #4 lore raw

PATCH v2 1/1 dm-crypt: Extend state buffer size in crypt_iv_lmk_one

Quoted 8 lines · Mikulas Patocka

345 bytes on the stack - I think it's too much, given the fact that it already uses 345 bytes (from SHASH_DESC_ON_STACK) and it may be called in a tasklet context. I'd prefer a solution that allocates less bytes.
I don't see the beginning of this thread, so I'd like to ask what's the problem here, what algorithm other than md5 is used here that causes the buffer overflow?

The md5 export size has increased due to the partial block API thus triggering the overflow.

How about this patch?

---8<---
Add a macro CRYPTO_MD5_STATESIZE for the Crypto API export state size of md5 and use that in dm-crypt instead of relying on the size of struct md5_state (the latter is currently undergoing a transition and may shrink).

Fixes 8cf4c341f193 ("crypto: md5-generic - Use API partial block handling")

Reported-by Milan Broz <[email protected]>

Signed-off-by Herbert Xu <[email protected]>

drivers/md/dm-crypt.c +7 -4

index 9dfdb63220d7..17157c4216a5 100644
@@ -517,7 +517,10 @@ static int crypt_iv_lmk_one(struct crypt_config *cc, u8 *iv,
 {
 	struct iv_lmk_private *lmk = &cc->iv_gen_private.lmk;
 	SHASH_DESC_ON_STACK(desc, lmk->hash_tfm);
-	struct md5_state md5state;
+	union {
+		struct md5_state md5state;
+		u8 state[CRYPTO_MD5_STATESIZE];
+	} u;
 	__le32 buf[4];
 	int i, r;

@@ -548,13 +551,13 @@ static int crypt_iv_lmk_one(struct crypt_config *cc, u8 *iv,
 		return r;
 
 	/* No MD5 padding here */
-	r = crypto_shash_export(desc, &md5state);
+	r = crypto_shash_export(desc, &u.md5state);
 	if (r)
 		return r;
 
 	for (i = 0; i < MD5_HASH_WORDS; i++)
-		__cpu_to_le32s(&md5state.hash[i]);
-	memcpy(iv, &md5state.hash, cc->iv_size);
+		__cpu_to_le32s(&u.md5state.hash[i]);
+	memcpy(iv, &u.md5state.hash, cc->iv_size);
 
 	return 0;
 }

include/crypto/hash.h +2 -0

index 6f6b9de12cd3..db294d452e8c 100644
@@ -202,6 +202,8 @@ struct shash_desc {
 #define HASH_REQUEST_CLONE(name, gfp) \
 	hash_request_clone(name, sizeof(__##name##_req), gfp)
 
+#define CRYPTO_HASH_STATESIZE(coresize, blocksize) (coresize + blocksize + 1)
+
 /**
  * struct shash_alg - synchronous message digest definition
  * @init: see struct ahash_alg

include/crypto/md5.h +4 -0

index 198b5d69b92f..28ee533a0507 100644
@@ -2,6 +2,7 @@
 #ifndef _CRYPTO_MD5_H
 #define _CRYPTO_MD5_H
 
+#include <crypto/hash.h>
 #include <linux/types.h>
 
 #define MD5_DIGEST_SIZE		16

@@ -15,6 +16,9 @@
 #define MD5_H2	0x98badcfeUL
 #define MD5_H3	0x10325476UL
 
+#define CRYPTO_MD5_STATESIZE \
+	CRYPTO_HASH_STATESIZE(MD5_STATE_SIZE, MD5_HMAC_BLOCK_SIZE)
+
 extern const u8 md5_zero_message_hash[MD5_DIGEST_SIZE];
 
 struct md5_state {

Signature · 3 lines

Email: Herbert Xu <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Milan Broz

author Tested-by on v2 1/1

2025-06-23 11:55 UTC #5 lore raw

Re: [v2 PATCH] dm-crypt: Extend state buffer size in crypt_iv_lmk_one

Quoted 6 lines · Herbert Xu

Quoted 5 lines · Mikulas Patocka
345 bytes on the stack - I think it's too much, given the fact that it already uses 345 bytes (from SHASH_DESC_ON_STACK) and it may be called in a tasklet context. I'd prefer a solution that allocates less bytes.

Quoted 8 lines

Quoted 3 lines
I don't see the beginning of this thread, so I'd like to ask what's the problem here, what algorithm other than md5 is used here that causes the buffer overflow?
The md5 export size has increased due to the partial block API thus triggering the overflow.
How about this patch?

For v2:

Tested-by Milan Broz <[email protected]>

Just for the context, the patch fixes OOPS on 32bit (but 64bit should overflow struct too):

...

The bisect was

efd62c85525e212705368b24eb90ef10778190f5 is the first bad commit commit efd62c85525e212705368b24eb90ef10778190f5 (HEAD)

Author: Herbert Xu <[email protected]>
Date:   Fri Apr 18 10:59:04 2025 +0800

crypto: md5-generic - Use API partial block handling

Use the Crypto API partial block handling.

Milan

Mikulas Patocka

maintainer

2025-06-23 12:42 UTC #6 lore raw

Re: [v2 PATCH] dm-crypt: Extend state buffer size in crypt_iv_lmk_one

Quoted 7 lines · Milan Broz

Quoted 6 lines · Herbert Xu
Quoted 5 lines · Mikulas Patocka
345 bytes on the stack - I think it's too much, given the fact that it already uses 345 bytes (from SHASH_DESC_ON_STACK) and it may be called in a tasklet context. I'd prefer a solution that allocates less bytes.

Quoted 8 lines

Quoted 8 lines
Quoted 3 lines
I don't see the beginning of this thread, so I'd like to ask what's the problem here, what algorithm other than md5 is used here that causes the buffer overflow?
The md5 export size has increased due to the partial block API thus triggering the overflow.
How about this patch?

OK. I accepted the patch and committed it to the linux-dm git.

Mikulas

Quoted 40 lines

For v2:
Tested-by Milan Broz <[email protected]>
Just for the context, the patch fixes OOPS on 32bit (but 64bit should overflow struct too):
: Oops: Oops: 0000 [#1] SMP
: CPU: 1 UID: 0 PID: 12 Comm: kworker/u16:0 Not tainted 6.16.0-rc2+ #993 PREEMPT(full)
: Hardware name: VMware, Inc. VMware Virtual Platform/440BX Desktop Reference Platform, BIOS 6.00 11/12/2020
: Workqueue: kcryptd-254:0-1 kcryptd_crypt [dm_crypt]
: EIP: __crypto_shash_export+0xf/0x90
: Code: 4a c1 c7 40 20 a0 b4 4a c1 81 cf 0e 00 04 08 89 78 50 e9 2b ff ff ff 8d 74 26 00 55 89 e5 57 56 53 89 c3 89 d6 8b 00 8b 40 14 <8b> 50 fc f6 40 13 01 74 04 4a 2b 50 14 85 c9 74 10 89 f2 89 d8 ff
: EAX: 303a3435 EBX: c3007c90 ECX: 00000000 EDX: c3007c38
: ESI: c3007c38 EDI: c3007c90 EBP: c3007bfc ESP: c3007bf0
: DS: 007b ES: 007b FS: 00d8 GS: 0000 SS: 0068 EFLAGS: 00010216
: CR0: 80050033 CR2: 303a3431 CR3: 04fbe000 CR4: 00350e90
: Call Trace:
: crypto_shash_export+0x65/0xc0
: crypt_iv_lmk_one+0x106/0x1a0 [dm_crypt]
...
The bisect was
efd62c85525e212705368b24eb90ef10778190f5 is the first bad commit commit efd62c85525e212705368b24eb90ef10778190f5 (HEAD)
Author: Herbert Xu <[email protected]>
Date:   Fri Apr 18 10:59:04 2025 +0800
crypto: md5-generic - Use API partial block handling
Use the Crypto API partial block handling.
Milan

Eric Biggers

2025-06-23 18:22 UTC #7 lore raw

Re: dm-crypt: Extend state buffer size in crypt_iv_lmk_one

In reply to Mikulas Patocka

Quoted 40 lines · Mikulas Patocka

Quoted 19 lines · Milan Broz
Hi,
Quoted 7 lines · Herbert Xu
The output buffer size of of crypto_shash_export is returned by crypto_shash_statesize. Alternatively HASH_MAX_STATESIZE may be used for stack buffers.
Fixes 8cf4c341f193 ("crypto: md5-generic - Use API partial block handling")
Reported-by Milan Broz <[email protected]>
Signed-off-by Herbert Xu <[email protected]>
Yes, that fixes the issue, thanks!
Tested-by Milan Broz <[email protected]>
Mikulas, I think this should go through DM tree, could you send it for 6.16? The full patch is here
https://lore.kernel.org/linux-crypto/[email protected]/T/#u
Quoted 14 lines
Quoted 5 lines
drivers/md/dm-crypt.c +0 -0
index 9dfdb63220d7..cb4617df7356 100644
drivers/md/dm-crypt.c static int crypt_iv_lmk_one(struct crypt_config *cc, u8 · line 524
Quoted 9 lines
drivers/md/dm-crypt.c +4 -1
@@ -517,7 +517,10 @@ static int crypt_iv_lmk_one(struct crypt_config *cc, u8
 *iv,
  {
  	struct iv_lmk_private *lmk = &cc->iv_gen_private.lmk;
  	SHASH_DESC_ON_STACK(desc, lmk->hash_tfm);
-	struct md5_state md5state;
+	union {
+		struct md5_state md5state;
+		u8 state[HASH_MAX_STATESIZE];
+	} u;
Hi

Quoted 3 lines

345 bytes on the stack - I think it's too much, given the fact that it already uses 345 bytes (from SHASH_DESC_ON_STACK) and it may be called in a tasklet context. I'd prefer a solution that allocates less bytes.

Of course, the correct solution is to just add MD5 support to lib/crypto/ and use that here. All that's needed is a single MD5 context (88 bytes), and direct calls to the MD5 code...

- Eric

Milan Broz

author

2025-06-24 16:59 UTC #8 lore raw

Re: dm-crypt: Extend state buffer size in crypt_iv_lmk_one

Quoted 3 lines · Eric Biggers

Of course, the correct solution is to just add MD5 support to lib/crypto/ and use that here. All that's needed is a single MD5 context (88 bytes), and direct calls to the MD5 code...

Feel free to port dm-crypt to this code once MD5 is in lib ;-)

(Use of that small context was what I tried to do initially when implementing loopaes-compatible IV in dm-crypt.)

Milan