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@xxxxxxxxxx> Thanks for sending this out. I've added dm-devel@xxxxxxxxxx to Cc, as I think the device-mapper developers need to be aware of this. I also added linux-fscrypt@xxxxxxxxxxxxxxx, 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@xxxxxxxxx and https://lore.kernel.org/r/20170616125511.GA11824@xxxxxxxxxxxxxxxxx. 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@xxxxxxxxxx/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