Re: [dm-devel] [PATCH v4 4/6] md: dm-crypt: switch to ESSIV crypto API template

[Milan Broz <gmazyland@gmail.com>]

On 21/06/2019 10:09, Ard Biesheuvel wrote:
> From: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> 
> Replace the explicit ESSIV handling in the dm-crypt driver with calls
> into the crypto API, which now possesses the capability to perform
> this processing within the crypto subsystem.

I tried a few crazy dm-crypt configurations and was not able to crash it this time :-)

So, it definitely need some more testing, but for now, I think it works.

Few comments below for this part:

> --- a/drivers/md/dm-crypt.c
> +++ b/drivers/md/dm-crypt.c

>  static const struct crypt_iv_operations crypt_iv_benbi_ops = {
>  	.ctr	   = crypt_iv_benbi_ctr,
>  	.dtr	   = crypt_iv_benbi_dtr,
> @@ -2283,7 +2112,7 @@ static int crypt_ctr_ivmode(struct dm_target *ti, const char *ivmode)
>  	else if (strcmp(ivmode, "plain64be") == 0)
>  		cc->iv_gen_ops = &crypt_iv_plain64be_ops;
>  	else if (strcmp(ivmode, "essiv") == 0)
> -		cc->iv_gen_ops = &crypt_iv_essiv_ops;
> +		cc->iv_gen_ops = &crypt_iv_plain64_ops;

This is quite misleading - it looks like you are switching to plain64 here.
The reality is that it uses plain64 to feed the ESSIV wrapper.

So either it need some comment to explain it here, or just keep simple essiv_iv_ops
and duplicate that plain64 generator (it is 2 lines of code).

For the clarity, I would prefer the second variant (duplicate ops) here.

> @@ -2515,8 +2357,18 @@ static int crypt_ctr_cipher_old(struct dm_target *ti, char *cipher_in, char *key
>  	if (!cipher_api)
>  		goto bad_mem;
>  
> -	ret = snprintf(cipher_api, CRYPTO_MAX_ALG_NAME,
> -		       "%s(%s)", chainmode, cipher);
> +	if (*ivmode && !strcmp(*ivmode, "essiv")) {
> +		if (!*ivopts) {
> +			ti->error = "Digest algorithm missing for ESSIV mode";
> +			return -EINVAL;
> +		}
> +		ret = snprintf(cipher_api, CRYPTO_MAX_ALG_NAME,
> +			       "essiv(%s(%s),%s,%s)", chainmode, cipher,
> +			       cipher, *ivopts);

This becomes quite long string already (limit is now 128 bytes), we should probably
check also for too long string. It will perhaps fail later, but I would better add

	if (ret < 0 || ret >= CRYPTO_MAX_ALG_NAME) {
	...

> +	} else {
> +		ret = snprintf(cipher_api, CRYPTO_MAX_ALG_NAME,
> +			       "%s(%s)", chainmode, cipher);
> +	}
>  	if (ret < 0) {
>  		kfree(cipher_api);
>  		goto bad_mem;
> 

Thanks,
Milan

--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel