diff mbox series

[2/4] dm crypt: Fix zoned block device support

Message ID 20210416030528.757513-3-damien.lemoal@wdc.com (mailing list archive)
State New
Headers show
Series Fix dm-crypt zoned block device support | expand

Commit Message

Damien Le Moal April 16, 2021, 3:05 a.m. UTC
Zone append BIOs (REQ_OP_ZONE_APPEND) always specify the start sector of
the zone to be written instead of the actual location sector to write.
The write location is determined by the device and returned to the host
upon completion of the operation. This interface, while simple and
efficient for writing into sequential zones of a zoned block device, is
incompatible with the use of sector values to calculate a cypher block
IV. All data written in a zone end up using the same IV values
corresponding to the first sectors of the zone, but read operation will
specify any sector within the zone, resulting in an IV mismatch between
encryption and decryption.

Using a single sector value (e.g. the zone start sector) for all read
and writes into a zone can solve this problem, but at the cost of
weakening the cypher chosen by the user. Instead, to solve this
problem, explicitly disable support for zone append operations using
the zone_append_not_supported field of struct dm_target if the IV mode
used is sector-based, that is for all IVs modes except null and random.

The cypher flag CRYPT_IV_NO_SECTORS iis introduced to indicate that the
cypher does not use sector values. This flag is set in
crypt_ctr_ivmode() for the null and random IV modes and checked in
crypt_ctr() to set to true zone_append_not_supported if
CRYPT_IV_NO_SECTORS is not set for the chosen cypher.

Reported-by: Shin'ichiro Kawasaki <shinichiro.kawasaki@wdc.com>
Signed-off-by: Damien Le Moal <damien.lemoal@wdc.com>
---
 drivers/md/dm-crypt.c | 48 +++++++++++++++++++++++++++++++++++--------
 1 file changed, 39 insertions(+), 9 deletions(-)

Comments

Johannes Thumshirn April 16, 2021, 7:13 a.m. UTC | #1
On 16/04/2021 05:05, Damien Le Moal wrote:

[...]

> +	CRYPT_IV_NO_SECTORS,		/* IV calculation does not use sectors */

[...]

> -	if (ivmode == NULL)
> +	if (ivmode == NULL) {
>  		cc->iv_gen_ops = NULL;
> -	else if (strcmp(ivmode, "plain") == 0)
> +		set_bit(CRYPT_IV_NO_SECTORS, &cc->cipher_flags);
> +	} else if (strcmp(ivmode, "plain") == 0)

[...]

> +		if (!test_bit(CRYPT_IV_NO_SECTORS, &cc->cipher_flags)) {
> +			DMWARN("Zone append is not supported with sector-based IV cyphers");
> +			ti->zone_append_not_supported = true;
> +		}

I think this negation is hard to follow, at least I had a hard time
reviewing it.

Wouldn't it make more sense to use CRYPT_IV_USE_SECTORS, set the bit
for algorithms that use sectors as IV (like plain64) and then do a 
normal

	if (test_bit(CRYPT_IV_USE_SECTORS, &cc->cipher_flags)) {
		DMWARN("Zone append is not supported with sector-based IV cyphers");
		ti->zone_append_not_supported = true;
	}

i.e. without the double negation?
Damien Le Moal April 16, 2021, 7:30 a.m. UTC | #2
On 2021/04/16 16:13, Johannes Thumshirn wrote:
> On 16/04/2021 05:05, Damien Le Moal wrote:
> 
> [...]
> 
>> +	CRYPT_IV_NO_SECTORS,		/* IV calculation does not use sectors */
> 
> [...]
> 
>> -	if (ivmode == NULL)
>> +	if (ivmode == NULL) {
>>  		cc->iv_gen_ops = NULL;
>> -	else if (strcmp(ivmode, "plain") == 0)
>> +		set_bit(CRYPT_IV_NO_SECTORS, &cc->cipher_flags);
>> +	} else if (strcmp(ivmode, "plain") == 0)
> 
> [...]
> 
>> +		if (!test_bit(CRYPT_IV_NO_SECTORS, &cc->cipher_flags)) {
>> +			DMWARN("Zone append is not supported with sector-based IV cyphers");
>> +			ti->zone_append_not_supported = true;
>> +		}
> 
> I think this negation is hard to follow, at least I had a hard time
> reviewing it.
> 
> Wouldn't it make more sense to use CRYPT_IV_USE_SECTORS, set the bit
> for algorithms that use sectors as IV (like plain64) and then do a 
> normal

There are only 2 IV modes that do not use sectors. null and random. All others
do. Hence the "NO_SECTORS" choice. That is the exception rather than the norm,
the flag indicates that.

> 
> 	if (test_bit(CRYPT_IV_USE_SECTORS, &cc->cipher_flags)) {
> 		DMWARN("Zone append is not supported with sector-based IV cyphers");
> 		ti->zone_append_not_supported = true;
> 	}
> 
> i.e. without the double negation?

Yes. I agree, it is more readable. But adds more lines for the same result. I
could add a small boolean helper to make the "!test_bit(CRYPT_IV_NO_SECTORS,
&cc->cipher_flags)" easier to understand.


> 
>
Johannes Thumshirn April 16, 2021, 9:10 a.m. UTC | #3
On 16/04/2021 09:30, Damien Le Moal wrote:
> On 2021/04/16 16:13, Johannes Thumshirn wrote:
>> On 16/04/2021 05:05, Damien Le Moal wrote:
>>
>> [...]
>>
>>> +	CRYPT_IV_NO_SECTORS,		/* IV calculation does not use sectors */
>>
>> [...]
>>
>>> -	if (ivmode == NULL)
>>> +	if (ivmode == NULL) {
>>>  		cc->iv_gen_ops = NULL;
>>> -	else if (strcmp(ivmode, "plain") == 0)
>>> +		set_bit(CRYPT_IV_NO_SECTORS, &cc->cipher_flags);
>>> +	} else if (strcmp(ivmode, "plain") == 0)
>>
>> [...]
>>
>>> +		if (!test_bit(CRYPT_IV_NO_SECTORS, &cc->cipher_flags)) {
>>> +			DMWARN("Zone append is not supported with sector-based IV cyphers");
>>> +			ti->zone_append_not_supported = true;
>>> +		}
>>
>> I think this negation is hard to follow, at least I had a hard time
>> reviewing it.
>>
>> Wouldn't it make more sense to use CRYPT_IV_USE_SECTORS, set the bit
>> for algorithms that use sectors as IV (like plain64) and then do a 
>> normal
> 
> There are only 2 IV modes that do not use sectors. null and random. All others
> do. Hence the "NO_SECTORS" choice. That is the exception rather than the norm,
> the flag indicates that.
> 
>>
>> 	if (test_bit(CRYPT_IV_USE_SECTORS, &cc->cipher_flags)) {
>> 		DMWARN("Zone append is not supported with sector-based IV cyphers");
>> 		ti->zone_append_not_supported = true;
>> 	}
>>
>> i.e. without the double negation?
> 
> Yes. I agree, it is more readable. But adds more lines for the same result. I
> could add a small boolean helper to make the "!test_bit(CRYPT_IV_NO_SECTORS,
> &cc->cipher_flags)" easier to understand.
> 

Yes I guessed this was the reason for the choice.
Maybe 

set_bit(CRYPT_IV_USE_SECTORS, &cc->cipher_flags);

if (!strcmp(ivmode, "plain") || !strcmp(ivmode, "random"))
	clear_bit(CRYPT_IV_USE_SECTORS, &cc->cipher_flags);

if (test_bit(CRYPT_IV_USE_SECTORS, &cc->cipher_flags)) {
	DMWARN("Zone append is not supported with sector-based IV cyphers");
	ti->zone_append_not_supported = true;
}


Ultimately it's your and Mikes's call, but I /think/ this makes the code easier
to understand.
diff mbox series

Patch

diff --git a/drivers/md/dm-crypt.c b/drivers/md/dm-crypt.c
index b0ab080f2567..0a44bc0ff960 100644
--- a/drivers/md/dm-crypt.c
+++ b/drivers/md/dm-crypt.c
@@ -137,6 +137,7 @@  enum cipher_flags {
 	CRYPT_MODE_INTEGRITY_AEAD,	/* Use authenticated mode for cipher */
 	CRYPT_IV_LARGE_SECTORS,		/* Calculate IV from sector_size, not 512B sectors */
 	CRYPT_ENCRYPT_PREPROCESS,	/* Must preprocess data for encryption (elephant) */
+	CRYPT_IV_NO_SECTORS,		/* IV calculation does not use sectors */
 };
 
 /*
@@ -2750,9 +2751,10 @@  static int crypt_ctr_ivmode(struct dm_target *ti, const char *ivmode)
 	}
 
 	/* Choose ivmode, see comments at iv code. */
-	if (ivmode == NULL)
+	if (ivmode == NULL) {
 		cc->iv_gen_ops = NULL;
-	else if (strcmp(ivmode, "plain") == 0)
+		set_bit(CRYPT_IV_NO_SECTORS, &cc->cipher_flags);
+	} else if (strcmp(ivmode, "plain") == 0)
 		cc->iv_gen_ops = &crypt_iv_plain_ops;
 	else if (strcmp(ivmode, "plain64") == 0)
 		cc->iv_gen_ops = &crypt_iv_plain64_ops;
@@ -2762,9 +2764,10 @@  static int crypt_ctr_ivmode(struct dm_target *ti, const char *ivmode)
 		cc->iv_gen_ops = &crypt_iv_essiv_ops;
 	else if (strcmp(ivmode, "benbi") == 0)
 		cc->iv_gen_ops = &crypt_iv_benbi_ops;
-	else if (strcmp(ivmode, "null") == 0)
+	else if (strcmp(ivmode, "null") == 0) {
 		cc->iv_gen_ops = &crypt_iv_null_ops;
-	else if (strcmp(ivmode, "eboiv") == 0)
+		set_bit(CRYPT_IV_NO_SECTORS, &cc->cipher_flags);
+	} else if (strcmp(ivmode, "eboiv") == 0)
 		cc->iv_gen_ops = &crypt_iv_eboiv_ops;
 	else if (strcmp(ivmode, "elephant") == 0) {
 		cc->iv_gen_ops = &crypt_iv_elephant_ops;
@@ -2791,6 +2794,7 @@  static int crypt_ctr_ivmode(struct dm_target *ti, const char *ivmode)
 		cc->key_extra_size = cc->iv_size + TCW_WHITENING_SIZE;
 	} else if (strcmp(ivmode, "random") == 0) {
 		cc->iv_gen_ops = &crypt_iv_random_ops;
+		set_bit(CRYPT_IV_NO_SECTORS, &cc->cipher_flags);
 		/* Need storage space in integrity fields. */
 		cc->integrity_iv_size = cc->iv_size;
 	} else {
@@ -3281,14 +3285,31 @@  static int crypt_ctr(struct dm_target *ti, unsigned int argc, char **argv)
 	}
 	cc->start = tmpll;
 
-	/*
-	 * For zoned block devices, we need to preserve the issuer write
-	 * ordering. To do so, disable write workqueues and force inline
-	 * encryption completion.
-	 */
 	if (bdev_is_zoned(cc->dev->bdev)) {
+		/*
+		 * For zoned block devices, we need to preserve the issuer write
+		 * ordering. To do so, disable write workqueues and force inline
+		 * encryption completion.
+		 */
 		set_bit(DM_CRYPT_NO_WRITE_WORKQUEUE, &cc->flags);
 		set_bit(DM_CRYPT_WRITE_INLINE, &cc->flags);
+
+		/*
+		 * All zone append writes to a zone of a zoned block device will
+		 * have the same BIO sector (the start of the zone). When the
+		 * cypher IV mode uses sector values, all data targeting a
+		 * zone will be encrypted using the first sector numbers of the
+		 * zone. This will not result in write errors but will
+		 * cause most reads to fail as reads will use the sector values
+		 * for the actual data location, resulting in IV mismatch.
+		 * To avoid this problem, allow zone append operations only for
+		 * cyphers with an IV mode not using sector values (null and
+		 * random IVs).
+		 */
+		if (!test_bit(CRYPT_IV_NO_SECTORS, &cc->cipher_flags)) {
+			DMWARN("Zone append is not supported with sector-based IV cyphers");
+			ti->zone_append_not_supported = true;
+		}
 	}
 
 	if (crypt_integrity_aead(cc) || cc->integrity_iv_size) {
@@ -3356,6 +3377,15 @@  static int crypt_map(struct dm_target *ti, struct bio *bio)
 	struct dm_crypt_io *io;
 	struct crypt_config *cc = ti->private;
 
+	/*
+	 * For zoned targets using a sector based IV, zone append is not
+	 * supported. We should not see any such operation in that case.
+	 * In the unlikely case we do, warn and fail the request.
+	 */
+	if (WARN_ON_ONCE(bio_op(bio) == REQ_OP_ZONE_APPEND &&
+			 !test_bit(CRYPT_IV_NO_SECTORS, &cc->cipher_flags)))
+		return DM_MAPIO_KILL;
+
 	/*
 	 * If bio is REQ_PREFLUSH or REQ_OP_DISCARD, just bypass crypt queues.
 	 * - for REQ_PREFLUSH device-mapper core ensures that no IO is in-flight