Re: [PATCH 1/1] block: Add support for setting inline encryption key per block device

Re: [PATCH 1/1] block: Add support for setting inline encryption key per block device

[Eric Biggers]

Hi Israel,

On Wed, Jul 20, 2022 at 02:26:31PM +0300, Israel Rukshin wrote:
> Until now, using inline encryption key has been possible only at
> filesystem level via fs-crypt. The patch allows to set a default
> inline encryption key per block device. Once the key is set, all the
> read commands will be decrypted, and all the write commands will
> be encrypted. The key can be overridden by another key from a higher
> level (FS/DM).
> 
> To add/remove a key, the patch introduces a block device ioctl:
>  - BLKCRYPTOSETKEY: set a key with the following attributes:
>     - crypto_mode: identifier for the encryption algorithm to use
>     - raw_key_ptr:  pointer to a buffer of the raw key
>     - raw_key_size: size in bytes of the raw key
>     - data_unit_size: the data unit size to use for en/decryption
>       (must be <= device logical block size)
> To remove the key, use the same ioctl with raw_key_size = 0.
> It is not possible to remove the key when it is in use by any
> in-flight IO or when the block device is open.
> 
> Signed-off-by: Israel Rukshin <israelr@nvidia.com>

Thanks for sending this out.  I've added dm-devel@redhat.com to Cc, as I think
the device-mapper developers need to be aware of this.  I also added
linux-fscrypt@vger.kernel.org, as this is relevant there too.

I'm glad to see a proposal in this area -- this is something that is greatly
needed.  Chrome OS is looking for something like "dm-crypt with inline crypto
support", which this should work for.  Android is also looking for something
similar with the additional property that filesystems can override the key used.

Some high-level comments about this approach (I'll send some more routine code
review nits in a separate email):

> @@ -134,7 +150,8 @@ static inline void bio_crypt_do_front_merge(struct request *rq,
>  bool __blk_crypto_bio_prep(struct bio **bio_ptr);
>  static inline bool blk_crypto_bio_prep(struct bio **bio_ptr)
>  {
> -	if (bio_has_crypt_ctx(*bio_ptr))
> +	if (bio_has_crypt_ctx(*bio_ptr) ||
> +	    blk_crypto_bio_set_default_ctx(*bio_ptr))
>  		return __blk_crypto_bio_prep(bio_ptr);
>  	return true;

This allows upper layers to override the block device key, which as I've
mentioned is very useful -- it allows block device encryption and file
encryption to be used together without the performance cost of double
encryption.  Android already needs and uses this type of encryption.

Still, it's important to understand the limitations of this particular way to
achieve this type of encryption, since I want to make sure everyone is on board.


First, this implementation currently doesn't provide a way to skip the default
key without setting an encryption context.  There are actually two cases where
the default key must be skipped when there is no encryption context.  The first
is when the filesystem is doing I/O to an encrypted file, but the filesystem
wasn't mounted with the "inlinecrypt" mount option.  This could be worked around
by requiring "inlinecrypt" if a default key is set; that loses some flexibility,
but it could be done.  The second case, which can't be worked around at the
filesystem level, is that the f2fs garbage collector sometimes has to move the
data of encrypted files on-disk without having access to their encryption key.

So we'll actually need a separate indicator for "skip the default key".  The
simplest solution would be a new flag in the bio.  However, to avoid using space
in struct bio, it could instead be a special value of the encryption context.


Second, both this solution and dm-based solutions have the property that they
allow the default key to be *arbitrarily* overridden by upper layers.  That
means that there is no *general* guarantee that all data on the device is
protected at least as well as the default key.  Consider e.g. the default key
being overridden by an all-zeros key.  Concerns about this sort of architecture
were raised on a dm-crypt patchset several years ago; see
https://lore.kernel.org/r/0b268ff7-5fc8-c85f-a530-82e9844f0400@gmail.com and
https://lore.kernel.org/r/20170616125511.GA11824@yeono.kjorling.se.

The alternative architecture for this that I've had in mind, and which has been
prototyped for f2fs
(https://lore.kernel.org/linux-f2fs-devel/20201005073606.1949772-1-satyat@google.com/T/#u),
is for the filesystem to manage *all* the encryption, and to mix the default key
into all file keys.  Then, all data on the block device would always be
protected by the default key or better, regardless of userspace.

On the other hand, I'd personally be fine with saying that this isn't actually
needed, i.e. that allowing arbitrary overriding of the default key is fine,
since userspace should just set up the keys properly.  For example, Android
doesn't need this at all, as it sets up all its keys properly.  But I want to
make sure that everyone is generally okay with this now, as I personally don't
see a fundamental difference between this and what the dm-crypt developers had
rejected *very* forcefully before.  Perhaps it's acceptable now simply because
it wouldn't be part of dm-crypt; it's a new thing, so it can have new semantics.

> +static int blk_crypto_ioctl_create_key(struct request_queue *q,
> +				       void __user *argp)
> +{
> +	struct blk_crypto_set_key_arg arg;
> +	u8 raw_key[BLK_CRYPTO_MAX_KEY_SIZE];
> +	struct blk_crypto_key *blk_key;
> +	int ret;
> +
> +	if (copy_from_user(&arg, argp, sizeof(arg)))
> +		return -EFAULT;
> +
> +	if (memchr_inv(arg.reserved, 0, sizeof(arg.reserved)))
> +		return -EINVAL;
> +
> +	if (!arg.raw_key_size)
> +		return blk_crypto_destroy_default_key(q);
> +
> +	if (q->blk_key) {
> +		pr_err("Crypto key is already configured\n");
> +		return -EBUSY;
> +	}
> +
> +	if (arg.raw_key_size > sizeof(raw_key))
> +		return -EINVAL;
> +
> +	if (arg.data_unit_size > queue_logical_block_size(q)) {
> +		pr_err("Data unit size is bigger than logical block size\n");
> +		return -EINVAL;
> +	}

This is forbidding data_unit_size greater than logical_block_size.  I see why
you did this: the block layer doesn't know what is above it, and it could
receive I/O requests targeting individual logical blocks.  However, this can
result in a big efficiency loss; a common example is when a filesystem with a
4096-byte block size is used on a block device with a logical block size of 512
bytes.  Since such a filesystem only submits I/O in 4096-byte blocks, it's safe
for the data_unit_size to be 4096 bytes as well.  This is much more efficient
than 512 bytes, since there is overhead for every data unit.  Note that some of
this overhead is intrinsic in the crypto algorithms themselves and cannot be
optimized out by better hardware.

The device-mapper based solution wouldn't have this problem, as the
device-mapper target (dm-inlinecrypt or whatever) would just advertise the
crypto data unit size as the logical block size, like dm-crypt does.

I'm wondering if anyone any thoughts about how to allow data_unit_size >
logical_block_size with this patch's approach.  I suppose that the ioctl could
just allow setting it, and the block layer could fail any I/O that isn't
properly aligned to the data_unit_size.

- Eric

Re: [PATCH 1/1] block: Add support for setting inline encryption key per block device

[Christoph Hellwig]

On Wed, Jul 20, 2022 at 11:49:07PM -0700, Eric Biggers wrote:
> On the other hand, I'd personally be fine with saying that this isn't actually
> needed, i.e. that allowing arbitrary overriding of the default key is fine,
> since userspace should just set up the keys properly.  For example, Android
> doesn't need this at all, as it sets up all its keys properly.  But I want to
> make sure that everyone is generally okay with this now, as I personally don't
> see a fundamental difference between this and what the dm-crypt developers had
> rejected *very* forcefully before.  Perhaps it's acceptable now simply because
> it wouldn't be part of dm-crypt; it's a new thing, so it can have new semantics.

I agree with both the dm-crypt maintainer and you on this.  dm-crypt is
a full disk encryption solution and has to provide guarantees, so it
can't let upper layers degrade the cypher.  The block layer support on
the other hand is just a building block an can as long as it is carefully
documented.

> I'm wondering if anyone any thoughts about how to allow data_unit_size >
> logical_block_size with this patch's approach.  I suppose that the ioctl could
> just allow setting it, and the block layer could fail any I/O that isn't
> properly aligned to the data_unit_size.

We could do that, but we'd need to comunicate the limit to the upper
layers both in the kernel an user space.  Effectively this changes the
logical block size for all practical purposes.

Re: [dm-devel] [PATCH 1/1] block: Add support for setting inline encryption key per block device

[Milan Broz]

On 21/07/2022 14:54, Christoph Hellwig wrote:
> On Wed, Jul 20, 2022 at 11:49:07PM -0700, Eric Biggers wrote:
>> On the other hand, I'd personally be fine with saying that this isn't actually
>> needed, i.e. that allowing arbitrary overriding of the default key is fine,
>> since userspace should just set up the keys properly.  For example, Android
>> doesn't need this at all, as it sets up all its keys properly.  But I want to
>> make sure that everyone is generally okay with this now, as I personally don't
>> see a fundamental difference between this and what the dm-crypt developers had
>> rejected *very* forcefully before.  Perhaps it's acceptable now simply because
>> it wouldn't be part of dm-crypt; it's a new thing, so it can have new semantics.
> 
> I agree with both the dm-crypt maintainer and you on this.  dm-crypt is
> a full disk encryption solution and has to provide guarantees, so it
> can't let upper layers degrade the cypher.  The block layer support on
> the other hand is just a building block an can as long as it is carefully
> documented.

Yes, that is what I meant when complaining about the last dm-crypt patch.

I think inline encryption for block devices is a good idea; my complaint was
integration with dm-crypt - as it can dilute expectations and create a new
threat model here.

But I also think that implementing this directly in the block layer makes sense.
Perhaps it should be generic enough.
(I can imagine dynamic inline encryption integrated into LVM ... :-)

>> I'm wondering if anyone any thoughts about how to allow data_unit_size >
>> logical_block_size with this patch's approach.  I suppose that the ioctl could
>> just allow setting it, and the block layer could fail any I/O that isn't
>> properly aligned to the data_unit_size.
> 
> We could do that, but we'd need to comunicate the limit to the upper
> layers both in the kernel an user space.  Effectively this changes the
> logical block size for all practical purposes.

I think you should support setting logical encryption size from the beginning,
at least prepare API for that. It has a big impact on performance.
The dm-crypt also requires aligned IO here. We propagate new logical size
to upper layers (and also to loop device below, if used).
(Also this revealed hacks in USB enclosures mapping unaligned devices...)

Milan

Re: [PATCH 1/1] block: Add support for setting inline encryption key per block device

[Daniil Lunev]

On Wed, Jul 20, 2022 at 11:49:07PM -0700, Eric Biggers wrote:
> I'm glad to see a proposal in this area -- this is something that is greatly
> needed.  Chrome OS is looking for something like "dm-crypt with inline crypto
> support", which this should work for.  Android is also looking for something
> similar with the additional property that filesystems can override the key used.
Yes, this is exciting to see proposals in this area. In ChromeOS we were
contemplating ways to upstream Eric's work for Android. This solution should
work generally for our use-case, however I would like to add a few extra pieces
we were considering.

One thing we were looking for is having an option to pass inline encryption keys
via keyrings, similarly to how dm-crypt allows suuplying keys both ways: raw and
keyring attached. I would assume that is something that should still be possible
with the IOCTL-based approach, though proposed API can make it a bit obscure. I
was wondering whether there should be a separate field to allow this kind of
differentiation?

The other aspect is the key lifetime. Current implementation doesn't allow to
unset the key once set. This is something that would still work with dm setups,
presumably, since the key lifetime is tied to the lifetime of the device itself,
but may render problematic if this is applied to a raw device or partition of a
raw device, I would assume - allowing no ways to rotate the key without reboot.
I am not sure if this is a particularly important issue, but something that I
wanted to raise for the discussion. This also becomes relevant in the context of
the keyring usages, i.e. whether to revoke the key from the block device when
the key is removed from the keyring, or assume it is bound at the time of device
setup. The dm-crypt follows the latter model, AFAIU, and it is fine to keep it
consistent, but then the question comes back to inability to remove the key in
the current API in general.

And speaking about dm, the other thing we were looking into is compatibility of
inline encryption key setup with dm stacks. Current kernel implementaiton
propagates the crypto context through linear and flakey target, we also had
initial tests enabling it on thin pools by adding DM_TARGET_PASSES_CRYPTO, which
didn't show any problems at first glance (but more testing is required). We
believe that an ability to setup multiple different dm targets with different
keys over the same physical device is an important use case - and as far as I
can tell proposed approach supports it, but wanted to highlight that as well.

--Daniil