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

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

[Milan Broz]

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.

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);
-- 
2.14.1

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

[Milan Broz]

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

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

[Mikulas Patocka]

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 */

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

[Milan Broz]

On 10/02/2017 04:43 PM, Mikulas Patocka wrote:
> 
> 
> 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.

ok, I thought it does not work if we change sector size between active and inactive table,
but your approach apparently works even for this case.

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

Tested-by: Milan Broz <gmazyland@gmail.com>

Thanks!

Milan

> 
> ---
>  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 */
> 

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

[Alasdair G Kergon]

On Mon, Oct 02, 2017 at 10:43:56AM -0400, Mikulas Patocka wrote:
> Here I send a patch that checks invalid limits when the table is loaded 
> and aborts the table load ioctl with an error.
 
The code does not already do this because it needs to avoid unnecessary
failures if any characteristics of the devices underneath change after
the 'load' but before the 'resume' (as can happen, for example, when a
tree of devices gets updated by performing all the loads before all the
resumes).

Alasdair

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

[Mike Snitzer]

On Tue, Oct 03 2017 at  8:05am -0400,
Alasdair G Kergon <agk@redhat.com> wrote:

> On Mon, Oct 02, 2017 at 10:43:56AM -0400, Mikulas Patocka wrote:
> > Here I send a patch that checks invalid limits when the table is loaded 
> > and aborts the table load ioctl with an error.
>  
> The code does not already do this because it needs to avoid unnecessary
> failures if any characteristics of the devices underneath change after
> the 'load' but before the 'resume' (as can happen, for example, when a
> tree of devices gets updated by performing all the loads before all the
> resumes).

Not sure why you're putting such value on that behaviour.  The earlier
we catch invalid tables the better off we are.  Failing at resume time
sucks (always has).

Could be there is some specific scenario where lvm2 is leveraging this
historic DM "feature" but it'd be nice to revalidate the need.

Mike

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

[Alasdair G Kergon]

On Tue, Oct 03, 2017 at 02:08:04PM -0400, Mike Snitzer wrote:
> Not sure why you're putting such value on that behaviour.  The earlier
> we catch invalid tables the better off we are.  Failing at resume time
> sucks (always has).
 
Validation code shouldn't be making assumptions about things that lie
completely outside its control and falsely failing operations that would
actually succeed if they were allowed to proceed.  The existing
kernel/userspace interface does not require userspace to load devices in
any particular sequence.  We could have provided a tree-based kernel/userspace
interface with stronger requirements like these, but the fact is, we haven't.
Perhaps we will one day.

As a minimum, you would need to change the patch to validate against the
inactive tables of underlying devices instead of the live ones - i.e. assume
that userspace already loaded all the underlying devices (and will resume them
all before the one being validated gets resumed).  Currently no such
ordering requirement is imposed on userspace, so you'd also need a
compatibility flag to enable the stronger contraints.

This patch can break valid userspace code.

Alasdair

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

[Mikulas Patocka]

On Tue, 3 Oct 2017, Alasdair G Kergon wrote:

> On Tue, Oct 03, 2017 at 02:08:04PM -0400, Mike Snitzer wrote:
> > Not sure why you're putting such value on that behaviour.  The earlier
> > we catch invalid tables the better off we are.  Failing at resume time
> > sucks (always has).
>  
> Validation code shouldn't be making assumptions about things that lie
> completely outside its control and falsely failing operations that would
> actually succeed if they were allowed to proceed.  The existing
> kernel/userspace interface does not require userspace to load devices in
> any particular sequence.  We could have provided a tree-based kernel/userspace
> interface with stronger requirements like these, but the fact is, we haven't.
> Perhaps we will one day.
> 
> As a minimum, you would need to change the patch to validate against the
> inactive tables of underlying devices instead of the live ones - i.e. assume
> that userspace already loaded all the underlying devices (and will resume them
> all before the one being validated gets resumed).  Currently no such
> ordering requirement is imposed on userspace, so you'd also need a
> compatibility flag to enable the stronger contraints.
> 
> This patch can break valid userspace code.
> 
> Alasdair

It would be interesting to know, why Milan wants the table load to fail.

It could be possible to check the validity of the alignment in the 
cryptsetup tool and not attempt to load invalid tables at all. Is there 
any reason, why we need to detect the misalignment in the kernel?

Mikulas

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

[Milan Broz]

On 10/03/2017 10:08 PM, Mikulas Patocka wrote:
> 
> It would be interesting to know, why Milan wants the table load to fail.

I mentioned this on IRC:
the only situation I care about in load is that size (dm-table length) is unaligned to optional sector_size.
create fails in this case, load should imho fail as well. 
...
if we say that dmsetup table output is always directly usable (as a mapping table),
then why should there be an exception for dmsetup table --inactive? (now it can print apparently invalid mapping)

Anyway, I am ok if it fails in resume - but do not keep the device suspended after the fail!

> It could be possible to check the validity of the alignment in the 
> cryptsetup tool and not attempt to load invalid tables at all. Is there 
> any reason, why we need to detect the misalignment in the kernel?

Cryptsetup already rejects such a mapping before even calling dm-ioctl.

But anyone can use dmsetup tool to do that. I just think that incompatible
sector vs. device size should be rejected in target constructor.
(IOW my former patch for dm-crypt that rejects only this exact situation without
doing more device-related tests like your generalized patch in table_load.)

Milan

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

[Mike Snitzer]

On Tue, Oct 03 2017 at  4:33pm -0400,
Milan Broz <gmazyland@gmail.com> wrote:

> On 10/03/2017 10:08 PM, Mikulas Patocka wrote:
> > 
> > It would be interesting to know, why Milan wants the table load to fail.
> 
> I mentioned this on IRC:
> the only situation I care about in load is that size (dm-table length) is unaligned to optional sector_size.
> create fails in this case, load should imho fail as well. 
> ...
> if we say that dmsetup table output is always directly usable (as a mapping table),
> then why should there be an exception for dmsetup table --inactive? (now it can print apparently invalid mapping)

The .ctr should validate the inactive table and that'll cause load to
fail.

Or dm-crypt could publish block_limits that reflect this optional
sector_size and we'll get create (resume) failure.. which I assume is
what you want to avoid.

> Anyway, I am ok if it fails in resume - but do not keep the device suspended after the fail!

Sounds like we need a patch to resume after failed inactive table load.
Might cause lvm2 to try to resume when there is no need.  But the user
would've already had to suspend and then resume to try to load the
inactive table.  If we resume with the original (working) table it may
surprise the user... will certainly cause lvm2 to fail its table
comparison tests if the resume to old working table is done without
erroring out.

So we'd need to still return error but resume with old table if it
exists... and who is asking for this again?  Just us devs who think
leaving the device suspended is bad form?

The user caused the problem by requesting a malformed table get
used... I'm not sure how I feel about covering for such imprecise users.

> > It could be possible to check the validity of the alignment in the 
> > cryptsetup tool and not attempt to load invalid tables at all. Is there 
> > any reason, why we need to detect the misalignment in the kernel?
> 
> Cryptsetup already rejects such a mapping before even calling dm-ioctl.
> 
> But anyone can use dmsetup tool to do that. I just think that incompatible
> sector vs. device size should be rejected in target constructor.
> (IOW my former patch for dm-crypt that rejects only this exact situation without
> doing more device-related tests like your generalized patch in table_load.)

I'll revisit your patch since it reflects what I first said above (about
the .ctr erroring out as needed).

Not sure why Mikulas is saying all the other targets need this too
(e.g. verity, integrity, etc).

Mike

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

[Milan Broz]

On 10/03/2017 11:18 PM, Mike Snitzer wrote:
> On Tue, Oct 03 2017 at  4:33pm -0400,
> Milan Broz <gmazyland@gmail.com> wrote:
> 
>> On 10/03/2017 10:08 PM, Mikulas Patocka wrote:
>>>
>>> It would be interesting to know, why Milan wants the table load to fail.
>>
>> I mentioned this on IRC:
>> the only situation I care about in load is that size (dm-table length) is unaligned to optional sector_size.
>> create fails in this case, load should imho fail as well. 
>> ...
>> if we say that dmsetup table output is always directly usable (as a mapping table),
>> then why should there be an exception for dmsetup table --inactive? (now it can print apparently invalid mapping)
> 
> The .ctr should validate the inactive table and that'll cause load to
> fail.

And that's exactly what is the former patch doing - we introduced a new parameter that
has new limitations, we should fix constructor. That's all I want :-)

> Or dm-crypt could publish block_limits that reflect this optional
> sector_size and we'll get create (resume) failure.. which I assume is
> what you want to avoid.

I think it is doing it already, that's why Mikulas patch works
(it validates reported device physical sector sizes, not the internal encryption block parameter).

>> Anyway, I am ok if it fails in resume - but do not keep the device suspended after the fail!
> 
> Sounds like we need a patch to resume after failed inactive table load.
> Might cause lvm2 to try to resume when there is no need.  But the user
> would've already had to suspend and then resume to try to load the
> inactive table.  If we resume with the original (working) table it may
> surprise the user... will certainly cause lvm2 to fail its table
> comparison tests if the resume to old working table is done without
> erroring out.
> 
> So we'd need to still return error but resume with old table if it
> exists... and who is asking for this again?  Just us devs who think
> leaving the device suspended is bad form?
> 
> The user caused the problem by requesting a malformed table get
> used... I'm not sure how I feel about covering for such imprecise users.

IMHO there are some basic principles that should be invariant.
We can say, in a specific case, that we intentionally violate it, because the risk is negligible.

But we should not be trapped in "normalization of deviation".
The rule "only developers (or imprecise users) report this so it is not serious problem yet"
sounds like this approach.

For me, these are invariants (must be enforced in kernel) that still make sense:

1) Target constructor must validate table and must fail if options are incompatible.

2) The "dmsetup table [--inactive]" (active and inactive table in-kernel) must not
contain incompatible options.
(I am not saying anything about in-table devices attributes validation, these can change before resume.)

It implies that it should be *load* operation that fails and not resume, causing that invalid
inactive table is never accepted.

Resume still can fail later for other reasons (underlying device disappeared and similar).

And this is *different* problem - should it reactivate old table if possible? Dunno.
I just can cay that suspended device are nasty and causes many problems so
I prefer our tools minimize situations when it happens.


For the reported problem - there are no users yet, only developers (me) did that mistake
and can report it.

I can imagine that the same mistake can be seen in scripts that automatically reacts
to device size changes. Up to now, there were no limitations to device size alignment.
I have no idea how many scripts people invented. I have fixed cryptsetup only.
If an unmodified script fails and keeps underlying device in suspended state,
for administrator, it would be really irritating - it should fail and not block the device.

Sorry for the rant but I couldn't resist. Back in quiet mode now ;-)

Milan

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

[Mike Snitzer]

On Wed, Oct 04 2017 at  2:45am -0400,
Milan Broz <gmazyland@gmail.com> wrote:

> On 10/03/2017 11:18 PM, Mike Snitzer wrote:
> > On Tue, Oct 03 2017 at  4:33pm -0400,
> > Milan Broz <gmazyland@gmail.com> wrote:
> > 
> >> On 10/03/2017 10:08 PM, Mikulas Patocka wrote:
> >>>
> >>> It would be interesting to know, why Milan wants the table load to fail.
> >>
> >> I mentioned this on IRC:
> >> the only situation I care about in load is that size (dm-table length) is unaligned to optional sector_size.
> >> create fails in this case, load should imho fail as well. 
> >> ...
> >> if we say that dmsetup table output is always directly usable (as a mapping table),
> >> then why should there be an exception for dmsetup table --inactive? (now it can print apparently invalid mapping)
> > 
> > The .ctr should validate the inactive table and that'll cause load to
> > fail.
> 
> And that's exactly what is the former patch doing - we introduced a new parameter that
> has new limitations, we should fix constructor. That's all I want :-)

Yeap, I've staged your fix.. enough with all this debate.
See: https://git.kernel.org/pub/scm/linux/kernel/git/device-mapper/linux-dm.git/commit/?h=for-4.14/dm&id=783874b050768d361239e444ba0fa396bb6d463f

(but yes I read your "rant".. I agreed with all you said.. but it lacked
the qualities of a flaming rant.. you must be happier ;)