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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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



[Index of Archives]     [Linux RAID]     [Linux SCSI]     [Linux ATA RAID]     [IDE]     [Linux Wireless]     [Linux Kernel]     [ATH6KL]     [Linux Bluetooth]     [Linux Netdev]     [Kernel Newbies]     [Security]     [Git]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Device Mapper]

  Powered by Linux