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
