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
