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

[Eric Biggers <ebiggers@kernel.org>]

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