Received-SPF: pass (google.com: domain of linux-crypto-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Thu, 1 Jun 2023 15:10:18 -0400
From:   Mike Snitzer <snitzer@kernel.org>
To:     Herbert Xu <herbert@gondor.apana.org.au>
Cc:     Alasdair Kergon <agk@redhat.com>, dm-devel@redhat.com,
        Linux Crypto Mailing List <linux-crypto@vger.kernel.org>
Subject: Re: dm crypt: Avoid using MAX_CIPHER_BLOCKSIZE
Message-ID: <ZHjtGvf+gHxeV83V@redhat.com>
References: <ZHhbL+SbWRnTW4b7@gondor.apana.org.au>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <ZHhbL+SbWRnTW4b7@gondor.apana.org.au>
Precedence: bulk

On Thu, Jun 01 2023 at  4:47P -0400,
Herbert Xu <herbert@gondor.apana.org.au> wrote:

> MAX_CIPHER_BLOCKSIZE is an internal implementation detail and should
> not be relied on by users of the Crypto API.
> 
> Instead of storing the IV on the stack, allocate it together with
> the crypto request.
> 
> Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>
> ---
> 
>  drivers/md/dm-crypt.c |   15 +++++++++++----
>  1 file changed, 11 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/md/dm-crypt.c b/drivers/md/dm-crypt.c
> index 40cb1719ae4d..0e7e443dde11 100644
> --- a/drivers/md/dm-crypt.c
> +++ b/drivers/md/dm-crypt.c
> @@ -31,10 +31,10 @@
>  #include <asm/unaligned.h>
>  #include <crypto/hash.h>
>  #include <crypto/md5.h>
> -#include <crypto/algapi.h>
>  #include <crypto/skcipher.h>
>  #include <crypto/aead.h>
>  #include <crypto/authenc.h>
> +#include <crypto/utils.h>
>  #include <linux/rtnetlink.h> /* for struct rtattr and RTA macros only */
>  #include <linux/key-type.h>
>  #include <keys/user-type.h>
> @@ -743,16 +743,23 @@ static int crypt_iv_eboiv_ctr(struct crypt_config *cc, struct dm_target *ti,
>  static int crypt_iv_eboiv_gen(struct crypt_config *cc, u8 *iv,
>  			    struct dm_crypt_request *dmreq)
>  {
> -	u8 buf[MAX_CIPHER_BLOCKSIZE] __aligned(__alignof__(__le64));
> +	struct crypto_skcipher *tfm = any_tfm(cc);
>  	struct skcipher_request *req;
>  	struct scatterlist src, dst;
>  	DECLARE_CRYPTO_WAIT(wait);
> +	unsigned int reqsize;
>  	int err;
> +	u8 *buf;
>  
> -	req = skcipher_request_alloc(any_tfm(cc), GFP_NOIO);
> +	reqsize = ALIGN(crypto_skcipher_reqsize(tfm), __alignof__(__le64));
> +
> +	req = kmalloc(reqsize + cc->iv_size, GFP_NOIO);
>  	if (!req)
>  		return -ENOMEM;
>  
> +	skcipher_request_set_tfm(req, tfm);
> +
> +	buf = (u8 *)req + reqsize;
>  	memset(buf, 0, cc->iv_size);
>  	*(__le64 *)buf = cpu_to_le64(dmreq->iv_sector * cc->sector_size);
>  
> @@ -761,7 +768,7 @@ static int crypt_iv_eboiv_gen(struct crypt_config *cc, u8 *iv,
>  	skcipher_request_set_crypt(req, &src, &dst, cc->iv_size, buf);
>  	skcipher_request_set_callback(req, 0, crypto_req_done, &wait);
>  	err = crypto_wait_req(crypto_skcipher_encrypt(req), &wait);
> -	skcipher_request_free(req);
> +	kfree_sensitive(req);
>  
>  	return err;
>  }


Strikes me as strange that open-coding skcipher_request_{alloc,free}
is ideal, but dm-crypt is the only non-crypto consumer of
MAX_CIPHER_BLOCKSIZE so really not worth standing up yet another
interface wrapper.

Anyway, this code is certainly better for dm-crypt's needs.  I'm happy
with you applying this change via your crypto tree.

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

Thanks.