OOPs in 6.16-rc2 crypto_shash_export due to partial block handling

OOPs in 6.16-rc2 crypto_shash_export due to partial block handling

[Milan Broz]

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 <herbert@gondor.apana.org.au>
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

dm-crypt: Extend state buffer size in crypt_iv_lmk_one

[Herbert Xu]

On Thu, Jun 19, 2025 at 11:17:37PM +0200, Milan Broz wrote:
>
> : 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 <gmazyland@gmail.com>
Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>

diff --git a/drivers/md/dm-crypt.c b/drivers/md/dm-crypt.c
index 9dfdb63220d7..cb4617df7356 100644
--- a/drivers/md/dm-crypt.c
+++ b/drivers/md/dm-crypt.c
@@ -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;
 }
-- 
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: dm-crypt: Extend state buffer size in crypt_iv_lmk_one

[Milan Broz]

Hi,

On 6/20/25 6:09 AM, Herbert Xu wrote:
> 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 <gmazyland@gmail.com>
> Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>

Yes, that fixes the issue, thanks!

Tested-by: Milan Broz <gmazyland@gmail.com>

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/aFTe3kDZXCAzcwNq@gondor.apana.org.au/T/#u

> diff --git a/drivers/md/dm-crypt.c b/drivers/md/dm-crypt.c
> index 9dfdb63220d7..cb4617df7356 100644
> --- a/drivers/md/dm-crypt.c
> +++ b/drivers/md/dm-crypt.c
> @@ -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

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

[Mikulas Patocka]

On Fri, 20 Jun 2025, Milan Broz wrote:

> Hi,
> 
> On 6/20/25 6:09 AM, Herbert Xu wrote:
> > 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 <gmazyland@gmail.com>
> > Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>
> 
> Yes, that fixes the issue, thanks!
> 
> Tested-by: Milan Broz <gmazyland@gmail.com>
> 
> 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/aFTe3kDZXCAzcwNq@gondor.apana.org.au/T/#u
> 
> > diff --git a/drivers/md/dm-crypt.c b/drivers/md/dm-crypt.c
> > index 9dfdb63220d7..cb4617df7356 100644
> > --- a/drivers/md/dm-crypt.c
> > +++ b/drivers/md/dm-crypt.c
> > @@ -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

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

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

[Herbert Xu]

On Mon, Jun 23, 2025 at 11:40:39AM +0200, Mikulas Patocka wrote:
>
> 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 <gmazyland@gmail.com>
Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>

diff --git a/drivers/md/dm-crypt.c b/drivers/md/dm-crypt.c
index 9dfdb63220d7..17157c4216a5 100644
--- a/drivers/md/dm-crypt.c
+++ b/drivers/md/dm-crypt.c
@@ -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;
 }
diff --git a/include/crypto/hash.h b/include/crypto/hash.h
index 6f6b9de12cd3..db294d452e8c 100644
--- a/include/crypto/hash.h
+++ b/include/crypto/hash.h
@@ -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
diff --git a/include/crypto/md5.h b/include/crypto/md5.h
index 198b5d69b92f..28ee533a0507 100644
--- a/include/crypto/md5.h
+++ b/include/crypto/md5.h
@@ -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 {
-- 
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: [v2 PATCH] dm-crypt: Extend state buffer size in crypt_iv_lmk_one

[Milan Broz]

On 6/23/25 1:11 PM, Herbert Xu wrote:
> On Mon, Jun 23, 2025 at 11:40:39AM +0200, Mikulas Patocka wrote:
>>
>> 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?

For v2:

Tested-by: Milan Broz <gmazyland@gmail.com>

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 <herbert@gondor.apana.org.au>
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

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

[Mikulas Patocka]

On Mon, 23 Jun 2025, Milan Broz wrote:

> On 6/23/25 1:11 PM, Herbert Xu wrote:
> > On Mon, Jun 23, 2025 at 11:40:39AM +0200, Mikulas Patocka wrote:
> > > 
> > > 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?

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

Mikulas

> For v2:
> 
> Tested-by: Milan Broz <gmazyland@gmail.com>
> 
> 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 <herbert@gondor.apana.org.au>
> 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
> 

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

[Eric Biggers]

On Mon, Jun 23, 2025 at 11:40:39AM +0200, Mikulas Patocka wrote:
> 
> 
> On Fri, 20 Jun 2025, Milan Broz wrote:
> 
> > Hi,
> > 
> > On 6/20/25 6:09 AM, Herbert Xu wrote:
> > > 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 <gmazyland@gmail.com>
> > > Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>
> > 
> > Yes, that fixes the issue, thanks!
> > 
> > Tested-by: Milan Broz <gmazyland@gmail.com>
> > 
> > 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/aFTe3kDZXCAzcwNq@gondor.apana.org.au/T/#u
> > 
> > > diff --git a/drivers/md/dm-crypt.c b/drivers/md/dm-crypt.c
> > > index 9dfdb63220d7..cb4617df7356 100644
> > > --- a/drivers/md/dm-crypt.c
> > > +++ b/drivers/md/dm-crypt.c
> > > @@ -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
> 
> 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

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

[Milan Broz]

On 6/23/25 8:22 PM, Eric Biggers wrote:
> 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