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

[Milan Broz <gmazyland@gmail.com>]

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.

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

# 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

> 
> Signed-off-by: Milan Broz <gmazyland@gmail.com>
> ---
>  drivers/md/dm-crypt.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/md/dm-crypt.c b/drivers/md/dm-crypt.c
> index 54aef8ed97db..488ecd0b1bd0 100644
> --- a/drivers/md/dm-crypt.c
> +++ b/drivers/md/dm-crypt.c
> @@ -2584,6 +2584,10 @@ static int crypt_ctr_optional(struct dm_target *ti, unsigned int argc, char **ar
>  				ti->error = "Invalid feature value for sector_size";
>  				return -EINVAL;
>  			}
> +			if (ti->len & ((cc->sector_size >> SECTOR_SHIFT) - 1)) {
> +				ti->error = "Device size is not multiple of sector_size feature";
> +				return -EINVAL;
> +			}
>  			cc->sector_shift = __ffs(cc->sector_size) - SECTOR_SHIFT;
>  		} else if (!strcasecmp(opt_string, "iv_large_sectors"))
>  			set_bit(CRYPT_IV_LARGE_SECTORS, &cc->cipher_flags);
>