[PATCH] dm crypt: Fix reqsize in crypt_iv_eboiv_gen

[PATCH] dm crypt: Fix reqsize in crypt_iv_eboiv_gen

[Herbert Xu]

On Fri, Oct 06, 2023 at 08:04:18AM +0700, Bagas Sanjaya wrote:
>
> > Git bisect lead me to:
> > # first bad commit: [e3023094dffb41540330fb0c74cd3a019cd525c2] dm crypt:
> > Avoid using MAX_CIPHER_BLOCKSIZE
> > 
> > If I git revert e3023094dffb41540330fb0c74cd3a019cd525c2 on current Linus'
> > git master, the issue goes away. So I'm personally not all that affected
> > anymore (if I'm ready to compile my kernels from now on), and I understand
> > that you have no clear way to reproduce this as it seems strongly bound to
> > hardware, but seems like this could point to a potentially serious security
> > issue since it involves both crypto and undefined behaviour.

Thanks for the report.  Sorry this is indeed my fault.  The allocated
buffer is too small as it's missing the size for the request object
itself.

Mike, would you be OK with me picking this fix up and pushing it to
Linus?

Cheers,

---8<---
A skcipher_request object is made up of struct skcipher_request
followed by a variable-sized trailer.  The allocation of the
skcipher_request and IV in crypt_iv_eboiv_gen is missing the
memory for struct skcipher_request.  Fix it by adding it to
reqsize.

Fixes: e3023094dffb ("dm crypt: Avoid using MAX_CIPHER_BLOCKSIZE")
Reported-by: Tatu Heikkil� <tatu.heikkila@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 f2662c21a6df..5315fd261c23 100644
--- a/drivers/md/dm-crypt.c
+++ b/drivers/md/dm-crypt.c
@@ -753,7 +753,8 @@ static int crypt_iv_eboiv_gen(struct crypt_config *cc, u8 *iv,
 	int err;
 	u8 *buf;
 
-	reqsize = ALIGN(crypto_skcipher_reqsize(tfm), __alignof__(__le64));
+	reqsize = sizeof(*req) + crypto_skcipher_reqsize(tfm);
+	reqsize = ALIGN(reqsize, __alignof__(__le64));
 
 	req = kmalloc(reqsize + cc->iv_size, GFP_NOIO);
 	if (!req)
-- 
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: Fix reqsize in crypt_iv_eboiv_gen

[Mike Snitzer]

On Thu, Oct 05 2023 at  9:41P -0400,
Herbert Xu <herbert@gondor.apana.org.au> wrote:

> On Fri, Oct 06, 2023 at 08:04:18AM +0700, Bagas Sanjaya wrote:
> >
> > > Git bisect lead me to:
> > > # first bad commit: [e3023094dffb41540330fb0c74cd3a019cd525c2] dm crypt:
> > > Avoid using MAX_CIPHER_BLOCKSIZE
> > > 
> > > If I git revert e3023094dffb41540330fb0c74cd3a019cd525c2 on current Linus'
> > > git master, the issue goes away. So I'm personally not all that affected
> > > anymore (if I'm ready to compile my kernels from now on), and I understand
> > > that you have no clear way to reproduce this as it seems strongly bound to
> > > hardware, but seems like this could point to a potentially serious security
> > > issue since it involves both crypto and undefined behaviour.
> 
> Thanks for the report.  Sorry this is indeed my fault.  The allocated
> buffer is too small as it's missing the size for the request object
> itself.
> 
> Mike, would you be OK with me picking this fix up and pushing it to
> Linus?
> 
> Cheers,
> 
> ---8<---
> A skcipher_request object is made up of struct skcipher_request
> followed by a variable-sized trailer.  The allocation of the
> skcipher_request and IV in crypt_iv_eboiv_gen is missing the
> memory for struct skcipher_request.  Fix it by adding it to
> reqsize.
> 
> Fixes: e3023094dffb ("dm crypt: Avoid using MAX_CIPHER_BLOCKSIZE")
> Reported-by: Tatu Heikkil� <tatu.heikkila@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 f2662c21a6df..5315fd261c23 100644
> --- a/drivers/md/dm-crypt.c
> +++ b/drivers/md/dm-crypt.c
> @@ -753,7 +753,8 @@ static int crypt_iv_eboiv_gen(struct crypt_config *cc, u8 *iv,
>  	int err;
>  	u8 *buf;
>  
> -	reqsize = ALIGN(crypto_skcipher_reqsize(tfm), __alignof__(__le64));
> +	reqsize = sizeof(*req) + crypto_skcipher_reqsize(tfm);
> +	reqsize = ALIGN(reqsize, __alignof__(__le64));
>  
>  	req = kmalloc(reqsize + cc->iv_size, GFP_NOIO);
>  	if (!req)

Sure, please do.  Shouldn't your header Cc: stable given that the
Fixes commit implies v6.5 needs this fix?

(sorry I missed this the first time I 'Reviewed-by' the original
commit)

Reviewed-by: Mike Mike Snitzer <snitzer@kernel.org>

Thanks,
Mike

Re: dm crypt: Fix reqsize in crypt_iv_eboiv_gen

[Herbert Xu]

On Thu, Oct 05, 2023 at 10:26:14PM -0400, Mike Snitzer wrote:
>
> Sure, please do.  Shouldn't your header Cc: stable given that the
> Fixes commit implies v6.5 needs this fix?

Sure I'll add it although it will be picked up automatically due
to the Fixes header.  I'll also fix the reporter's name.

> Reviewed-by: Mike Mike Snitzer <snitzer@kernel.org>

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: dm crypt: Fix reqsize in crypt_iv_eboiv_gen

[Linux regression tracking (Thorsten Leemhuis)]

On 06.10.23 04:33, Herbert Xu wrote:
> On Thu, Oct 05, 2023 at 10:26:14PM -0400, Mike Snitzer wrote:

BTW, Herbert, thx for taking care of this quickly!

>> Sure, please do.  Shouldn't your header Cc: stable given that the
>> Fixes commit implies v6.5 needs this fix?
> 
> Sure I'll add it although it will be picked up automatically due
> to the Fixes header.

FWIW, as some people don't know this: that might be the case, but there
is no guarantee, hence it's better to add it:

https://lore.kernel.org/all/2023060703-colony-shakily-3514@gregkh/

>  I'll also fix the reporter's name.

Side note: a Link: or Closes: tag to the report
(https://lore.kernel.org/all/f1b8d8f5-2079-537e-9d0f-d58da166fe50@gmail.com/
) would be nice as well.

Thx again. Ciao, Thorsten

Re: [PATCH] dm crypt: Fix reqsize in crypt_iv_eboiv_gen

[Tatu Heikkilä]

On Fri, Oct 06, 2023 at 09:41:55 +0800, Herbert Xu wrote:
>
> On Fri, Oct 06, 2023 at 08:04:18AM +0700, Bagas Sanjaya wrote:
> >
> > > Git bisect lead me to:
> > > # first bad commit: [e3023094dffb41540330fb0c74cd3a019cd525c2] dm crypt:
> > > Avoid using MAX_CIPHER_BLOCKSIZE
> > > 
> > > If I git revert e3023094dffb41540330fb0c74cd3a019cd525c2 on current Linus'
> > > git master, the issue goes away. So I'm personally not all that affected
> > > anymore (if I'm ready to compile my kernels from now on), and I understand
> > > that you have no clear way to reproduce this as it seems strongly bound to
> > > hardware, but seems like this could point to a potentially serious security
> > > issue since it involves both crypto and undefined behaviour.
> 
> Thanks for the report.  Sorry this is indeed my fault.  The allocated
> buffer is too small as it's missing the size for the request object
> itself.

Thank you for your prompt fix, I can access the volume without issue now. :-)
-Tatu