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

[Surachai Saiwong]

2562-06-24 14:05 GMT+07:00, Milan Broz <gmazyland at gmail.com>:
> On 21/06/2019 10:09, Ard Biesheuvel wrote:
>> From: Ard Biesheuvel <ard.biesheuvel at 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 at redhat.com
> https://www.redhat.com/mailman/listinfo/dm-devel
>