dm crypt: use unsigned long long instead of sector_t to store iv_offset

The iv_offset in the mapping table of crypt target is a 64bit number when iv mode is plain64 or plain64be. It will be assigned to iv_offset of struct crypt_config, cc_sector of struct convert_context and iv_sector of struct dm_crypt_request. These structures members are defined as a sector_t. But sector_t is 32bit when CONFIG_LBDAF is not set in 32bit kernel. In this situation sector_t is not big enough to store the 64bit iv_offset.

---
 drivers/md/dm-crypt.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

index b8eec51..49be7a6 100644
@@ -49,7 +49,7 @@ struct convert_context {
 	struct bio *bio_out;
 	struct bvec_iter iter_in;
 	struct bvec_iter iter_out;
-	sector_t cc_sector;
+	unsigned long long cc_sector;
 	atomic_t cc_pending;
 	union {
 		struct skcipher_request *req;

@@ -81,7 +81,7 @@ struct dm_crypt_request {
 	struct convert_context *ctx;
 	struct scatterlist sg_in[4];
 	struct scatterlist sg_out[4];
-	sector_t iv_sector;
+	unsigned long long iv_sector;
 };
 
 struct crypt_config;

@@ -160,7 +160,7 @@ struct crypt_config {
 		struct iv_lmk_private lmk;
 		struct iv_tcw_private tcw;
 	} iv_gen_private;
-	sector_t iv_offset;
+	unsigned long long iv_offset;
 	unsigned int iv_size;
 	unsigned short int sector_size;
 	unsigned char sector_shift;

2.7.4

I really don't think this is needed.

cc->iv_offset can only address a the address space used to access the device. Which is expressed in terms of sectors. Therefore if CONFIG_LBDAF is not set in 32bit kernel then there is no need to address beyond that which 'sector_t' addresses.

Please show proof to the contrary if you still think this change is needed.

Mike

Sorry I made a mistake. I read Documentation/device-mapper/dm-crypt.txt again and found that the IV offset is a sector count. So it make sense to store the iv_offset as a sector_t.

In addition, I want to describe what problem I met in the beginning. I made a crypt.img with the crypt param "aes-cbc-plain64 0x1234...5678 1311768465173141112 /dev/loop0 0" in a 32bit kernel with CONFIG_LBDAF=y. The iv_offset is set to a 64bit number and the iv mode is set to plain64. Someday I recompiled my kernel but CONFIG_LBDAF is not set this time. When I reload the crypt.img with the same crypt param in new kernel, I got ioctl(..., DM_TABLE_LOAD, ...) return 0 but the content of /dev/dm-0 is incorrect.

So, is this situation, set the iv mode to plain64 or plain64be in a 32bit kernel with CONFIG_LBDAF is not set, a problem? Should the crypt_ctr() return an error code when this happned? Or we just support 64bit iv mode all the time regardless of CONFIG_LBDAF?

Please see Milan's reply from earlier today. You did find a real bug. Milan is going to iterate on your patch (likely switch to using uint64_t and update the patch header to capture all the useful context for why this is a real issue -- albeit one that has apparently been around since 2.6.20).

I'll still attribute the change to you, but Milan's contribution will be an incremental improvement on your initial patch.

Thanks for reporting and fixing this.

Mike

Hi,

The iv_offset is Initialization Vector offset (for symmetric block cipher) that is not in principle limited by the device size sector offset. It is just an additional tweak to the cipher mode IV.

If we use 32bit only for 64bit IV, it is wrong and a patch is needed.

It is easy to reproduce, even it shows this causes data corruption:

# dd if=/dev/zero of=tst.img bs=1M count=1

On 32bit system (use IV offset value that overflows to 64bit; CONFIG_LBDAF if off) (and note IV value overflow in table!!)

# echo "tst"|cryptsetup open --type plain -c aes-xts-plain64 --skip 500000000000000000 tst.img test

# sha256sum /dev/mapper/test
533e25c09176632b3794f35303488c4a8f3f965dffffa6ec2df347c168cb6c19  /dev/mapper/test

# dmsetup table test
0 2048 crypt aes-xts-plain64 0000000000000000000000000000000000000000000000000000000000000000 3551657984 7:0 0

On 64bit system (the same image):

# echo "tst"|cryptsetup open --type plain -c aes-xts-plain64 --skip 500000000000000000 tst.img test

# sha256sum /dev/mapper/test
5d16160f9d5f8c33d8051e65fdb4f003cc31cd652b5abb08f03aa6fce0df75fc  /dev/mapper/test

# dmsetup table test
0 2048 crypt aes-xts-plain64 0000000000000000000000000000000000000000000000000000000000000000 500000000000000000 7:0 0

This must be fixed, thanks for reporting it!

Unfortunately I still do not see the original patch in the list you are replying to.

Once it is on the list, I'll ack it.

Thanks,
Milan

The iv_offset in the mapping table of crypt target is a 64bit number when iv mode is plain64 or plain64be. It will be assigned to iv_offset of struct crypt_config, cc_sector of struct convert_context and iv_sector of struct dm_crypt_request. These structures members are defined as a sector_t. But sector_t is 32bit when CONFIG_LBDAF is not set in 32bit kernel. In this situation sector_t is not big enough to store the 64bit iv_offset.

---
 drivers/md/dm-crypt.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

index b8eec51..49be7a6 100644
@@ -49,7 +49,7 @@ struct convert_context {
 	struct bio *bio_out;
 	struct bvec_iter iter_in;
 	struct bvec_iter iter_out;
-	sector_t cc_sector;
+	unsigned long long cc_sector;
 	atomic_t cc_pending;
 	union {
 		struct skcipher_request *req;

@@ -81,7 +81,7 @@ struct dm_crypt_request {
 	struct convert_context *ctx;
 	struct scatterlist sg_in[4];
 	struct scatterlist sg_out[4];
-	sector_t iv_sector;
+	unsigned long long iv_sector;
 };
 
 struct crypt_config;

@@ -160,7 +160,7 @@ struct crypt_config {
 		struct iv_lmk_private lmk;
 		struct iv_tcw_private tcw;
 	} iv_gen_private;
-	sector_t iv_offset;
+	unsigned long long iv_offset;
 	unsigned int iv_size;
 	unsigned short int sector_size;
 	unsigned char sector_shift;

2.7.4

I think there are more IV algorithms that are affected (ESSIV, benbi, ...)

I am ok with "unsigned loing long" there (despite it is u64 in reality), but we are already using "unsigned long long" when parsing the table in constructor...

We should add reproducer to the patch header, here using the cryptsetup.

Prepare test image and device (loop is automatically allocated by cryptsetup):

  # dd if=/dev/zero of=tst.img bs=1M count=1
  # echo "tst"|cryptsetup open --type plain -c aes-xts-plain64 --skip 500000000000000000 tst.img test

On 32bit system (use IV offset value that overflows to 64bit; CONFIG_LBDAF if off) and device checksum is wrong:

  # dmsetup table test --showkeys
  0 2048 crypt aes-xts-plain64 dfa7cfe3c481f2239155739c42e539ae8f2d38f304dcc89d20b26f69daaf0933 3551657984 7:0 0

  # sha256sum /dev/mapper/test
  533e25c09176632b3794f35303488c4a8f3f965dffffa6ec2df347c168cb6c19  /dev/mapper/test

On 64bit system (and on 32bit system with the patch), table and checksum is now correct:

  # dmsetup table test --showkeys
  0 2048 crypt aes-xts-plain64 dfa7cfe3c481f2239155739c42e539ae8f2d38f304dcc89d20b26f69daaf0933 500000000000000000 7:0 0

  # sha256sum /dev/mapper/test
  5d16160f9d5f8c33d8051e65fdb4f003cc31cd652b5abb08f03aa6fce0df75fc  /dev/mapper/test

You can add my

Milan

The iv_offset in the mapping table of crypt target is a 64bit number when IV algorithm is plain64, plain64be, essiv or benbi. It will be assigned to iv_offset of struct crypt_config, cc_sector of struct convert_context and iv_sector of struct dm_crypt_request. These structures members are defined as a sector_t. But sector_t is 32bit when CONFIG_LBDAF is not set in 32bit kernel. In this situation sector_t is not big enough to store the 64bit iv_offset.

Here is a reproducer.
Prepare test image and device (loop is automatically allocated by cryptsetup):

  # dd if=/dev/zero of=tst.img bs=1M count=1
  # echo "tst"|cryptsetup open --type plain -c aes-xts-plain64 \
  --skip 500000000000000000 tst.img test

On 32bit system (use IV offset value that overflows to 64bit; CONFIG_LBDAF if off) and device checksum is wrong:

  # dmsetup table test --showkeys
  0 2048 crypt aes-xts-plain64 dfa7cfe3c481f2239155739c42e539ae8f2d38f304dcc89d20b26f69daaf0933 3551657984 7:0 0

  # sha256sum /dev/mapper/test
  533e25c09176632b3794f35303488c4a8f3f965dffffa6ec2df347c168cb6c19 /dev/mapper/test

On 64bit system (and on 32bit system with the patch), table and checksum is now correct:

  # dmsetup table test --showkeys
  0 2048 crypt aes-xts-plain64 dfa7cfe3c481f2239155739c42e539ae8f2d38f304dcc89d20b26f69daaf0933 500000000000000000 7:0 0

  # sha256sum /dev/mapper/test
  5d16160f9d5f8c33d8051e65fdb4f003cc31cd652b5abb08f03aa6fce0df75fc /dev/mapper/test

---

Changes since v2:
* add all IV algorithms that are affected in commit message
* add reproducer in commit message
* use u64 instead of unsigned long long

---
 drivers/md/dm-crypt.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

index b8eec51..9a4dec0 100644
@@ -49,7 +49,7 @@ struct convert_context {
 	struct bio *bio_out;
 	struct bvec_iter iter_in;
 	struct bvec_iter iter_out;
-	sector_t cc_sector;
+	u64 cc_sector;
 	atomic_t cc_pending;
 	union {
 		struct skcipher_request *req;

@@ -81,7 +81,7 @@ struct dm_crypt_request {
 	struct convert_context *ctx;
 	struct scatterlist sg_in[4];
 	struct scatterlist sg_out[4];
-	sector_t iv_sector;
+	u64 iv_sector;
 };
 
 struct crypt_config;

@@ -160,7 +160,7 @@ struct crypt_config {
 		struct iv_lmk_private lmk;
 		struct iv_tcw_private tcw;
 	} iv_gen_private;
-	sector_t iv_offset;
+	u64 iv_offset;
 	unsigned int iv_size;
 	unsigned short int sector_size;
 	unsigned char sector_shift;

2.7.4