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

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

[AliOS system security]

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.

Signed-off-by: AliOS system security <alios_sys_security@linux.alibaba.com>
---
 drivers/md/dm-crypt.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/md/dm-crypt.c b/drivers/md/dm-crypt.c
index b8eec51..49be7a6 100644
--- a/drivers/md/dm-crypt.c
+++ b/drivers/md/dm-crypt.c
@@ -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

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

[Mike Snitzer]

On Thu, Nov 01 2018 at  4:53am -0400,
AliOS system security <alios_sys_security@linux.alibaba.com> wrote:

> 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.

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

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

[AliOS system security]

On 2018/11/2 4:06, Mike Snitzer wrote:
> On Thu, Nov 01 2018 at  4:53am -0400,
> AliOS system security <alios_sys_security@linux.alibaba.com> wrote:
>
>> 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.
> 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?

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

[Mike Snitzer]

On Fri, Nov 02 2018 at 12:31am -0400,
AliOS system security <alios_sys_security@linux.alibaba.com> wrote:

> On 2018/11/2 4:06, Mike Snitzer wrote:
> >On Thu, Nov 01 2018 at  4:53am -0400,
> >AliOS system security <alios_sys_security@linux.alibaba.com> wrote:
> >
> >>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.
> >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

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

[Milan Broz]

On 01/11/2018 21:06, Mike Snitzer wrote:
> On Thu, Nov 01 2018 at  4:53am -0400,
> AliOS system security <alios_sys_security@linux.alibaba.com> wrote:
> 
>> 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.
> 
> 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.

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.

> Please show proof to the contrary if you still think this change 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

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

[AliOS system security]

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.

Signed-off-by: AliOS system security <alios_sys_security@linux.alibaba.com>
---
 drivers/md/dm-crypt.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/md/dm-crypt.c b/drivers/md/dm-crypt.c
index b8eec51..49be7a6 100644
--- a/drivers/md/dm-crypt.c
+++ b/drivers/md/dm-crypt.c
@@ -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

Re: [PATCH] dm crypt: use unsigned long long instead of sector_t to store iv_offset

[Milan Broz]

On 03/11/2018 04:53, AliOS system security wrote:
> 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.
> 
> Signed-off-by: AliOS system security <alios_sys_security@linux.alibaba.com>

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
Tested-and-Reviewed-by: Milan Broz <gmazyland@gmail.com>

Milan

> ---
>  drivers/md/dm-crypt.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/md/dm-crypt.c b/drivers/md/dm-crypt.c
> index b8eec51..49be7a6 100644
> --- a/drivers/md/dm-crypt.c
> +++ b/drivers/md/dm-crypt.c
> @@ -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;
> 

[PATCH v2] dm crypt: use u64 instead of sector_t to store iv_offset

[AliOS system security]

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

Signed-off-by: AliOS system security <alios_sys_security@linux.alibaba.com>
Tested-and-Reviewed-by: Milan Broz <gmazyland@gmail.com>
---

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(-)

diff --git a/drivers/md/dm-crypt.c b/drivers/md/dm-crypt.c
index b8eec51..9a4dec0 100644
--- a/drivers/md/dm-crypt.c
+++ b/drivers/md/dm-crypt.c
@@ -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