| Message ID | 1658316391-13472-2-git-send-email-israelr@nvidia.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | block: Add ioctl for setting default inline crypto key | expand |
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
> + if (bc->bc_key->q) > + atomic_inc(&bc->bc_key->q->blk_key_use_count); > } I don't think a global atomic for each I/O is a good idea. > +static int blk_crypto_destroy_default_key(struct request_queue *q) > +{ > + int ret; > + > + if (!q->blk_key) > + return 0; > + > + blk_mq_freeze_queue(q); > + blk_mq_quiesce_queue(q); > + if (atomic_read(&q->blk_key_use_count)) { But once the queue is frozen all I/O is gone at least for blk-mq drivers, so I don't think we'll need this as long as we limit the feature to blk-mq drivers. Eventually we'll need to make blk_mq_freeze_queue also track outstanding I/Os on bio based drivers, in which case it will work there as well. Alternatively we can support evicting the key only if the device is not otherwise opened only. > + if (q->blk_key) { > + pr_err("Crypto key is already configured\n"); > + return -EBUSY; > + } Don't we nee locking to make this check race free? > + switch (cmd) { > + case BLKCRYPTOSETKEY: > + if (mode & FMODE_EXCL) > + return blk_crypto_ioctl_create_key(q, argp); > + > + if (IS_ERR(blkdev_get_by_dev(bdev->bd_dev, mode | FMODE_EXCL, > + &bdev))) > + return -EBUSY; > + ret = blk_crypto_ioctl_create_key(q, argp); > + blkdev_put(bdev, mode | FMODE_EXCL); I think we can just safely require an FMODE_EXCL open here. > #ifdef CONFIG_BLK_INLINE_ENCRYPTION > struct blk_crypto_profile *crypto_profile; > struct kobject *crypto_kobject; > + struct blk_crypto_key *blk_key; > + atomic_t blk_key_use_count; > #endif Both the existing and new fields really should go into the gendisk, but I think we need to clean up the existing bits first > +#ifndef __UAPI_LINUX_BLK_CRYPTO_H > +#define __UAPI_LINUX_BLK_CRYPTO_H > + > +enum blk_crypto_mode_num { > + BLK_ENCRYPTION_MODE_INVALID, > + BLK_ENCRYPTION_MODE_AES_256_XTS, > + BLK_ENCRYPTION_MODE_AES_128_CBC_ESSIV, > + BLK_ENCRYPTION_MODE_ADIANTUM, > + BLK_ENCRYPTION_MODE_MAX, > +}; Please make these #defines so that userspace can probe for newly added ones. a _MAX value should not be part of the uapi headers. > +struct blk_crypto_set_key_arg { Why is this in fs.h and not the new blk-cryto uapi header?
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.
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
On Wed, Jul 20, 2022 at 02:26:31PM +0300, Israel Rukshin wrote: > +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; > + } Doesn't this need locking so that multiple executions of this ioctl can't run on the block device at the same time? Also, the key is actually being associated with the request_queue. I thought it was going to be the block_device? Doesn't it have to be the block_device so that different partitions on the same disk can have different settings? Also, the pr_err should be removed. Likewise in several places below. > + > + 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; > + } > + > + if (copy_from_user(raw_key, u64_to_user_ptr(arg.raw_key_ptr), > + arg.raw_key_size)) { > + ret = -EFAULT; > + goto err; > + } > + > + blk_key = kzalloc(sizeof(*blk_key), GFP_KERNEL); > + if (!blk_key) { > + ret = -ENOMEM; > + goto err; > + } > + > + ret = blk_crypto_init_key(blk_key, raw_key, arg.crypto_mode, > + sizeof(u64), arg.data_unit_size); > + if (ret) { > + pr_err("Failed to init inline encryption key\n"); > + goto key_err; > + } > + > + /* Block key size is taken from crypto mode */ > + if (arg.raw_key_size != blk_key->size) { > + ret = -EINVAL; > + goto key_err; > + } The key size check above happens too late. The easiest solution would be to add the key size as an argument to blk_crypto_init_key(), and make it validate the key size. > + } > + blk_key->q = q; > + q->blk_key = blk_key; How does this interact with concurrent I/O? Also, doesn't the block device's page cache need to be invalidated when a key is set or removed? > diff --git a/include/uapi/linux/blk-crypto.h b/include/uapi/linux/blk-crypto.h > new file mode 100644 > index 000000000000..5a65ebaf6038 > --- /dev/null > +++ b/include/uapi/linux/blk-crypto.h > @@ -0,0 +1,14 @@ > +/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */ > + > +#ifndef __UAPI_LINUX_BLK_CRYPTO_H > +#define __UAPI_LINUX_BLK_CRYPTO_H > + > +enum blk_crypto_mode_num { > + BLK_ENCRYPTION_MODE_INVALID, > + BLK_ENCRYPTION_MODE_AES_256_XTS, > + BLK_ENCRYPTION_MODE_AES_128_CBC_ESSIV, > + BLK_ENCRYPTION_MODE_ADIANTUM, > + BLK_ENCRYPTION_MODE_MAX, > +}; Instead of an enum, these should use #define's with explicit numbers. Also, INVALID and MAX shouldn't be included. > + > +#endif /* __UAPI_LINUX_BLK_CRYPTO_H */ > diff --git a/include/uapi/linux/fs.h b/include/uapi/linux/fs.h > index bdf7b404b3e7..398a77895e96 100644 > --- a/include/uapi/linux/fs.h > +++ b/include/uapi/linux/fs.h > @@ -121,6 +121,14 @@ struct fsxattr { > unsigned char fsx_pad[8]; > }; > > +struct blk_crypto_set_key_arg { > + __u32 crypto_mode; > + __u64 raw_key_ptr; > + __u32 raw_key_size; > + __u32 data_unit_size; > + __u32 reserved[9]; /* must be zero */ > +}; The reserved fields should be __u64 so that it's easy to use them for pointers or other 64-bit data later if it becomes necessary. Also, reserved fields should also be prefixed with "__". Also, a padding field is needed between crypto_mode and raw_key_ptr. Also, it would be a good idea to make the reserved space even larger, as people may want to add all sorts of weird settings here in the future (look at dm-crypt). The following might be good; it makes the whole struct a nice round size, 128 bytes: struct blk_crypto_set_key_arg { __u32 crypto_mode; __u32 __reserved1; __u64 raw_key_ptr; __u32 raw_key_size; __u32 data_unit_size; __u64 __reserved2[13]; }; - Eric
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
Hi Eric, Thanks for the review! On 7/26/2022 3:42 AM, Eric Biggers wrote: > On Wed, Jul 20, 2022 at 02:26:31PM +0300, Israel Rukshin wrote: >> +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; >> + } > Doesn't this need locking so that multiple executions of this ioctl can't run on > the block device at the same time? Yes, it should and I will add on V2. > > Also, the key is actually being associated with the request_queue. I thought it > was going to be the block_device? Doesn't it have to be the block_device so > that different partitions on the same disk can have different settings? I will check your suggestion. Currently, all the partitions on the disk have the same key. > > Also, the pr_err should be removed. Likewise in several places below. > >> + >> + 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; >> + } >> + >> + if (copy_from_user(raw_key, u64_to_user_ptr(arg.raw_key_ptr), >> + arg.raw_key_size)) { >> + ret = -EFAULT; >> + goto err; >> + } >> + >> + blk_key = kzalloc(sizeof(*blk_key), GFP_KERNEL); >> + if (!blk_key) { >> + ret = -ENOMEM; >> + goto err; >> + } >> + >> + ret = blk_crypto_init_key(blk_key, raw_key, arg.crypto_mode, >> + sizeof(u64), arg.data_unit_size); >> + if (ret) { >> + pr_err("Failed to init inline encryption key\n"); >> + goto key_err; >> + } >> + >> + /* Block key size is taken from crypto mode */ >> + if (arg.raw_key_size != blk_key->size) { >> + ret = -EINVAL; >> + goto key_err; >> + } > The key size check above happens too late. The easiest solution would be to add > the key size as an argument to blk_crypto_init_key(), and make it validate the > key size. > >> + } >> + blk_key->q = q; >> + q->blk_key = blk_key; > How does this interact with concurrent I/O? Also, doesn't the block device's > page cache need to be invalidated when a key is set or removed? In case of removing the key, I am freezing the queue to avoid concurrent I/O. I can do the same thing when setting the key. Setting a key when running I/O is not well defined. At this implementation, only part the concurrent I/O will be encrypted. After the ioctl is done then all the new I/O will be encrypted. >> diff --git a/include/uapi/linux/blk-crypto.h b/include/uapi/linux/blk-crypto.h >> new file mode 100644 >> index 000000000000..5a65ebaf6038 >> --- /dev/null >> +++ b/include/uapi/linux/blk-crypto.h >> @@ -0,0 +1,14 @@ >> +/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */ >> + >> +#ifndef __UAPI_LINUX_BLK_CRYPTO_H >> +#define __UAPI_LINUX_BLK_CRYPTO_H >> + >> +enum blk_crypto_mode_num { >> + BLK_ENCRYPTION_MODE_INVALID, >> + BLK_ENCRYPTION_MODE_AES_256_XTS, >> + BLK_ENCRYPTION_MODE_AES_128_CBC_ESSIV, >> + BLK_ENCRYPTION_MODE_ADIANTUM, >> + BLK_ENCRYPTION_MODE_MAX, >> +}; > Instead of an enum, these should use #define's with explicit numbers. Also, > INVALID and MAX shouldn't be included. > >> + >> +#endif /* __UAPI_LINUX_BLK_CRYPTO_H */ >> diff --git a/include/uapi/linux/fs.h b/include/uapi/linux/fs.h >> index bdf7b404b3e7..398a77895e96 100644 >> --- a/include/uapi/linux/fs.h >> +++ b/include/uapi/linux/fs.h >> @@ -121,6 +121,14 @@ struct fsxattr { >> unsigned char fsx_pad[8]; >> }; >> >> +struct blk_crypto_set_key_arg { >> + __u32 crypto_mode; >> + __u64 raw_key_ptr; >> + __u32 raw_key_size; >> + __u32 data_unit_size; >> + __u32 reserved[9]; /* must be zero */ >> +}; > The reserved fields should be __u64 so that it's easy to use them for pointers > or other 64-bit data later if it becomes necessary. Also, reserved fields > should also be prefixed with "__". Also, a padding field is needed between > crypto_mode and raw_key_ptr. Also, it would be a good idea to make the reserved > space even larger, as people may want to add all sorts of weird settings here in > the future (look at dm-crypt). > > The following might be good; it makes the whole struct a nice round size, 128 > bytes: > > struct blk_crypto_set_key_arg { > __u32 crypto_mode; > __u32 __reserved1; > __u64 raw_key_ptr; > __u32 raw_key_size; > __u32 data_unit_size; > __u64 __reserved2[13]; > }; > > - Eric Israel
diff --git a/block/blk-core.c b/block/blk-core.c index 27fb1357ad4b..80fed8c794d7 100644 --- a/block/blk-core.c +++ b/block/blk-core.c @@ -474,6 +474,10 @@ struct request_queue *blk_alloc_queue(int node_id, bool alloc_srcu) PERCPU_REF_INIT_ATOMIC, GFP_KERNEL)) goto fail_stats; +#ifdef CONFIG_BLK_INLINE_ENCRYPTION + atomic_set(&q->blk_key_use_count, 0); +#endif + blk_queue_dma_alignment(q, 511); blk_set_default_limits(&q->limits); q->nr_requests = BLKDEV_DEFAULT_RQ; diff --git a/block/blk-crypto-internal.h b/block/blk-crypto-internal.h index e6818ffaddbf..33a162697e53 100644 --- a/block/blk-crypto-internal.h +++ b/block/blk-crypto-internal.h @@ -65,6 +65,11 @@ static inline bool blk_crypto_rq_is_encrypted(struct request *rq) return rq->crypt_ctx; } +bool blk_crypto_bio_set_default_ctx(struct bio *bio); + +int blk_crypto_ioctl(struct block_device *bdev, fmode_t mode, unsigned int cmd, + void __user *argp); + #else /* CONFIG_BLK_INLINE_ENCRYPTION */ static inline int blk_crypto_sysfs_register(struct request_queue *q) @@ -105,6 +110,17 @@ static inline bool blk_crypto_rq_is_encrypted(struct request *rq) return false; } +static bool blk_crypto_bio_set_default_ctx(struct bio *bio) +{ + return false; +} + +static inline int blk_crypto_ioctl(struct block_device *bdev, fmode_t mode, + unsigned int cmd, void __user *argp) +{ + return -ENOTTY; +} + #endif /* CONFIG_BLK_INLINE_ENCRYPTION */ void __bio_crypt_advance(struct bio *bio, unsigned int bytes); @@ -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; } diff --git a/block/blk-crypto-profile.c b/block/blk-crypto-profile.c index 96c511967386..4ca75520b466 100644 --- a/block/blk-crypto-profile.c +++ b/block/blk-crypto-profile.c @@ -459,6 +459,7 @@ bool blk_crypto_register(struct blk_crypto_profile *profile, return false; } q->crypto_profile = profile; + q->blk_key = NULL; return true; } EXPORT_SYMBOL_GPL(blk_crypto_register); diff --git a/block/blk-crypto.c b/block/blk-crypto.c index a496aaef85ba..39129d8b120f 100644 --- a/block/blk-crypto.c +++ b/block/blk-crypto.c @@ -98,20 +98,30 @@ void bio_crypt_set_ctx(struct bio *bio, const struct blk_crypto_key *key, memcpy(bc->bc_dun, dun, sizeof(bc->bc_dun)); bio->bi_crypt_context = bc; + if (bc->bc_key->q) + atomic_inc(&bc->bc_key->q->blk_key_use_count); } void __bio_crypt_free_ctx(struct bio *bio) { + struct request_queue *q = bio->bi_crypt_context->bc_key->q; + + if (q) + atomic_dec(&q->blk_key_use_count); mempool_free(bio->bi_crypt_context, bio_crypt_ctx_pool); bio->bi_crypt_context = NULL; } int __bio_crypt_clone(struct bio *dst, struct bio *src, gfp_t gfp_mask) { + struct request_queue *q = src->bi_crypt_context->bc_key->q; + dst->bi_crypt_context = mempool_alloc(bio_crypt_ctx_pool, gfp_mask); if (!dst->bi_crypt_context) return -ENOMEM; *dst->bi_crypt_context = *src->bi_crypt_context; + if (q) + atomic_inc(&q->blk_key_use_count); return 0; } @@ -413,3 +423,149 @@ int blk_crypto_evict_key(struct request_queue *q, return blk_crypto_fallback_evict_key(key); } EXPORT_SYMBOL_GPL(blk_crypto_evict_key); + +bool blk_crypto_bio_set_default_ctx(struct bio *bio) +{ + struct request_queue *q = bdev_get_queue(bio->bi_bdev); + u64 dun[BLK_CRYPTO_DUN_ARRAY_SIZE]; + + if (!q->blk_key) + return false; + + if (bio_op(bio) != REQ_OP_READ && bio_op(bio) != REQ_OP_WRITE) + return false; + + if (!bio_sectors(bio)) + return false; + + memset(dun, 0, BLK_CRYPTO_MAX_IV_SIZE); + dun[0] = bio->bi_iter.bi_sector >> + (q->blk_key->data_unit_size_bits - SECTOR_SHIFT); + bio_crypt_set_ctx(bio, q->blk_key, dun, GFP_KERNEL); + + return true; +} + +static int blk_crypto_destroy_default_key(struct request_queue *q) +{ + int ret; + + if (!q->blk_key) + return 0; + + blk_mq_freeze_queue(q); + blk_mq_quiesce_queue(q); + if (atomic_read(&q->blk_key_use_count)) { + ret = -EBUSY; + } else { + ret = blk_crypto_evict_key(q, q->blk_key); + if (!ret) { + kfree_sensitive(q->blk_key); + q->blk_key = NULL; + } + } + blk_mq_unquiesce_queue(q); + blk_mq_unfreeze_queue(q); + + return ret; +} + +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; + } + + if (copy_from_user(raw_key, u64_to_user_ptr(arg.raw_key_ptr), + arg.raw_key_size)) { + ret = -EFAULT; + goto err; + } + + blk_key = kzalloc(sizeof(*blk_key), GFP_KERNEL); + if (!blk_key) { + ret = -ENOMEM; + goto err; + } + + ret = blk_crypto_init_key(blk_key, raw_key, arg.crypto_mode, + sizeof(u64), arg.data_unit_size); + if (ret) { + pr_err("Failed to init inline encryption key\n"); + goto key_err; + } + + /* Block key size is taken from crypto mode */ + if (arg.raw_key_size != blk_key->size) { + ret = -EINVAL; + goto key_err; + } + + ret = blk_crypto_start_using_key(blk_key, q); + if (ret) { + pr_err("Failed to use inline encryption key\n"); + goto key_err; + } + blk_key->q = q; + q->blk_key = blk_key; + + memzero_explicit(raw_key, sizeof(raw_key)); + + return 0; + +key_err: + kfree_sensitive(blk_key); +err: + memzero_explicit(raw_key, sizeof(raw_key)); + return ret; +} + +int blk_crypto_ioctl(struct block_device *bdev, fmode_t mode, unsigned int cmd, + void __user *argp) +{ + struct request_queue *q = bdev_get_queue(bdev); + int ret; + + if (!q->crypto_profile) + return -EOPNOTSUPP; + + switch (cmd) { + case BLKCRYPTOSETKEY: + if (mode & FMODE_EXCL) + return blk_crypto_ioctl_create_key(q, argp); + + if (IS_ERR(blkdev_get_by_dev(bdev->bd_dev, mode | FMODE_EXCL, + &bdev))) + return -EBUSY; + ret = blk_crypto_ioctl_create_key(q, argp); + blkdev_put(bdev, mode | FMODE_EXCL); + + return ret; + default: + return -ENOTTY; + } +} diff --git a/block/blk-sysfs.c b/block/blk-sysfs.c index 9b905e9443e4..2d28b17393db 100644 --- a/block/blk-sysfs.c +++ b/block/blk-sysfs.c @@ -765,6 +765,13 @@ static void blk_release_queue(struct kobject *kobj) might_sleep(); +#ifdef CONFIG_BLK_INLINE_ENCRYPTION + if (q->blk_key) { + blk_crypto_evict_key(q, q->blk_key); + kfree_sensitive(q->blk_key); + } +#endif + percpu_ref_exit(&q->q_usage_counter); if (q->poll_stat) diff --git a/block/ioctl.c b/block/ioctl.c index 46949f1b0dba..b5358819de6a 100644 --- a/block/ioctl.c +++ b/block/ioctl.c @@ -12,6 +12,7 @@ #include <linux/pr.h> #include <linux/uaccess.h> #include "blk.h" +#include "blk-crypto-internal.h" static int blkpg_do_ioctl(struct block_device *bdev, struct blkpg_partition __user *upart, int op) @@ -532,6 +533,8 @@ static int blkdev_common_ioctl(struct block_device *bdev, fmode_t mode, case BLKTRACESTOP: case BLKTRACETEARDOWN: return blk_trace_ioctl(bdev, cmd, argp); + case BLKCRYPTOSETKEY: + return blk_crypto_ioctl(bdev, mode, cmd, argp); case IOC_PR_REGISTER: return blkdev_pr_register(bdev, argp); case IOC_PR_RESERVE: diff --git a/include/linux/blk-crypto.h b/include/linux/blk-crypto.h index 69b24fe92cbf..cea421c9374f 100644 --- a/include/linux/blk-crypto.h +++ b/include/linux/blk-crypto.h @@ -7,14 +7,7 @@ #define __LINUX_BLK_CRYPTO_H #include <linux/types.h> - -enum blk_crypto_mode_num { - BLK_ENCRYPTION_MODE_INVALID, - BLK_ENCRYPTION_MODE_AES_256_XTS, - BLK_ENCRYPTION_MODE_AES_128_CBC_ESSIV, - BLK_ENCRYPTION_MODE_ADIANTUM, - BLK_ENCRYPTION_MODE_MAX, -}; +#include <uapi/linux/blk-crypto.h> #define BLK_CRYPTO_MAX_KEY_SIZE 64 /** @@ -39,6 +32,7 @@ struct blk_crypto_config { * @data_unit_size_bits: log2 of data_unit_size * @size: size of this key in bytes (determined by @crypto_cfg.crypto_mode) * @raw: the raw bytes of this key. Only the first @size bytes are used. + * @q: if set, this key is used as a default key for this queue. * * A blk_crypto_key is immutable once created, and many bios can reference it at * the same time. It must not be freed until all bios using it have completed @@ -49,6 +43,7 @@ struct blk_crypto_key { unsigned int data_unit_size_bits; unsigned int size; u8 raw[BLK_CRYPTO_MAX_KEY_SIZE]; + struct request_queue *q; }; #define BLK_CRYPTO_MAX_IV_SIZE 32 diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h index dd552479a15c..b45b13ea2f7d 100644 --- a/include/linux/blkdev.h +++ b/include/linux/blkdev.h @@ -429,6 +429,8 @@ struct request_queue { #ifdef CONFIG_BLK_INLINE_ENCRYPTION struct blk_crypto_profile *crypto_profile; struct kobject *crypto_kobject; + struct blk_crypto_key *blk_key; + atomic_t blk_key_use_count; #endif unsigned int rq_timeout; diff --git a/include/uapi/linux/blk-crypto.h b/include/uapi/linux/blk-crypto.h new file mode 100644 index 000000000000..5a65ebaf6038 --- /dev/null +++ b/include/uapi/linux/blk-crypto.h @@ -0,0 +1,14 @@ +/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */ + +#ifndef __UAPI_LINUX_BLK_CRYPTO_H +#define __UAPI_LINUX_BLK_CRYPTO_H + +enum blk_crypto_mode_num { + BLK_ENCRYPTION_MODE_INVALID, + BLK_ENCRYPTION_MODE_AES_256_XTS, + BLK_ENCRYPTION_MODE_AES_128_CBC_ESSIV, + BLK_ENCRYPTION_MODE_ADIANTUM, + BLK_ENCRYPTION_MODE_MAX, +}; + +#endif /* __UAPI_LINUX_BLK_CRYPTO_H */ diff --git a/include/uapi/linux/fs.h b/include/uapi/linux/fs.h index bdf7b404b3e7..398a77895e96 100644 --- a/include/uapi/linux/fs.h +++ b/include/uapi/linux/fs.h @@ -121,6 +121,14 @@ struct fsxattr { unsigned char fsx_pad[8]; }; +struct blk_crypto_set_key_arg { + __u32 crypto_mode; + __u64 raw_key_ptr; + __u32 raw_key_size; + __u32 data_unit_size; + __u32 reserved[9]; /* must be zero */ +}; + /* * Flags for the fsx_xflags field */ @@ -185,6 +193,7 @@ struct fsxattr { #define BLKROTATIONAL _IO(0x12,126) #define BLKZEROOUT _IO(0x12,127) #define BLKGETDISKSEQ _IOR(0x12,128,__u64) +#define BLKCRYPTOSETKEY _IOW(0x12, 129, struct blk_crypto_set_key_arg) /* * A jump here: 130-136 are reserved for zoned block devices * (see uapi/linux/blkzoned.h)
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> --- block/blk-core.c | 4 + block/blk-crypto-internal.h | 19 +++- block/blk-crypto-profile.c | 1 + block/blk-crypto.c | 156 ++++++++++++++++++++++++++++++++ block/blk-sysfs.c | 7 ++ block/ioctl.c | 3 + include/linux/blk-crypto.h | 11 +-- include/linux/blkdev.h | 2 + include/uapi/linux/blk-crypto.h | 14 +++ include/uapi/linux/fs.h | 9 ++ 10 files changed, 217 insertions(+), 9 deletions(-) create mode 100644 include/uapi/linux/blk-crypto.h