[PATCH 0/1 RFC] block: Add ioctl for setting default inline crypto key

[PATCH 0/1 RFC] block: Add ioctl for setting default inline crypto key

[Israel Rukshin]

Hi Jens/Christoph/Eric

I am working to add support for inline encryption/decryption
at storage protocols like nvmf over RDMA. Right now, the only
way to use the inline crypto feature is via fs-crypt. This patch
allows to use this feature also directly on a block device.
This patch comes after my former dm-crypt patch for inline encryption
was rejected ("[PATCH 1/1] dm crypt: Add inline encryption support")
by the maintainers. This alternative solution of a new block ioctl
was suggested by Christoph Hellwig and Eric Biggers.

I tested this patch with modified nvme-rdma as the block device.
I would like to collect some comments on this approach and I'll send the
entire patch set including NVMe/RDMA changes for inline encryption/decryption.

Israel Rukshin (1):
  block: Add support for setting inline encryption key per block device

 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

-- 
2.18.2

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

[Israel Rukshin]

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

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

Re: [dm-devel] [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

--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel

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: [dm-devel] [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.

--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel

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

--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel

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: [dm-devel] [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

--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel

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

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

[Christoph Hellwig]

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

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

[Eric Biggers]

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

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

[Israel Rukshin]

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

Re: [PATCH 0/1 RFC] block: Add ioctl for setting default inline crypto key

[Christoph Hellwig]

On Wed, Jul 20, 2022 at 02:26:30PM +0300, Israel Rukshin wrote:
> Hi Jens/Christoph/Eric
> 
> I am working to add support for inline encryption/decryption
> at storage protocols like nvmf over RDMA.

What would that be? nvmf over RDMA suggest on-the-wire transport
encryption, which really is a different think that what fscrypt
and blk-crypto deal with.  NVMe has a Key per I/O TP, which is
still waiting for the messy TCG counter part, but as far as I
can tell it is in many ways designed to more or less intentionally
not fit the fscrypt model.