Re: [PATCH] dm-crypt: Reject sector_size feature if device length is not aligned to it

[Mikulas Patocka <mpatocka@redhat.com>]

On Sat, 30 Sep 2017, Milan Broz wrote:

> On 09/13/2017 03:45 PM, Milan Broz wrote:
> > If a crypt mapping uses optional sector_size feature, additional
> > restrictions to mapped device segment size must be applied in constructor,
> > otherwise the device activation will fail later.
> 
> Hi,
> 
> we had some discussion with Mikulas if this check should be better in generic DM code.
> 
> I think that for this case it is not a good idea - dm-crypt can increase
> encryption sector size during load (it is stupid to do, but I see no reason why to block it).
> And then only constructor of the target itself know what is possible and what should be rejected.

The same argument also applies to verity, integrity and zoned target. I 
think it should be tested in the generic dm code, not duplicated in these 
targets.

Here I send a patch that checks invalid limits when the table is loaded 
and aborts the table load ioctl with an error.

> Anyway, there is a short reproducer what this patch solves:
> 
> Create simple mapping with 4096 encryption sector:
> 
> # dmsetup create test --table "0 8 crypt cipher_null - 0 /dev/sdb 0 1 sector_size:4096"
> 
> Now load new unaligned-length table (this should fail!)
> # dmsetup load test --table "0 9 crypt cipher_null - 0 /dev/sdb 0 1 sector_size:4096"
> 
> Inactive table is apparently accepted:
> # dmsetup table --inactive
> test: 0 9 crypt cipher_null 0 0 8:16 0 1 sector_size:4096
> 
> And now, resume fails, keeping the device in suspended state afterward:
> 
> # dmsetup resume test
> device-mapper: resume ioctl on test failed: Invalid argument
> Command failed
> kernel: device-mapper: table: 254:0: len=9 not aligned to h/w logical block size 4096 of sdb

BTW. if you resume it twice, it will continue working with the old table.

> # dmsetup info -c
> Name             Maj Min Stat Open Targ Event  UUID
> test             254   0 L-sw    0    1      0
> 
> With the patch applied, the load step correctly fails:
> # dmsetup load test --table "0 9 crypt cipher_null - 0 /dev/sdb 0 1 sector_size:4096"
> device-mapper: reload ioctl on test failed: Invalid argument
> kernel: device-mapper: table: 254:0: crypt: Device size is not multiple of sector_size feature
> 
> Please consider this for 4.14 (and stable 4.12+ perhaps).
> 
> Thanks,
> Milan

From: Mikulas Patocka <mpatocka@redhat.com>
Subject: dm: check invalid limits when table is created

Device mapper checks invalid limits when resuming a device and swapping a 
table, however it may be too late becuase it makes the resume ioctl fail. 
This patch checks the limits when a table is loaded, so that the table 
load ioctl will fail.

Signed-off-by: Mikulas Patocka <mpatocka@redhat.com>

---
 drivers/md/dm-ioctl.c |    5 +++++
 1 file changed, 5 insertions(+)

Index: linux-2.6/drivers/md/dm-ioctl.c
===================================================================
--- linux-2.6.orig/drivers/md/dm-ioctl.c
+++ linux-2.6/drivers/md/dm-ioctl.c
@@ -1308,6 +1308,7 @@ static int table_load(struct file *filp,
 	struct dm_table *t, *old_map = NULL;
 	struct mapped_device *md;
 	struct target_type *immutable_target_type;
+	struct queue_limits dummy_limits;
 
 	md = find_device(param);
 	if (!md)
@@ -1349,6 +1350,10 @@ static int table_load(struct file *filp,
 		goto err_unlock_md_type;
 	}
 
+	r = dm_calculate_queue_limits(t, &dummy_limits);
+	if (r)
+		goto err_unlock_md_type;
+
 	dm_unlock_md_type(md);
 
 	/* stage inactive table */