| Message ID | 1541062439-55558-1-git-send-email-alios_sys_security@linux.alibaba.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Delegated to: | Mike Snitzer |
| Headers | show |
| Series | dm crypt: use unsigned long long instead of sector_t to store iv_offset | expand |
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? -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel
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 -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel
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;
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(-)