[dm-devel] [RFC PATCH 0/1] Add inline encryption support for dm-crypt

[dm-devel] [RFC PATCH 0/1] Add inline encryption support for dm-crypt

[Israel Rukshin]

Hi,

I am working to add support for inline encryption/decryption
at storage protocols like nvmf over RDMA. The HW that I am
working with is ConnecX-6 Dx, which supports inline crypto
and can send the data on the fabric at the same time.

This patchset is based on v5.16-rc4 with Eric Biggers patches
of the HW wrapped keys.
It can be retrieved from branch "wip-wrapped-keys" at
https://git.kernel.org/pub/scm/fs/fscrypt/fscrypt.git

I tested this patch with modified nvme-rdma as the block device
and created a DM crypt on top of it. I got performance enhancement
compared to SW crypto. I tested the HW wrapped inline mode with
the HW and the standard mode with a fallback algorithm.

Israel Rukshin (1):
  dm crypt: Add inline encryption support

 block/blk-crypto.c    |   3 +
 drivers/md/dm-crypt.c | 202 ++++++++++++++++++++++++++++++++++++------
 2 files changed, 180 insertions(+), 25 deletions(-)

-- 
2.27.0

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

[dm-devel] [PATCH 1/1] dm crypt: Add inline encryption support

[Israel Rukshin]

Using inline encryption means that the block layer handles the
decryption/encryption as part of the bio, instead of dm-crypt
doing the crypto by itself via Linux's crypto API. This model
is needed to take advantage of the inline encryption hardware
on the market.

To use inline encryption, the new dm-crypt optional parameter
"inline_crypt" should be set for the configured mapping. Afterwards,
dm-crypt will provide the crypto parameters to the block layer by
creating a cypto profile and by filling the bios with crypto context.
In case the block device or the fallback algorithm doesn't support
this feature, the mapping will fail.

Signed-off-by: Israel Rukshin <israelr@nvidia.com>
---
 block/blk-crypto.c    |   3 +
 drivers/md/dm-crypt.c | 202 ++++++++++++++++++++++++++++++++++++------
 2 files changed, 180 insertions(+), 25 deletions(-)

diff --git a/block/blk-crypto.c b/block/blk-crypto.c
index 1c08d3b6e24a..65f13549eb5f 100644
--- a/block/blk-crypto.c
+++ b/block/blk-crypto.c
@@ -102,6 +102,7 @@ void bio_crypt_set_ctx(struct bio *bio, const struct blk_crypto_key *key,
 
 	bio->bi_crypt_context = bc;
 }
+EXPORT_SYMBOL_GPL(bio_crypt_set_ctx);
 
 void __bio_crypt_free_ctx(struct bio *bio)
 {
@@ -370,6 +371,7 @@ int blk_crypto_init_key(struct blk_crypto_key *blk_key,
 
 	return 0;
 }
+EXPORT_SYMBOL_GPL(blk_crypto_init_key);
 
 /*
  * Check if bios with @cfg can be en/decrypted by blk-crypto (i.e. either the
@@ -411,6 +413,7 @@ int blk_crypto_start_using_key(const struct blk_crypto_key *key,
 	}
 	return blk_crypto_fallback_start_using_mode(key->crypto_cfg.crypto_mode);
 }
+EXPORT_SYMBOL_GPL(blk_crypto_start_using_key);
 
 /**
  * blk_crypto_evict_key() - Evict a key from any inline encryption hardware
diff --git a/drivers/md/dm-crypt.c b/drivers/md/dm-crypt.c
index d4ae31558826..4c0e1904942b 100644
--- a/drivers/md/dm-crypt.c
+++ b/drivers/md/dm-crypt.c
@@ -39,6 +39,7 @@
 #include <keys/user-type.h>
 #include <keys/encrypted-type.h>
 #include <keys/trusted-type.h>
+#include <linux/blk-crypto.h>
 
 #include <linux/device-mapper.h>
 
@@ -134,7 +135,7 @@ struct iv_elephant_private {
 enum flags { DM_CRYPT_SUSPENDED, DM_CRYPT_KEY_VALID,
 	     DM_CRYPT_SAME_CPU, DM_CRYPT_NO_OFFLOAD,
 	     DM_CRYPT_NO_READ_WORKQUEUE, DM_CRYPT_NO_WRITE_WORKQUEUE,
-	     DM_CRYPT_WRITE_INLINE };
+	     DM_CRYPT_WRITE_INLINE, DM_CRYPT_INLINE_ENCRYPTION };
 
 enum cipher_flags {
 	CRYPT_MODE_INTEGRITY_AEAD,	/* Use authenticated mode for cipher */
@@ -220,6 +221,11 @@ struct crypt_config {
 	struct bio_set bs;
 	struct mutex bio_alloc_lock;
 
+#ifdef CONFIG_BLK_INLINE_ENCRYPTION
+	enum blk_crypto_mode_num crypto_mode;
+	enum blk_crypto_key_type key_type;
+	struct blk_crypto_key *blk_key;
+#endif
 	u8 *authenc_key; /* space for keys in authenc() format (if used) */
 	u8 key[];
 };
@@ -2383,11 +2389,103 @@ static void crypt_copy_authenckey(char *p, const void *key,
 	memcpy(p, key, enckeylen);
 }
 
+#ifdef CONFIG_BLK_INLINE_ENCRYPTION
+static int crypt_select_inline_crypt_mode(struct dm_target *ti, char *cipher,
+					  char *ivmode)
+{
+	struct crypt_config *cc = ti->private;
+
+	if (strcmp(cipher, "xts(aes)") == 0) {
+		cc->crypto_mode = BLK_ENCRYPTION_MODE_AES_256_XTS;
+		cc->key_type = BLK_CRYPTO_KEY_TYPE_STANDARD;
+	} else if (strcmp(cipher, "xts(paes)") == 0) {
+		cc->crypto_mode = BLK_ENCRYPTION_MODE_AES_256_XTS;
+		cc->key_type = BLK_CRYPTO_KEY_TYPE_HW_WRAPPED;
+	} else {
+		ti->error = "Invalid cipher for inline_crypt";
+		return -EINVAL;
+	}
+
+	if (ivmode == NULL || (strcmp(ivmode, "plain64") == 0)) {
+		cc->iv_size = 8;
+	} else {
+		ti->error = "Invalid IV mode for inline_crypt";
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
+static int crypt_prepare_inline_crypt_key(struct crypt_config *cc)
+{
+	int ret;
+
+	cc->blk_key = kzalloc(sizeof(*cc->blk_key), GFP_KERNEL);
+	if (!cc->blk_key)
+		return -ENOMEM;
+
+	ret = blk_crypto_init_key(cc->blk_key, cc->key, cc->key_size,
+				  cc->key_type, cc->crypto_mode, cc->iv_size,
+				  cc->sector_size);
+	if (ret) {
+		DMERR("Failed to init inline encryption key");
+		goto bad_key;
+	}
+
+	ret = blk_crypto_start_using_key(cc->blk_key,
+					 bdev_get_queue(cc->dev->bdev));
+	if (ret) {
+		DMERR("Failed to use inline encryption key");
+		goto bad_key;
+	}
+
+	return 0;
+bad_key:
+	kfree_sensitive(cc->blk_key);
+	cc->blk_key = NULL;
+	return ret;
+}
+
+static void crypt_destroy_inline_crypt_key(struct crypt_config *cc)
+{
+	if (cc->blk_key) {
+		blk_crypto_evict_key(bdev_get_queue(cc->dev->bdev),
+				     cc->blk_key);
+		kfree_sensitive(cc->blk_key);
+		cc->blk_key = NULL;
+	}
+}
+
+static void crypt_inline_encrypt_submit(struct dm_target *ti, struct bio *bio)
+{
+	struct crypt_config *cc = ti->private;
+	u64 dun[BLK_CRYPTO_DUN_ARRAY_SIZE];
+
+	bio_set_dev(bio, cc->dev->bdev);
+	if (bio_sectors(bio)) {
+		memset(dun, 0, BLK_CRYPTO_MAX_IV_SIZE);
+		bio->bi_iter.bi_sector = cc->start +
+			dm_target_offset(ti, bio->bi_iter.bi_sector);
+		dun[0] = le64_to_cpu(bio->bi_iter.bi_sector + cc->iv_offset);
+		bio_crypt_set_ctx(bio, cc->blk_key, dun, GFP_KERNEL);
+	}
+
+	submit_bio_noacct(bio);
+}
+#endif
+
 static int crypt_setkey(struct crypt_config *cc)
 {
 	unsigned subkey_size;
 	int err = 0, i, r;
 
+#ifdef CONFIG_BLK_INLINE_ENCRYPTION
+	if (test_bit(DM_CRYPT_INLINE_ENCRYPTION, &cc->flags)) {
+		crypt_destroy_inline_crypt_key(cc);
+		return crypt_prepare_inline_crypt_key(cc);
+	}
+#endif
+
 	/* Ignore extra keys (which are used for IV etc) */
 	subkey_size = crypt_subkey_size(cc);
 
@@ -2648,6 +2746,15 @@ static int crypt_wipe_key(struct crypt_config *cc)
 
 	kfree_sensitive(cc->key_string);
 	cc->key_string = NULL;
+
+#ifdef CONFIG_BLK_INLINE_ENCRYPTION
+	if (test_bit(DM_CRYPT_INLINE_ENCRYPTION, &cc->flags)) {
+		crypt_destroy_inline_crypt_key(cc);
+		memset(&cc->key, 0, cc->key_size * sizeof(u8));
+		return 0;
+	}
+#endif
+
 	r = crypt_setkey(cc);
 	memset(&cc->key, 0, cc->key_size * sizeof(u8));
 
@@ -2713,6 +2820,10 @@ static void crypt_dtr(struct dm_target *ti)
 	if (cc->crypt_queue)
 		destroy_workqueue(cc->crypt_queue);
 
+#ifdef CONFIG_BLK_INLINE_ENCRYPTION
+	crypt_destroy_inline_crypt_key(cc);
+#endif
+
 	crypt_free_tfms(cc);
 
 	bioset_exit(&cc->bs);
@@ -2888,6 +2999,11 @@ static int crypt_ctr_cipher_new(struct dm_target *ti, char *cipher_in, char *key
 	/* The rest is crypto API spec */
 	cipher_api = tmp;
 
+#ifdef CONFIG_BLK_INLINE_ENCRYPTION
+	if (test_bit(DM_CRYPT_INLINE_ENCRYPTION, &cc->flags))
+		return crypt_select_inline_crypt_mode(ti, cipher_api, *ivmode);
+#endif
+
 	/* Alloc AEAD, can be used only in new format. */
 	if (crypt_integrity_aead(cc)) {
 		ret = crypt_ctr_auth_cipher(cc, cipher_api);
@@ -3001,6 +3117,11 @@ static int crypt_ctr_cipher_old(struct dm_target *ti, char *cipher_in, char *key
 		goto bad_mem;
 	}
 
+#ifdef CONFIG_BLK_INLINE_ENCRYPTION
+	if (test_bit(DM_CRYPT_INLINE_ENCRYPTION, &cc->flags))
+		return crypt_select_inline_crypt_mode(ti, cipher_api, *ivmode);
+#endif
+
 	/* Allocate cipher */
 	ret = crypt_alloc_tfms(cc, cipher_api);
 	if (ret < 0) {
@@ -3036,9 +3157,11 @@ static int crypt_ctr_cipher(struct dm_target *ti, char *cipher_in, char *key)
 		return ret;
 
 	/* Initialize IV */
-	ret = crypt_ctr_ivmode(ti, ivmode);
-	if (ret < 0)
-		return ret;
+	if (!test_bit(DM_CRYPT_INLINE_ENCRYPTION, &cc->flags)) {
+		ret = crypt_ctr_ivmode(ti, ivmode);
+		if (ret < 0)
+			return ret;
+	}
 
 	/* Initialize and set key */
 	ret = crypt_set_key(cc, key);
@@ -3111,6 +3234,10 @@ static int crypt_ctr_optional(struct dm_target *ti, unsigned int argc, char **ar
 			set_bit(DM_CRYPT_NO_READ_WORKQUEUE, &cc->flags);
 		else if (!strcasecmp(opt_string, "no_write_workqueue"))
 			set_bit(DM_CRYPT_NO_WRITE_WORKQUEUE, &cc->flags);
+#ifdef CONFIG_BLK_INLINE_ENCRYPTION
+		else if (!strcasecmp(opt_string, "inline_crypt"))
+			set_bit(DM_CRYPT_INLINE_ENCRYPTION, &cc->flags);
+#endif
 		else if (sscanf(opt_string, "integrity:%u:", &val) == 1) {
 			if (val == 0 || val > MAX_TAG_SIZE) {
 				ti->error = "Invalid integrity arguments";
@@ -3218,10 +3345,36 @@ static int crypt_ctr(struct dm_target *ti, unsigned int argc, char **argv)
 			goto bad;
 	}
 
+	ret = -EINVAL;
+	if ((sscanf(argv[2], "%llu%c", &tmpll, &dummy) != 1) ||
+	    (tmpll & ((cc->sector_size >> SECTOR_SHIFT) - 1))) {
+		ti->error = "Invalid iv_offset sector";
+		goto bad;
+	}
+	cc->iv_offset = tmpll;
+
+	ret = dm_get_device(ti, argv[3], dm_table_get_mode(ti->table),
+			    &cc->dev);
+	if (ret) {
+		ti->error = "Device lookup failed";
+		goto bad;
+	}
+
+	ret = -EINVAL;
+	if (sscanf(argv[4], "%llu%c", &tmpll, &dummy) != 1 ||
+	    tmpll != (sector_t)tmpll) {
+		ti->error = "Invalid device sector";
+		goto bad;
+	}
+	cc->start = tmpll;
+
 	ret = crypt_ctr_cipher(ti, argv[0], argv[1]);
 	if (ret < 0)
 		goto bad;
 
+	if (test_bit(DM_CRYPT_INLINE_ENCRYPTION, &cc->flags))
+		return 0;
+
 	if (crypt_integrity_aead(cc)) {
 		cc->dmreq_start = sizeof(struct aead_request);
 		cc->dmreq_start += crypto_aead_reqsize(any_tfm_aead(cc));
@@ -3277,27 +3430,6 @@ static int crypt_ctr(struct dm_target *ti, unsigned int argc, char **argv)
 
 	mutex_init(&cc->bio_alloc_lock);
 
-	ret = -EINVAL;
-	if ((sscanf(argv[2], "%llu%c", &tmpll, &dummy) != 1) ||
-	    (tmpll & ((cc->sector_size >> SECTOR_SHIFT) - 1))) {
-		ti->error = "Invalid iv_offset sector";
-		goto bad;
-	}
-	cc->iv_offset = tmpll;
-
-	ret = dm_get_device(ti, argv[3], dm_table_get_mode(ti->table), &cc->dev);
-	if (ret) {
-		ti->error = "Device lookup failed";
-		goto bad;
-	}
-
-	ret = -EINVAL;
-	if (sscanf(argv[4], "%llu%c", &tmpll, &dummy) != 1 || tmpll != (sector_t)tmpll) {
-		ti->error = "Invalid device sector";
-		goto bad;
-	}
-	cc->start = tmpll;
-
 	if (bdev_is_zoned(cc->dev->bdev)) {
 		/*
 		 * For zoned block devices, we need to preserve the issuer write
@@ -3419,6 +3551,13 @@ static int crypt_map(struct dm_target *ti, struct bio *bio)
 	if (unlikely(bio->bi_iter.bi_size & (cc->sector_size - 1)))
 		return DM_MAPIO_KILL;
 
+#ifdef CONFIG_BLK_INLINE_ENCRYPTION
+	if (test_bit(DM_CRYPT_INLINE_ENCRYPTION, &cc->flags)) {
+		crypt_inline_encrypt_submit(ti, bio);
+		return DM_MAPIO_SUBMITTED;
+	}
+#endif
+
 	io = dm_per_bio_data(bio, cc->per_bio_data_size);
 	crypt_io_init(io, cc, bio, dm_target_offset(ti, bio->bi_iter.bi_sector));
 
@@ -3481,6 +3620,10 @@ static void crypt_status(struct dm_target *ti, status_type_t type,
 		num_feature_args += test_bit(DM_CRYPT_NO_OFFLOAD, &cc->flags);
 		num_feature_args += test_bit(DM_CRYPT_NO_READ_WORKQUEUE, &cc->flags);
 		num_feature_args += test_bit(DM_CRYPT_NO_WRITE_WORKQUEUE, &cc->flags);
+#ifdef CONFIG_BLK_INLINE_ENCRYPTION
+		num_feature_args +=
+			test_bit(DM_CRYPT_INLINE_ENCRYPTION, &cc->flags);
+#endif
 		num_feature_args += cc->sector_size != (1 << SECTOR_SHIFT);
 		num_feature_args += test_bit(CRYPT_IV_LARGE_SECTORS, &cc->cipher_flags);
 		if (cc->on_disk_tag_size)
@@ -3497,6 +3640,10 @@ static void crypt_status(struct dm_target *ti, status_type_t type,
 				DMEMIT(" no_read_workqueue");
 			if (test_bit(DM_CRYPT_NO_WRITE_WORKQUEUE, &cc->flags))
 				DMEMIT(" no_write_workqueue");
+#ifdef CONFIG_BLK_INLINE_ENCRYPTION
+			if (test_bit(DM_CRYPT_INLINE_ENCRYPTION, &cc->flags))
+				DMEMIT(" inline_crypt");
+#endif
 			if (cc->on_disk_tag_size)
 				DMEMIT(" integrity:%u:%s", cc->on_disk_tag_size, cc->cipher_auth);
 			if (cc->sector_size != (1 << SECTOR_SHIFT))
@@ -3516,6 +3663,11 @@ static void crypt_status(struct dm_target *ti, status_type_t type,
 		       'y' : 'n');
 		DMEMIT(",no_write_workqueue=%c", test_bit(DM_CRYPT_NO_WRITE_WORKQUEUE, &cc->flags) ?
 		       'y' : 'n');
+#ifdef CONFIG_BLK_INLINE_ENCRYPTION
+		DMEMIT(",inline_crypt=%c",
+		       test_bit(DM_CRYPT_INLINE_ENCRYPTION, &cc->flags) ?
+		       'y' : 'n');
+#endif
 		DMEMIT(",iv_large_sectors=%c", test_bit(CRYPT_IV_LARGE_SECTORS, &cc->cipher_flags) ?
 		       'y' : 'n');
 
-- 
2.27.0

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

Re: [dm-devel] [RFC PATCH 0/1] Add inline encryption support for dm-crypt

[Bart Van Assche]

On 1/13/22 10:09, Israel Rukshin wrote:
> I am working to add support for inline encryption/decryption
> at storage protocols like nvmf over RDMA. The HW that I am
> working with is ConnecX-6 Dx, which supports inline crypto
> and can send the data on the fabric at the same time.
> 
> This patchset is based on v5.16-rc4 with Eric Biggers patches
> of the HW wrapped keys.
> It can be retrieved from branch "wip-wrapped-keys" at
> https://git.kernel.org/pub/scm/fs/fscrypt/fscrypt.git
> 
> I tested this patch with modified nvme-rdma as the block device
> and created a DM crypt on top of it. I got performance enhancement
> compared to SW crypto. I tested the HW wrapped inline mode with
> the HW and the standard mode with a fallback algorithm.

Hi Israel,

Thank you for your work. For future patches related to inline 
encryption, please Cc Eric Biggers.

Thanks,

Bart.

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

Re: [dm-devel] [PATCH 1/1] dm crypt: Add inline encryption support

[Damien Le Moal]

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

Re: [dm-devel] [PATCH 1/1] dm crypt: Add inline encryption support

[Damien Le Moal]

On 2022/01/14 4:11, Israel Rukshin wrote:
> Using inline encryption means that the block layer handles the
> decryption/encryption as part of the bio, instead of dm-crypt
> doing the crypto by itself via Linux's crypto API. This model
> is needed to take advantage of the inline encryption hardware
> on the market.
> 
> To use inline encryption, the new dm-crypt optional parameter
> "inline_crypt" should be set for the configured mapping. Afterwards,
> dm-crypt will provide the crypto parameters to the block layer by
> creating a cypto profile and by filling the bios with crypto context.
> In case the block device or the fallback algorithm doesn't support
> this feature, the mapping will fail.
> 
> Signed-off-by: Israel Rukshin <israelr@nvidia.com>
> ---
>  block/blk-crypto.c    |   3 +
>  drivers/md/dm-crypt.c | 202 ++++++++++++++++++++++++++++++++++++------
>  2 files changed, 180 insertions(+), 25 deletions(-)
> 
> diff --git a/block/blk-crypto.c b/block/blk-crypto.c
> index 1c08d3b6e24a..65f13549eb5f 100644
> --- a/block/blk-crypto.c
> +++ b/block/blk-crypto.c
> @@ -102,6 +102,7 @@ void bio_crypt_set_ctx(struct bio *bio, const struct blk_crypto_key *key,
>  
>  	bio->bi_crypt_context = bc;
>  }
> +EXPORT_SYMBOL_GPL(bio_crypt_set_ctx);
>  
>  void __bio_crypt_free_ctx(struct bio *bio)
>  {
> @@ -370,6 +371,7 @@ int blk_crypto_init_key(struct blk_crypto_key *blk_key,
>  
>  	return 0;
>  }
> +EXPORT_SYMBOL_GPL(blk_crypto_init_key);
>  
>  /*
>   * Check if bios with @cfg can be en/decrypted by blk-crypto (i.e. either the
> @@ -411,6 +413,7 @@ int blk_crypto_start_using_key(const struct blk_crypto_key *key,
>  	}
>  	return blk_crypto_fallback_start_using_mode(key->crypto_cfg.crypto_mode);
>  }
> +EXPORT_SYMBOL_GPL(blk_crypto_start_using_key);
>  
>  /**
>   * blk_crypto_evict_key() - Evict a key from any inline encryption hardware

These changes could probably go into a separate prep patch.

> diff --git a/drivers/md/dm-crypt.c b/drivers/md/dm-crypt.c
> index d4ae31558826..4c0e1904942b 100644
> --- a/drivers/md/dm-crypt.c
> +++ b/drivers/md/dm-crypt.c
> @@ -39,6 +39,7 @@
>  #include <keys/user-type.h>
>  #include <keys/encrypted-type.h>
>  #include <keys/trusted-type.h>
> +#include <linux/blk-crypto.h>
>  
>  #include <linux/device-mapper.h>
>  
> @@ -134,7 +135,7 @@ struct iv_elephant_private {
>  enum flags { DM_CRYPT_SUSPENDED, DM_CRYPT_KEY_VALID,
>  	     DM_CRYPT_SAME_CPU, DM_CRYPT_NO_OFFLOAD,
>  	     DM_CRYPT_NO_READ_WORKQUEUE, DM_CRYPT_NO_WRITE_WORKQUEUE,
> -	     DM_CRYPT_WRITE_INLINE };
> +	     DM_CRYPT_WRITE_INLINE, DM_CRYPT_INLINE_ENCRYPTION };
>  
>  enum cipher_flags {
>  	CRYPT_MODE_INTEGRITY_AEAD,	/* Use authenticated mode for cipher */
> @@ -220,6 +221,11 @@ struct crypt_config {
>  	struct bio_set bs;
>  	struct mutex bio_alloc_lock;
>  
> +#ifdef CONFIG_BLK_INLINE_ENCRYPTION
> +	enum blk_crypto_mode_num crypto_mode;
> +	enum blk_crypto_key_type key_type;
> +	struct blk_crypto_key *blk_key;
> +#endif
>  	u8 *authenc_key; /* space for keys in authenc() format (if used) */
>  	u8 key[];
>  };
> @@ -2383,11 +2389,103 @@ static void crypt_copy_authenckey(char *p, const void *key,
>  	memcpy(p, key, enckeylen);
>  }
>  
> +#ifdef CONFIG_BLK_INLINE_ENCRYPTION
> +static int crypt_select_inline_crypt_mode(struct dm_target *ti, char *cipher,
> +					  char *ivmode)
> +{
> +	struct crypt_config *cc = ti->private;
> +
> +	if (strcmp(cipher, "xts(aes)") == 0) {
> +		cc->crypto_mode = BLK_ENCRYPTION_MODE_AES_256_XTS;
> +		cc->key_type = BLK_CRYPTO_KEY_TYPE_STANDARD;
> +	} else if (strcmp(cipher, "xts(paes)") == 0) {
> +		cc->crypto_mode = BLK_ENCRYPTION_MODE_AES_256_XTS;
> +		cc->key_type = BLK_CRYPTO_KEY_TYPE_HW_WRAPPED;
> +	} else {
> +		ti->error = "Invalid cipher for inline_crypt";
> +		return -EINVAL;
> +	}
> +
> +	if (ivmode == NULL || (strcmp(ivmode, "plain64") == 0)) {
> +		cc->iv_size = 8;
> +	} else {
> +		ti->error = "Invalid IV mode for inline_crypt";
> +		return -EINVAL;
> +	}
> +
> +	return 0;
> +}
> +
> +static int crypt_prepare_inline_crypt_key(struct crypt_config *cc)
> +{
> +	int ret;
> +
> +	cc->blk_key = kzalloc(sizeof(*cc->blk_key), GFP_KERNEL);
> +	if (!cc->blk_key)
> +		return -ENOMEM;
> +
> +	ret = blk_crypto_init_key(cc->blk_key, cc->key, cc->key_size,
> +				  cc->key_type, cc->crypto_mode, cc->iv_size,
> +				  cc->sector_size);
> +	if (ret) {
> +		DMERR("Failed to init inline encryption key");
> +		goto bad_key;
> +	}
> +
> +	ret = blk_crypto_start_using_key(cc->blk_key,
> +					 bdev_get_queue(cc->dev->bdev));
> +	if (ret) {
> +		DMERR("Failed to use inline encryption key");
> +		goto bad_key;
> +	}
> +
> +	return 0;
> +bad_key:
> +	kfree_sensitive(cc->blk_key);
> +	cc->blk_key = NULL;
> +	return ret;
> +}
> +
> +static void crypt_destroy_inline_crypt_key(struct crypt_config *cc)
> +{
> +	if (cc->blk_key) {
> +		blk_crypto_evict_key(bdev_get_queue(cc->dev->bdev),
> +				     cc->blk_key);
> +		kfree_sensitive(cc->blk_key);
> +		cc->blk_key = NULL;
> +	}
> +}
> +
> +static void crypt_inline_encrypt_submit(struct dm_target *ti, struct bio *bio)
> +{
> +	struct crypt_config *cc = ti->private;
> +	u64 dun[BLK_CRYPTO_DUN_ARRAY_SIZE];
> +
> +	bio_set_dev(bio, cc->dev->bdev);
> +	if (bio_sectors(bio)) {
> +		memset(dun, 0, BLK_CRYPTO_MAX_IV_SIZE);
> +		bio->bi_iter.bi_sector = cc->start +
> +			dm_target_offset(ti, bio->bi_iter.bi_sector);
> +		dun[0] = le64_to_cpu(bio->bi_iter.bi_sector + cc->iv_offset);
> +		bio_crypt_set_ctx(bio, cc->blk_key, dun, GFP_KERNEL);
> +	}
> +
> +	submit_bio_noacct(bio);
> +}

#else

define the above functions as empty (see below).

> +#endif
> +
>  static int crypt_setkey(struct crypt_config *cc)
>  {
>  	unsigned subkey_size;
>  	int err = 0, i, r;
>  
> +#ifdef CONFIG_BLK_INLINE_ENCRYPTION
> +	if (test_bit(DM_CRYPT_INLINE_ENCRYPTION, &cc->flags)) {
> +		crypt_destroy_inline_crypt_key(cc);
> +		return crypt_prepare_inline_crypt_key(cc);
> +	}
> +#endif

You could avoid the ifdef here using IS_ENABLED(CONFIG_BLK_INLINE_ENCRYPTION)
directly in the if condition. That will make the code cleaner. That said, since
DM_CRYPT_INLINE_ENCRYPTION can only be set if CONFIG_BLK_INLINE_ENCRYPTION is
enabled, I am not sure if the ifdef buys you much in the
!CONFIG_BLK_INLINE_ENCRYPTION case.

> +
>  	/* Ignore extra keys (which are used for IV etc) */
>  	subkey_size = crypt_subkey_size(cc);
>  
> @@ -2648,6 +2746,15 @@ static int crypt_wipe_key(struct crypt_config *cc)
>  
>  	kfree_sensitive(cc->key_string);
>  	cc->key_string = NULL;
> +
> +#ifdef CONFIG_BLK_INLINE_ENCRYPTION
> +	if (test_bit(DM_CRYPT_INLINE_ENCRYPTION, &cc->flags)) {
> +		crypt_destroy_inline_crypt_key(cc);
> +		memset(&cc->key, 0, cc->key_size * sizeof(u8));
> +		return 0;
> +	}
> +#endif

same comment as above and for most of the following ifdef additions.

> +
>  	r = crypt_setkey(cc);
>  	memset(&cc->key, 0, cc->key_size * sizeof(u8));
>  
> @@ -2713,6 +2820,10 @@ static void crypt_dtr(struct dm_target *ti)
>  	if (cc->crypt_queue)
>  		destroy_workqueue(cc->crypt_queue);
>  
> +#ifdef CONFIG_BLK_INLINE_ENCRYPTION
> +	crypt_destroy_inline_crypt_key(cc);
> +#endif

You can avoid the ifdef here if this function is defined as empty in the
!CONFIG_BLK_INLINE_ENCRYPTION case (see above the comment about #else).

> +
>  	crypt_free_tfms(cc);
>  
>  	bioset_exit(&cc->bs);
> @@ -2888,6 +2999,11 @@ static int crypt_ctr_cipher_new(struct dm_target *ti, char *cipher_in, char *key
>  	/* The rest is crypto API spec */
>  	cipher_api = tmp;
>  
> +#ifdef CONFIG_BLK_INLINE_ENCRYPTION
> +	if (test_bit(DM_CRYPT_INLINE_ENCRYPTION, &cc->flags))
> +		return crypt_select_inline_crypt_mode(ti, cipher_api, *ivmode);
> +#endif
> +
>  	/* Alloc AEAD, can be used only in new format. */
>  	if (crypt_integrity_aead(cc)) {
>  		ret = crypt_ctr_auth_cipher(cc, cipher_api);
> @@ -3001,6 +3117,11 @@ static int crypt_ctr_cipher_old(struct dm_target *ti, char *cipher_in, char *key
>  		goto bad_mem;
>  	}
>  
> +#ifdef CONFIG_BLK_INLINE_ENCRYPTION
> +	if (test_bit(DM_CRYPT_INLINE_ENCRYPTION, &cc->flags))
> +		return crypt_select_inline_crypt_mode(ti, cipher_api, *ivmode);
> +#endif
> +
>  	/* Allocate cipher */
>  	ret = crypt_alloc_tfms(cc, cipher_api);
>  	if (ret < 0) {
> @@ -3036,9 +3157,11 @@ static int crypt_ctr_cipher(struct dm_target *ti, char *cipher_in, char *key)
>  		return ret;
>  
>  	/* Initialize IV */
> -	ret = crypt_ctr_ivmode(ti, ivmode);
> -	if (ret < 0)
> -		return ret;
> +	if (!test_bit(DM_CRYPT_INLINE_ENCRYPTION, &cc->flags)) {
> +		ret = crypt_ctr_ivmode(ti, ivmode);
> +		if (ret < 0)
> +			return ret;
> +	}
>  
>  	/* Initialize and set key */
>  	ret = crypt_set_key(cc, key);
> @@ -3111,6 +3234,10 @@ static int crypt_ctr_optional(struct dm_target *ti, unsigned int argc, char **ar
>  			set_bit(DM_CRYPT_NO_READ_WORKQUEUE, &cc->flags);
>  		else if (!strcasecmp(opt_string, "no_write_workqueue"))
>  			set_bit(DM_CRYPT_NO_WRITE_WORKQUEUE, &cc->flags);
> +#ifdef CONFIG_BLK_INLINE_ENCRYPTION
> +		else if (!strcasecmp(opt_string, "inline_crypt"))
> +			set_bit(DM_CRYPT_INLINE_ENCRYPTION, &cc->flags);
> +#endif

May be add a warning here for the !CONFIG_BLK_INLINE_ENCRYPTION case ?

>  		else if (sscanf(opt_string, "integrity:%u:", &val) == 1) {
>  			if (val == 0 || val > MAX_TAG_SIZE) {
>  				ti->error = "Invalid integrity arguments";
> @@ -3218,10 +3345,36 @@ static int crypt_ctr(struct dm_target *ti, unsigned int argc, char **argv)
>  			goto bad;
>  	}
>  
> +	ret = -EINVAL;
> +	if ((sscanf(argv[2], "%llu%c", &tmpll, &dummy) != 1) ||
> +	    (tmpll & ((cc->sector_size >> SECTOR_SHIFT) - 1))) {
> +		ti->error = "Invalid iv_offset sector";
> +		goto bad;
> +	}
> +	cc->iv_offset = tmpll;
> +
> +	ret = dm_get_device(ti, argv[3], dm_table_get_mode(ti->table),
> +			    &cc->dev);
> +	if (ret) {
> +		ti->error = "Device lookup failed";
> +		goto bad;
> +	}
> +
> +	ret = -EINVAL;
> +	if (sscanf(argv[4], "%llu%c", &tmpll, &dummy) != 1 ||
> +	    tmpll != (sector_t)tmpll) {
> +		ti->error = "Invalid device sector";
> +		goto bad;
> +	}
> +	cc->start = tmpll;
> +
>  	ret = crypt_ctr_cipher(ti, argv[0], argv[1]);
>  	if (ret < 0)
>  		goto bad;
>  
> +	if (test_bit(DM_CRYPT_INLINE_ENCRYPTION, &cc->flags))
> +		return 0;
> +
>  	if (crypt_integrity_aead(cc)) {
>  		cc->dmreq_start = sizeof(struct aead_request);
>  		cc->dmreq_start += crypto_aead_reqsize(any_tfm_aead(cc));
> @@ -3277,27 +3430,6 @@ static int crypt_ctr(struct dm_target *ti, unsigned int argc, char **argv)
>  
>  	mutex_init(&cc->bio_alloc_lock);
>  
> -	ret = -EINVAL;
> -	if ((sscanf(argv[2], "%llu%c", &tmpll, &dummy) != 1) ||
> -	    (tmpll & ((cc->sector_size >> SECTOR_SHIFT) - 1))) {
> -		ti->error = "Invalid iv_offset sector";
> -		goto bad;
> -	}
> -	cc->iv_offset = tmpll;
> -
> -	ret = dm_get_device(ti, argv[3], dm_table_get_mode(ti->table), &cc->dev);
> -	if (ret) {
> -		ti->error = "Device lookup failed";
> -		goto bad;
> -	}
> -
> -	ret = -EINVAL;
> -	if (sscanf(argv[4], "%llu%c", &tmpll, &dummy) != 1 || tmpll != (sector_t)tmpll) {
> -		ti->error = "Invalid device sector";
> -		goto bad;
> -	}
> -	cc->start = tmpll;
> -
>  	if (bdev_is_zoned(cc->dev->bdev)) {
>  		/*
>  		 * For zoned block devices, we need to preserve the issuer write
> @@ -3419,6 +3551,13 @@ static int crypt_map(struct dm_target *ti, struct bio *bio)
>  	if (unlikely(bio->bi_iter.bi_size & (cc->sector_size - 1)))
>  		return DM_MAPIO_KILL;
>  
> +#ifdef CONFIG_BLK_INLINE_ENCRYPTION
> +	if (test_bit(DM_CRYPT_INLINE_ENCRYPTION, &cc->flags)) {
> +		crypt_inline_encrypt_submit(ti, bio);
> +		return DM_MAPIO_SUBMITTED;
> +	}
> +#endif
> +
>  	io = dm_per_bio_data(bio, cc->per_bio_data_size);
>  	crypt_io_init(io, cc, bio, dm_target_offset(ti, bio->bi_iter.bi_sector));
>  
> @@ -3481,6 +3620,10 @@ static void crypt_status(struct dm_target *ti, status_type_t type,
>  		num_feature_args += test_bit(DM_CRYPT_NO_OFFLOAD, &cc->flags);
>  		num_feature_args += test_bit(DM_CRYPT_NO_READ_WORKQUEUE, &cc->flags);
>  		num_feature_args += test_bit(DM_CRYPT_NO_WRITE_WORKQUEUE, &cc->flags);
> +#ifdef CONFIG_BLK_INLINE_ENCRYPTION
> +		num_feature_args +=
> +			test_bit(DM_CRYPT_INLINE_ENCRYPTION, &cc->flags);
> +#endif

You do not think that you need the ifdef here. If CONFIG_BLK_INLINE_ENCRYPTION
is not enabled, then DM_CRYPT_INLINE_ENCRYPTION is never set.

>  		num_feature_args += cc->sector_size != (1 << SECTOR_SHIFT);
>  		num_feature_args += test_bit(CRYPT_IV_LARGE_SECTORS, &cc->cipher_flags);
>  		if (cc->on_disk_tag_size)
> @@ -3497,6 +3640,10 @@ static void crypt_status(struct dm_target *ti, status_type_t type,
>  				DMEMIT(" no_read_workqueue");
>  			if (test_bit(DM_CRYPT_NO_WRITE_WORKQUEUE, &cc->flags))
>  				DMEMIT(" no_write_workqueue");
> +#ifdef CONFIG_BLK_INLINE_ENCRYPTION
> +			if (test_bit(DM_CRYPT_INLINE_ENCRYPTION, &cc->flags))
> +				DMEMIT(" inline_crypt");
> +#endif

ditto.

>  			if (cc->on_disk_tag_size)
>  				DMEMIT(" integrity:%u:%s", cc->on_disk_tag_size, cc->cipher_auth);
>  			if (cc->sector_size != (1 << SECTOR_SHIFT))
> @@ -3516,6 +3663,11 @@ static void crypt_status(struct dm_target *ti, status_type_t type,
>  		       'y' : 'n');
>  		DMEMIT(",no_write_workqueue=%c", test_bit(DM_CRYPT_NO_WRITE_WORKQUEUE, &cc->flags) ?
>  		       'y' : 'n');
> +#ifdef CONFIG_BLK_INLINE_ENCRYPTION
> +		DMEMIT(",inline_crypt=%c",
> +		       test_bit(DM_CRYPT_INLINE_ENCRYPTION, &cc->flags) ?
> +		       'y' : 'n');
> +#endif

You do not think that you need the ifdef here. If CONFIG_BLK_INLINE_ENCRYPTION
is not enabled, then inline_crypt=n will always be reported, which is fine I think.

>  		DMEMIT(",iv_large_sectors=%c", test_bit(CRYPT_IV_LARGE_SECTORS, &cc->cipher_flags) ?
>  		       'y' : 'n');
>  


-- 
Damien Le Moal
Western Digital Research

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

Re: [dm-devel] [RFC PATCH 0/1] Add inline encryption support for dm-crypt

[Milan Broz]

On 13/01/2022 19:09, Israel Rukshin wrote:
> Hi,
> 
> I am working to add support for inline encryption/decryption
> at storage protocols like nvmf over RDMA. The HW that I am
> working with is ConnecX-6 Dx, which supports inline crypto
> and can send the data on the fabric at the same time.

This idea comes from time to time, and it makes dm-crypt bloated,
and mainly it moves responsibility for encryption to block layer
crypto.
It adds two completely different sector encryption paths there.

Also, see my comments here (for similar patches)
https://lore.kernel.org/dm-devel/c94d425a-bca4-8a8b-47bf-451239d29ebd@gmail.com/

I think dm-crypt should stay as SW crypto only (using kernel crypto API,
so HW acceleration is done through crypto drivers there).

A cleaner solution is to write a much simpler new dm-crypt-inline target,
which will implement only inline encryption.
(And userspace can decide which target to use.)
Code should be just an extension to the dm-linear target, most
of dm-crypt complexity is not needed here.

Also, please think about configuration - how do you want to configure it?

Just my opinion, it is, of course, up to DM maintainer if he takes such patches.

Thanks,
Milan

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

Re: [dm-devel] [RFC PATCH 0/1] Add inline encryption support for dm-crypt

[Eric Biggers]

On Fri, Jan 14, 2022 at 09:51:20PM +0100, Milan Broz wrote:
> On 13/01/2022 19:09, Israel Rukshin wrote:
> > Hi,
> > 
> > I am working to add support for inline encryption/decryption
> > at storage protocols like nvmf over RDMA. The HW that I am
> > working with is ConnecX-6 Dx, which supports inline crypto
> > and can send the data on the fabric at the same time.
> 
> This idea comes from time to time, and it makes dm-crypt bloated,
> and mainly it moves responsibility for encryption to block layer
> crypto.
> It adds two completely different sector encryption paths there.
> 
> Also, see my comments here (for similar patches)
> https://lore.kernel.org/dm-devel/c94d425a-bca4-8a8b-47bf-451239d29ebd@gmail.com/
> 
> I think dm-crypt should stay as SW crypto only (using kernel crypto API,
> so HW acceleration is done through crypto drivers there).
> 
> A cleaner solution is to write a much simpler new dm-crypt-inline target,
> which will implement only inline encryption.
> (And userspace can decide which target to use.)
> Code should be just an extension to the dm-linear target, most
> of dm-crypt complexity is not needed here.
> 
> Also, please think about configuration - how do you want to configure it?
> 
> Just my opinion, it is, of course, up to DM maintainer if he takes such patches.
> 

IMO, adding inline encryption support to dm-crypt would be fine.  Normally,
blk-crypto is just an alternate implementation of encryption/decryption.  I'm
not sure that a separate dm target is warranted just because of a different
implementation, as opposed to different *behavior*.  (Support for wrapped keys
does complicate things a bit, as they do change behavior.)  But, I'd also be
fine with a separate dm target if the dm maintainers prefer that route.

Note that in the Android Common Kernels, there is already a dm target called
"dm-default-key" which uses dm-crypt compatible syntax but uses blk-crypto
(inline encryption) rather than the crypto API:
https://android.googlesource.com/kernel/common/+/refs/heads/android-mainline/drivers/md/dm-default-key.c

It differs slightly from what is being proposed here in that dm-default-key's
purpose is to implement filesystem "metadata encryption", so it has logic to
skip encrypting blocks that have their encryption controlled at the filesystem
level due to being part of an encrypted file's contents.  I expect that logic
would be unacceptable upstream, as it's a layering violation.  (The long-term
plan is to handle metadata encryption entirely at the filesystem level instead.)

But with that potentially-controversial logic removed, it would basically be
dm-inline-crypt already, so it would be a good starting point.

- Eric

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

Re: [dm-devel] [RFC PATCH 0/1] Add inline encryption support for dm-crypt

[Eric Biggers]

Hi Israel,

On Thu, Jan 13, 2022 at 08:09:00PM +0200, Israel Rukshin wrote:
> Hi,
> 
> I am working to add support for inline encryption/decryption
> at storage protocols like nvmf over RDMA. The HW that I am
> working with is ConnecX-6 Dx, which supports inline crypto
> and can send the data on the fabric at the same time.
> 
> This patchset is based on v5.16-rc4 with Eric Biggers patches
> of the HW wrapped keys.
> It can be retrieved from branch "wip-wrapped-keys" at
> https://git.kernel.org/pub/scm/fs/fscrypt/fscrypt.git
> 
> I tested this patch with modified nvme-rdma as the block device
> and created a DM crypt on top of it. I got performance enhancement
> compared to SW crypto. I tested the HW wrapped inline mode with
> the HW and the standard mode with a fallback algorithm.
> 
> Israel Rukshin (1):
>   dm crypt: Add inline encryption support
> 
>  block/blk-crypto.c    |   3 +
>  drivers/md/dm-crypt.c | 202 ++++++++++++++++++++++++++++++++++++------
>  2 files changed, 180 insertions(+), 25 deletions(-)

I appreciate the interest in my patchset that adds support for hardware-wrapped
inline encryption keys
(https://lore.kernel.org/linux-block/20211116033240.39001-1-ebiggers@kernel.org).
So far I've received very little feedback on it.

One of the challenges I've been having is that I still have no platform on which
I can actually test hardware-wrapped keys with the upstream kernel.  The feature
cannot be accepted upstream until there is a way to test it.  It's almost
working on the SM8350 SoC, but I am waiting for Qualcomm to fix some things.

It sounds like you've implemented a block device driver that exposes support for
hardware-wrapped keys.  Can you please post that driver?

Can you also elaborate on how wrapped keys work in your case?  In particular,
are they compatible with the whole design which I've documented in detail in my
patchset?  That would include implementing the ->import_key, ->generate_key,
->prepare_key, and ->derive_sw_secret operations.  All the different parts are
important.  If something needs to be optional, that's something I can consider
incorporating into the design, but it would restrict the use cases.

Also, will your driver only support wrapped keys, or will it support standard
keys too?

- Eric

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

Re: [dm-devel] [RFC PATCH 0/1] Add inline encryption support for dm-crypt

[Milan Broz]

On 14/01/2022 23:27, Eric Biggers wrote:
> On Fri, Jan 14, 2022 at 09:51:20PM +0100, Milan Broz wrote:
>> On 13/01/2022 19:09, Israel Rukshin wrote:
>>> Hi,
>>>
>>> I am working to add support for inline encryption/decryption
>>> at storage protocols like nvmf over RDMA. The HW that I am
>>> working with is ConnecX-6 Dx, which supports inline crypto
>>> and can send the data on the fabric at the same time.
>>
>> This idea comes from time to time, and it makes dm-crypt bloated,
>> and mainly it moves responsibility for encryption to block layer
>> crypto.
>> It adds two completely different sector encryption paths there.
>>
>> Also, see my comments here (for similar patches)
>> https://lore.kernel.org/dm-devel/c94d425a-bca4-8a8b-47bf-451239d29ebd@gmail.com/
>>
>> I think dm-crypt should stay as SW crypto only (using kernel crypto API,
>> so HW acceleration is done through crypto drivers there).
>>
>> A cleaner solution is to write a much simpler new dm-crypt-inline target,
>> which will implement only inline encryption.
>> (And userspace can decide which target to use.)
>> Code should be just an extension to the dm-linear target, most
>> of dm-crypt complexity is not needed here.
>>
>> Also, please think about configuration - how do you want to configure it?
>>
>> Just my opinion, it is, of course, up to DM maintainer if he takes such patches.
>>
> 
> IMO, adding inline encryption support to dm-crypt would be fine.  Normally,
> blk-crypto is just an alternate implementation of encryption/decryption.  I'm
> not sure that a separate dm target is warranted just because of a different
> implementation, as opposed to different *behavior*.  (Support for wrapped keys
> does complicate things a bit, as they do change behavior.)  But, I'd also be
> fine with a separate dm target if the dm maintainers prefer that route.

I would expect some issues with FIPS people here (currently, it is handled
by enabling various crypto API drivers) about crypto boundaries and such stuff.
But it is up to the corporate people, not me.

Sadly, nobody did try to push some device-mapper functionality into the block layer.
Then inline encryption can be just a block device configuration or whatever....
(discussed in 2010 or so... https://lwn.net/Articles/400589/)
</sigh>

I would prefer to separate SW FDE (dm-crypt, as it is) and a target that delegates
encryption to the block layer (inline encryption).
But it is just cryptsetup's maintainer view, as we have already too complex
detection which features can be enabled on various kernel configs, etc.

> Note that in the Android Common Kernels, there is already a dm target called
> "dm-default-key" which uses dm-crypt compatible syntax but uses blk-crypto
> (inline encryption) rather than the crypto API:
> https://android.googlesource.com/kernel/common/+/refs/heads/android-mainline/drivers/md/dm-default-key.c

Any plans to submit this to mainline? Or it is just too controversial?

> 
> It differs slightly from what is being proposed here in that dm-default-key's
> purpose is to implement filesystem "metadata encryption", so it has logic to
> skip encrypting blocks that have their encryption controlled at the filesystem
> level due to being part of an encrypted file's contents.  I expect that logic
> would be unacceptable upstream, as it's a layering violation.  (The long-term
> plan is to handle metadata encryption entirely at the filesystem level instead.)

Well, I wish that we have strong authenticated encryption in filesystem even for
metadata, where it fits better in the fist place....
But fscrypt is still not here (or am I mistaken?)

Milan

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

Re: [dm-devel] [RFC PATCH 0/1] Add inline encryption support for dm-crypt

[Israel Rukshin]

Hi Eric,

On 1/15/2022 12:49 AM, Eric Biggers wrote:
> Hi Israel,
>
> On Thu, Jan 13, 2022 at 08:09:00PM +0200, Israel Rukshin wrote:
>> Hi,
>>
>> I am working to add support for inline encryption/decryption
>> at storage protocols like nvmf over RDMA. The HW that I am
>> working with is ConnecX-6 Dx, which supports inline crypto
>> and can send the data on the fabric at the same time.
>>
>> This patchset is based on v5.16-rc4 with Eric Biggers patches
>> of the HW wrapped keys.
>> It can be retrieved from branch "wip-wrapped-keys" at
>> https://git.kernel.org/pub/scm/fs/fscrypt/fscrypt.git
>>
>> I tested this patch with modified nvme-rdma as the block device
>> and created a DM crypt on top of it. I got performance enhancement
>> compared to SW crypto. I tested the HW wrapped inline mode with
>> the HW and the standard mode with a fallback algorithm.
>>
>> Israel Rukshin (1):
>>    dm crypt: Add inline encryption support
>>
>>   block/blk-crypto.c    |   3 +
>>   drivers/md/dm-crypt.c | 202 ++++++++++++++++++++++++++++++++++++------
>>   2 files changed, 180 insertions(+), 25 deletions(-)
> I appreciate the interest in my patchset that adds support for hardware-wrapped
> inline encryption keys
> (https://lore.kernel.org/linux-block/20211116033240.39001-1-ebiggers@kernel.org).
> So far I've received very little feedback on it.
>
> One of the challenges I've been having is that I still have no platform on which
> I can actually test hardware-wrapped keys with the upstream kernel.  The feature
> cannot be accepted upstream until there is a way to test it.  It's almost
> working on the SM8350 SoC, but I am waiting for Qualcomm to fix some things.
>
> It sounds like you've implemented a block device driver that exposes support for
> hardware-wrapped keys.  Can you please post that driver?

Yes, I implemented your inline callbacks at nvme-rdma driver. The driver 
communicates with

the HW via a general ib_verbs layer, so I had to extend ib_verbs and 
mlx5_ib drivers. Those

patches are at internal review and I will send the nvme-rdma patches 
afterwards.

>
> Can you also elaborate on how wrapped keys work in your case?  In particular,
> are they compatible with the whole design which I've documented in detail in my
> patchset?  That would include implementing the ->import_key, ->generate_key,
> ->prepare_key, and ->derive_sw_secret operations.  All the different parts are
> important.  If something needs to be optional, that's something I can consider
> incorporating into the design, but it would restrict the use cases.

In my case, the user should create wrapped keys by himself via a user 
space  tool based

on openssl library. Therefore, the ->import_key, ->generate_key and 
->prepare_key are

not necessary at my case. Regarding ->derive_sw_secret, it is not 
supported right now by

ConnectX6 DX firmware (we plan to add support for this).  To test 
fscrypt with wrapped keys,

a temporary WA was added to the ->derive_sw_secret at nvme-rdma driver. 
The other callbacks

you mentioned were left empty.

>
> Also, will your driver only support wrapped keys, or will it support standard
> keys too?

Our driver will support both modes. The first step is to support the 
standard keys, because of

the gap I mentioned above. As I understand, ->derive_sw_secret is not 
needed for dm-crypt.

Is that right?

>
> - Eric


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

Re: [dm-devel] [PATCH 1/1] dm crypt: Add inline encryption support

[Israel Rukshin]

Hi Damien,

On 1/14/2022 10:14 AM, Damien Le Moal wrote:
> On 2022/01/14 4:11, Israel Rukshin wrote:
>> Using inline encryption means that the block layer handles the
>> decryption/encryption as part of the bio, instead of dm-crypt
>> doing the crypto by itself via Linux's crypto API. This model
>> is needed to take advantage of the inline encryption hardware
>> on the market.
>>
>> To use inline encryption, the new dm-crypt optional parameter
>> "inline_crypt" should be set for the configured mapping. Afterwards,
>> dm-crypt will provide the crypto parameters to the block layer by
>> creating a cypto profile and by filling the bios with crypto context.
>> In case the block device or the fallback algorithm doesn't support
>> this feature, the mapping will fail.
>>
>> Signed-off-by: Israel Rukshin <israelr@nvidia.com>
>> ---
>>   block/blk-crypto.c    |   3 +
>>   drivers/md/dm-crypt.c | 202 ++++++++++++++++++++++++++++++++++++------
>>   2 files changed, 180 insertions(+), 25 deletions(-)
>>
>> diff --git a/block/blk-crypto.c b/block/blk-crypto.c
>> index 1c08d3b6e24a..65f13549eb5f 100644
>> --- a/block/blk-crypto.c
>> +++ b/block/blk-crypto.c
>> @@ -102,6 +102,7 @@ void bio_crypt_set_ctx(struct bio *bio, const struct blk_crypto_key *key,
>>   
>>   	bio->bi_crypt_context = bc;
>>   }
>> +EXPORT_SYMBOL_GPL(bio_crypt_set_ctx);
>>   
>>   void __bio_crypt_free_ctx(struct bio *bio)
>>   {
>> @@ -370,6 +371,7 @@ int blk_crypto_init_key(struct blk_crypto_key *blk_key,
>>   
>>   	return 0;
>>   }
>> +EXPORT_SYMBOL_GPL(blk_crypto_init_key);
>>   
>>   /*
>>    * Check if bios with @cfg can be en/decrypted by blk-crypto (i.e. either the
>> @@ -411,6 +413,7 @@ int blk_crypto_start_using_key(const struct blk_crypto_key *key,
>>   	}
>>   	return blk_crypto_fallback_start_using_mode(key->crypto_cfg.crypto_mode);
>>   }
>> +EXPORT_SYMBOL_GPL(blk_crypto_start_using_key);
>>   
>>   /**
>>    * blk_crypto_evict_key() - Evict a key from any inline encryption hardware
> These changes could probably go into a separate prep patch.
>
>> diff --git a/drivers/md/dm-crypt.c b/drivers/md/dm-crypt.c
>> index d4ae31558826..4c0e1904942b 100644
>> --- a/drivers/md/dm-crypt.c
>> +++ b/drivers/md/dm-crypt.c
>> @@ -39,6 +39,7 @@
>>   #include <keys/user-type.h>
>>   #include <keys/encrypted-type.h>
>>   #include <keys/trusted-type.h>
>> +#include <linux/blk-crypto.h>
>>   
>>   #include <linux/device-mapper.h>
>>   
>> @@ -134,7 +135,7 @@ struct iv_elephant_private {
>>   enum flags { DM_CRYPT_SUSPENDED, DM_CRYPT_KEY_VALID,
>>   	     DM_CRYPT_SAME_CPU, DM_CRYPT_NO_OFFLOAD,
>>   	     DM_CRYPT_NO_READ_WORKQUEUE, DM_CRYPT_NO_WRITE_WORKQUEUE,
>> -	     DM_CRYPT_WRITE_INLINE };
>> +	     DM_CRYPT_WRITE_INLINE, DM_CRYPT_INLINE_ENCRYPTION };
>>   
>>   enum cipher_flags {
>>   	CRYPT_MODE_INTEGRITY_AEAD,	/* Use authenticated mode for cipher */
>> @@ -220,6 +221,11 @@ struct crypt_config {
>>   	struct bio_set bs;
>>   	struct mutex bio_alloc_lock;
>>   
>> +#ifdef CONFIG_BLK_INLINE_ENCRYPTION
>> +	enum blk_crypto_mode_num crypto_mode;
>> +	enum blk_crypto_key_type key_type;
>> +	struct blk_crypto_key *blk_key;
>> +#endif
>>   	u8 *authenc_key; /* space for keys in authenc() format (if used) */
>>   	u8 key[];
>>   };
>> @@ -2383,11 +2389,103 @@ static void crypt_copy_authenckey(char *p, const void *key,
>>   	memcpy(p, key, enckeylen);
>>   }
>>   
>> +#ifdef CONFIG_BLK_INLINE_ENCRYPTION
>> +static int crypt_select_inline_crypt_mode(struct dm_target *ti, char *cipher,
>> +					  char *ivmode)
>> +{
>> +	struct crypt_config *cc = ti->private;
>> +
>> +	if (strcmp(cipher, "xts(aes)") == 0) {
>> +		cc->crypto_mode = BLK_ENCRYPTION_MODE_AES_256_XTS;
>> +		cc->key_type = BLK_CRYPTO_KEY_TYPE_STANDARD;
>> +	} else if (strcmp(cipher, "xts(paes)") == 0) {
>> +		cc->crypto_mode = BLK_ENCRYPTION_MODE_AES_256_XTS;
>> +		cc->key_type = BLK_CRYPTO_KEY_TYPE_HW_WRAPPED;
>> +	} else {
>> +		ti->error = "Invalid cipher for inline_crypt";
>> +		return -EINVAL;
>> +	}
>> +
>> +	if (ivmode == NULL || (strcmp(ivmode, "plain64") == 0)) {
>> +		cc->iv_size = 8;
>> +	} else {
>> +		ti->error = "Invalid IV mode for inline_crypt";
>> +		return -EINVAL;
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +static int crypt_prepare_inline_crypt_key(struct crypt_config *cc)
>> +{
>> +	int ret;
>> +
>> +	cc->blk_key = kzalloc(sizeof(*cc->blk_key), GFP_KERNEL);
>> +	if (!cc->blk_key)
>> +		return -ENOMEM;
>> +
>> +	ret = blk_crypto_init_key(cc->blk_key, cc->key, cc->key_size,
>> +				  cc->key_type, cc->crypto_mode, cc->iv_size,
>> +				  cc->sector_size);
>> +	if (ret) {
>> +		DMERR("Failed to init inline encryption key");
>> +		goto bad_key;
>> +	}
>> +
>> +	ret = blk_crypto_start_using_key(cc->blk_key,
>> +					 bdev_get_queue(cc->dev->bdev));
>> +	if (ret) {
>> +		DMERR("Failed to use inline encryption key");
>> +		goto bad_key;
>> +	}
>> +
>> +	return 0;
>> +bad_key:
>> +	kfree_sensitive(cc->blk_key);
>> +	cc->blk_key = NULL;
>> +	return ret;
>> +}
>> +
>> +static void crypt_destroy_inline_crypt_key(struct crypt_config *cc)
>> +{
>> +	if (cc->blk_key) {
>> +		blk_crypto_evict_key(bdev_get_queue(cc->dev->bdev),
>> +				     cc->blk_key);
>> +		kfree_sensitive(cc->blk_key);
>> +		cc->blk_key = NULL;
>> +	}
>> +}
>> +
>> +static void crypt_inline_encrypt_submit(struct dm_target *ti, struct bio *bio)
>> +{
>> +	struct crypt_config *cc = ti->private;
>> +	u64 dun[BLK_CRYPTO_DUN_ARRAY_SIZE];
>> +
>> +	bio_set_dev(bio, cc->dev->bdev);
>> +	if (bio_sectors(bio)) {
>> +		memset(dun, 0, BLK_CRYPTO_MAX_IV_SIZE);
>> +		bio->bi_iter.bi_sector = cc->start +
>> +			dm_target_offset(ti, bio->bi_iter.bi_sector);
>> +		dun[0] = le64_to_cpu(bio->bi_iter.bi_sector + cc->iv_offset);
>> +		bio_crypt_set_ctx(bio, cc->blk_key, dun, GFP_KERNEL);
>> +	}
>> +
>> +	submit_bio_noacct(bio);
>> +}
> #else
>
> define the above functions as empty (see below).
Good idea.
>
>> +#endif
>> +
>>   static int crypt_setkey(struct crypt_config *cc)
>>   {
>>   	unsigned subkey_size;
>>   	int err = 0, i, r;
>>   
>> +#ifdef CONFIG_BLK_INLINE_ENCRYPTION
>> +	if (test_bit(DM_CRYPT_INLINE_ENCRYPTION, &cc->flags)) {
>> +		crypt_destroy_inline_crypt_key(cc);
>> +		return crypt_prepare_inline_crypt_key(cc);
>> +	}
>> +#endif
> You could avoid the ifdef here using IS_ENABLED(CONFIG_BLK_INLINE_ENCRYPTION)
> directly in the if condition. That will make the code cleaner. That said, since
> DM_CRYPT_INLINE_ENCRYPTION can only be set if CONFIG_BLK_INLINE_ENCRYPTION is
> enabled, I am not sure if the ifdef buys you much in the
> !CONFIG_BLK_INLINE_ENCRYPTION case.
>
>> +
>>   	/* Ignore extra keys (which are used for IV etc) */
>>   	subkey_size = crypt_subkey_size(cc);
>>   
>> @@ -2648,6 +2746,15 @@ static int crypt_wipe_key(struct crypt_config *cc)
>>   
>>   	kfree_sensitive(cc->key_string);
>>   	cc->key_string = NULL;
>> +
>> +#ifdef CONFIG_BLK_INLINE_ENCRYPTION
>> +	if (test_bit(DM_CRYPT_INLINE_ENCRYPTION, &cc->flags)) {
>> +		crypt_destroy_inline_crypt_key(cc);
>> +		memset(&cc->key, 0, cc->key_size * sizeof(u8));
>> +		return 0;
>> +	}
>> +#endif
> same comment as above and for most of the following ifdef additions.
>
>> +
>>   	r = crypt_setkey(cc);
>>   	memset(&cc->key, 0, cc->key_size * sizeof(u8));
>>   
>> @@ -2713,6 +2820,10 @@ static void crypt_dtr(struct dm_target *ti)
>>   	if (cc->crypt_queue)
>>   		destroy_workqueue(cc->crypt_queue);
>>   
>> +#ifdef CONFIG_BLK_INLINE_ENCRYPTION
>> +	crypt_destroy_inline_crypt_key(cc);
>> +#endif
> You can avoid the ifdef here if this function is defined as empty in the
> !CONFIG_BLK_INLINE_ENCRYPTION case (see above the comment about #else).
>
>> +
>>   	crypt_free_tfms(cc);
>>   
>>   	bioset_exit(&cc->bs);
>> @@ -2888,6 +2999,11 @@ static int crypt_ctr_cipher_new(struct dm_target *ti, char *cipher_in, char *key
>>   	/* The rest is crypto API spec */
>>   	cipher_api = tmp;
>>   
>> +#ifdef CONFIG_BLK_INLINE_ENCRYPTION
>> +	if (test_bit(DM_CRYPT_INLINE_ENCRYPTION, &cc->flags))
>> +		return crypt_select_inline_crypt_mode(ti, cipher_api, *ivmode);
>> +#endif
>> +
>>   	/* Alloc AEAD, can be used only in new format. */
>>   	if (crypt_integrity_aead(cc)) {
>>   		ret = crypt_ctr_auth_cipher(cc, cipher_api);
>> @@ -3001,6 +3117,11 @@ static int crypt_ctr_cipher_old(struct dm_target *ti, char *cipher_in, char *key
>>   		goto bad_mem;
>>   	}
>>   
>> +#ifdef CONFIG_BLK_INLINE_ENCRYPTION
>> +	if (test_bit(DM_CRYPT_INLINE_ENCRYPTION, &cc->flags))
>> +		return crypt_select_inline_crypt_mode(ti, cipher_api, *ivmode);
>> +#endif
>> +
>>   	/* Allocate cipher */
>>   	ret = crypt_alloc_tfms(cc, cipher_api);
>>   	if (ret < 0) {
>> @@ -3036,9 +3157,11 @@ static int crypt_ctr_cipher(struct dm_target *ti, char *cipher_in, char *key)
>>   		return ret;
>>   
>>   	/* Initialize IV */
>> -	ret = crypt_ctr_ivmode(ti, ivmode);
>> -	if (ret < 0)
>> -		return ret;
>> +	if (!test_bit(DM_CRYPT_INLINE_ENCRYPTION, &cc->flags)) {
>> +		ret = crypt_ctr_ivmode(ti, ivmode);
>> +		if (ret < 0)
>> +			return ret;
>> +	}
>>   
>>   	/* Initialize and set key */
>>   	ret = crypt_set_key(cc, key);
>> @@ -3111,6 +3234,10 @@ static int crypt_ctr_optional(struct dm_target *ti, unsigned int argc, char **ar
>>   			set_bit(DM_CRYPT_NO_READ_WORKQUEUE, &cc->flags);
>>   		else if (!strcasecmp(opt_string, "no_write_workqueue"))
>>   			set_bit(DM_CRYPT_NO_WRITE_WORKQUEUE, &cc->flags);
>> +#ifdef CONFIG_BLK_INLINE_ENCRYPTION
>> +		else if (!strcasecmp(opt_string, "inline_crypt"))
>> +			set_bit(DM_CRYPT_INLINE_ENCRYPTION, &cc->flags);
>> +#endif
> May be add a warning here for the !CONFIG_BLK_INLINE_ENCRYPTION case ?

There is a general warring in case of an option doesn't match:

ti->error = "Invalid feature arguments";

>
>>   		else if (sscanf(opt_string, "integrity:%u:", &val) == 1) {
>>   			if (val == 0 || val > MAX_TAG_SIZE) {
>>   				ti->error = "Invalid integrity arguments";
>> @@ -3218,10 +3345,36 @@ static int crypt_ctr(struct dm_target *ti, unsigned int argc, char **argv)
>>   			goto bad;
>>   	}
>>   
>> +	ret = -EINVAL;
>> +	if ((sscanf(argv[2], "%llu%c", &tmpll, &dummy) != 1) ||
>> +	    (tmpll & ((cc->sector_size >> SECTOR_SHIFT) - 1))) {
>> +		ti->error = "Invalid iv_offset sector";
>> +		goto bad;
>> +	}
>> +	cc->iv_offset = tmpll;
>> +
>> +	ret = dm_get_device(ti, argv[3], dm_table_get_mode(ti->table),
>> +			    &cc->dev);
>> +	if (ret) {
>> +		ti->error = "Device lookup failed";
>> +		goto bad;
>> +	}
>> +
>> +	ret = -EINVAL;
>> +	if (sscanf(argv[4], "%llu%c", &tmpll, &dummy) != 1 ||
>> +	    tmpll != (sector_t)tmpll) {
>> +		ti->error = "Invalid device sector";
>> +		goto bad;
>> +	}
>> +	cc->start = tmpll;
>> +
>>   	ret = crypt_ctr_cipher(ti, argv[0], argv[1]);
>>   	if (ret < 0)
>>   		goto bad;
>>   
>> +	if (test_bit(DM_CRYPT_INLINE_ENCRYPTION, &cc->flags))
>> +		return 0;
>> +
>>   	if (crypt_integrity_aead(cc)) {
>>   		cc->dmreq_start = sizeof(struct aead_request);
>>   		cc->dmreq_start += crypto_aead_reqsize(any_tfm_aead(cc));
>> @@ -3277,27 +3430,6 @@ static int crypt_ctr(struct dm_target *ti, unsigned int argc, char **argv)
>>   
>>   	mutex_init(&cc->bio_alloc_lock);
>>   
>> -	ret = -EINVAL;
>> -	if ((sscanf(argv[2], "%llu%c", &tmpll, &dummy) != 1) ||
>> -	    (tmpll & ((cc->sector_size >> SECTOR_SHIFT) - 1))) {
>> -		ti->error = "Invalid iv_offset sector";
>> -		goto bad;
>> -	}
>> -	cc->iv_offset = tmpll;
>> -
>> -	ret = dm_get_device(ti, argv[3], dm_table_get_mode(ti->table), &cc->dev);
>> -	if (ret) {
>> -		ti->error = "Device lookup failed";
>> -		goto bad;
>> -	}
>> -
>> -	ret = -EINVAL;
>> -	if (sscanf(argv[4], "%llu%c", &tmpll, &dummy) != 1 || tmpll != (sector_t)tmpll) {
>> -		ti->error = "Invalid device sector";
>> -		goto bad;
>> -	}
>> -	cc->start = tmpll;
>> -
>>   	if (bdev_is_zoned(cc->dev->bdev)) {
>>   		/*
>>   		 * For zoned block devices, we need to preserve the issuer write
>> @@ -3419,6 +3551,13 @@ static int crypt_map(struct dm_target *ti, struct bio *bio)
>>   	if (unlikely(bio->bi_iter.bi_size & (cc->sector_size - 1)))
>>   		return DM_MAPIO_KILL;
>>   
>> +#ifdef CONFIG_BLK_INLINE_ENCRYPTION
>> +	if (test_bit(DM_CRYPT_INLINE_ENCRYPTION, &cc->flags)) {
>> +		crypt_inline_encrypt_submit(ti, bio);
>> +		return DM_MAPIO_SUBMITTED;
>> +	}
>> +#endif
>> +
>>   	io = dm_per_bio_data(bio, cc->per_bio_data_size);
>>   	crypt_io_init(io, cc, bio, dm_target_offset(ti, bio->bi_iter.bi_sector));
>>   
>> @@ -3481,6 +3620,10 @@ static void crypt_status(struct dm_target *ti, status_type_t type,
>>   		num_feature_args += test_bit(DM_CRYPT_NO_OFFLOAD, &cc->flags);
>>   		num_feature_args += test_bit(DM_CRYPT_NO_READ_WORKQUEUE, &cc->flags);
>>   		num_feature_args += test_bit(DM_CRYPT_NO_WRITE_WORKQUEUE, &cc->flags);
>> +#ifdef CONFIG_BLK_INLINE_ENCRYPTION
>> +		num_feature_args +=
>> +			test_bit(DM_CRYPT_INLINE_ENCRYPTION, &cc->flags);
>> +#endif
> You do not think that you need the ifdef here. If CONFIG_BLK_INLINE_ENCRYPTION
> is not enabled, then DM_CRYPT_INLINE_ENCRYPTION is never set.
>
>>   		num_feature_args += cc->sector_size != (1 << SECTOR_SHIFT);
>>   		num_feature_args += test_bit(CRYPT_IV_LARGE_SECTORS, &cc->cipher_flags);
>>   		if (cc->on_disk_tag_size)
>> @@ -3497,6 +3640,10 @@ static void crypt_status(struct dm_target *ti, status_type_t type,
>>   				DMEMIT(" no_read_workqueue");
>>   			if (test_bit(DM_CRYPT_NO_WRITE_WORKQUEUE, &cc->flags))
>>   				DMEMIT(" no_write_workqueue");
>> +#ifdef CONFIG_BLK_INLINE_ENCRYPTION
>> +			if (test_bit(DM_CRYPT_INLINE_ENCRYPTION, &cc->flags))
>> +				DMEMIT(" inline_crypt");
>> +#endif
> ditto.
>
>>   			if (cc->on_disk_tag_size)
>>   				DMEMIT(" integrity:%u:%s", cc->on_disk_tag_size, cc->cipher_auth);
>>   			if (cc->sector_size != (1 << SECTOR_SHIFT))
>> @@ -3516,6 +3663,11 @@ static void crypt_status(struct dm_target *ti, status_type_t type,
>>   		       'y' : 'n');
>>   		DMEMIT(",no_write_workqueue=%c", test_bit(DM_CRYPT_NO_WRITE_WORKQUEUE, &cc->flags) ?
>>   		       'y' : 'n');
>> +#ifdef CONFIG_BLK_INLINE_ENCRYPTION
>> +		DMEMIT(",inline_crypt=%c",
>> +		       test_bit(DM_CRYPT_INLINE_ENCRYPTION, &cc->flags) ?
>> +		       'y' : 'n');
>> +#endif
> You do not think that you need the ifdef here. If CONFIG_BLK_INLINE_ENCRYPTION
> is not enabled, then inline_crypt=n will always be reported, which is fine I think.
>
>>   		DMEMIT(",iv_large_sectors=%c", test_bit(CRYPT_IV_LARGE_SECTORS, &cc->cipher_flags) ?
>>   		       'y' : 'n');
>>   

Thanks for the review,

Israel

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

Re: [dm-devel] [RFC PATCH 0/1] Add inline encryption support for dm-crypt

[Christoph Hellwig]

On Fri, Jan 14, 2022 at 09:51:20PM +0100, Milan Broz wrote:
> I think dm-crypt should stay as SW crypto only (using kernel crypto API,
> so HW acceleration is done through crypto drivers there).
> 
> A cleaner solution is to write a much simpler new dm-crypt-inline target,
> which will implement only inline encryption.
> (And userspace can decide which target to use.)
> Code should be just an extension to the dm-linear target, most
> of dm-crypt complexity is not needed here.

Why do we even need a dm target for this as well?  There should be no
need to clone or remap bios, so I think hamdling inline crypto should be
just a small addition to the core block layer.

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

Re: [dm-devel] [RFC PATCH 0/1] Add inline encryption support for dm-crypt

[Milan Broz]

On 17/01/2022 08:52, Christoph Hellwig wrote:
> On Fri, Jan 14, 2022 at 09:51:20PM +0100, Milan Broz wrote:
>> I think dm-crypt should stay as SW crypto only (using kernel crypto API,
>> so HW acceleration is done through crypto drivers there).
>>
>> A cleaner solution is to write a much simpler new dm-crypt-inline target,
>> which will implement only inline encryption.
>> (And userspace can decide which target to use.)
>> Code should be just an extension to the dm-linear target, most
>> of dm-crypt complexity is not needed here.
> 
> Why do we even need a dm target for this as well?  There should be no
> need to clone or remap bios, so I think hamdling inline crypto should be
> just a small addition to the core block layer.

Well, yes, that was my question too :-)

Maybe there is need to have some new userspace utility to configure that
but otherwise I think that for inline encryption device mapper layer
only increases complexity and reduces IO performance.

Could anyone elaborate what it the reason for such DM extension?
Compatibility with existing encryption/key management tools (like LUKS)?
Easy support in LVM? ...

Milan

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

Re: [dm-devel] [RFC PATCH 0/1] Add inline encryption support for dm-crypt

[Israel Rukshin]

On 1/17/2022 12:50 PM, Milan Broz wrote:
> On 17/01/2022 08:52, Christoph Hellwig wrote:
>> On Fri, Jan 14, 2022 at 09:51:20PM +0100, Milan Broz wrote:
>>> I think dm-crypt should stay as SW crypto only (using kernel crypto 
>>> API,
>>> so HW acceleration is done through crypto drivers there).
>>>
>>> A cleaner solution is to write a much simpler new dm-crypt-inline 
>>> target,
>>> which will implement only inline encryption.
>>> (And userspace can decide which target to use.)
>>> Code should be just an extension to the dm-linear target, most
>>> of dm-crypt complexity is not needed here.
>>
>> Why do we even need a dm target for this as well?  There should be no
>> need to clone or remap bios, so I think hamdling inline crypto should be
>> just a small addition to the core block layer.
>
> Well, yes, that was my question too :-)
>
> Maybe there is need to have some new userspace utility to configure that
> but otherwise I think that for inline encryption device mapper layer
> only increases complexity and reduces IO performance.
>
Regarding performance degradation, I added only one if condition at the 
data path (inside #ifdef).
> Could anyone elaborate what it the reason for such DM extension?
> Compatibility with existing encryption/key management tools (like LUKS)?
> Easy support in LVM? ...

DM extension gives us several capabilities:

1. Use the Linux keyring and other key management tools.

     - I used "keyctl padd user test-key @u < /tmp/wrapped_dek" at my tests

2. Split a single block device into several DMs. Allow us to use a 
different encryption key and encryption mode per DM.

3. Replace a key during I/O by using "dmsetup suspend /dev/dm-0" and 
"dmsetup  resume /dev/dm-0".

>
> Milan


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

Re: [dm-devel] [RFC PATCH 0/1] Add inline encryption support for dm-crypt

[Christoph Hellwig]

On Mon, Jan 17, 2022 at 04:00:59PM +0200, Israel Rukshin wrote:
> DM extension gives us several capabilities:
> 
> 1. Use the Linux keyring and other key management tools.
> 
>     - I used "keyctl padd user test-key @u < /tmp/wrapped_dek" at my tests

Well, and kernel consumer can do that.

> 2. Split a single block device into several DMs. Allow us to use a different
> encryption key and encryption mode per DM.

If we allow setting a default key for every block device you can still
do that using normal dm-linear.

> 
> 3. Replace a key during I/O by using "dmsetup suspend /dev/dm-0" and
> "dmsetup  resume /dev/dm-0".

With a block layer ioctl that also works easily.

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

Re: [dm-devel] [RFC PATCH 0/1] Add inline encryption support for dm-crypt

[Eric Biggers]

On Sat, Jan 15, 2022 at 10:22:26AM +0100, Milan Broz wrote:
> > Note that in the Android Common Kernels, there is already a dm target called
> > "dm-default-key" which uses dm-crypt compatible syntax but uses blk-crypto
> > (inline encryption) rather than the crypto API:
> > https://android.googlesource.com/kernel/common/+/refs/heads/android-mainline/drivers/md/dm-default-key.c
> 
> Any plans to submit this to mainline? Or it is just too controversial?
> 
> > 
> > It differs slightly from what is being proposed here in that dm-default-key's
> > purpose is to implement filesystem "metadata encryption", so it has logic to
> > skip encrypting blocks that have their encryption controlled at the filesystem
> > level due to being part of an encrypted file's contents.  I expect that logic
> > would be unacceptable upstream, as it's a layering violation.  (The long-term
> > plan is to handle metadata encryption entirely at the filesystem level instead.)
> 
> Well, I wish that we have strong authenticated encryption in filesystem even for
> metadata, where it fits better in the fist place....
> But fscrypt is still not here (or am I mistaken?)
> 

I doubt that people would find Android's dm-default-key to be acceptable, given
that it's a layering violation, and a similar approach was rejected in the past
(https://lore.kernel.org/dm-devel/20170614234040.4326-1-mhalcrow@google.com/T/#u).
dm-default-key's purpose is filesystem metadata encryption; it encrypts all
blocks that aren't already part of an encrypted file's contents.  It differs
from dm-crypt + fscrypt together (which the upstream kernel already supports) in
that file contents aren't encrypted twice; this was a non-negotiable performance
requirement.  Obviously, this required a new flag in struct bio to indicate
which bios are reading/writing from an encrypted file's contents.  I doubt the
block layer people would find that to be acceptable.

In principle, the filesystem is the right place to implement metadata encryption
in this way.  This would also allow the kernel to enforce (via the key
hierarchy) that fscrypt keys are never weaker than the metadata encryption key.
Satya Tangirala previously implemented a proof of concept of this for f2fs
(https://lore.kernel.org/linux-f2fs-devel/20201217150435.1505269-1-satyat@google.com/T/#u).
Unfortunately, Satya has left Google and no one is currently working on this.
But it is still the long-term plan.

Authenticated encryption is certainly desirable, but not really feasible to
retrofit into filesystems that overwrite data in-place.  (Yes, dm-integrity
exists, but its performance impact is too much for the vast majority of users.)
Even f2fs isn't entirely log-structured; it often overwrites blocks in-place.

- Eric

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

Re: [dm-devel] [RFC PATCH 0/1] Add inline encryption support for dm-crypt

[Eric Biggers]

On Sun, Jan 16, 2022 at 12:15:22PM +0200, Israel Rukshin wrote:
> 
> Yes, I implemented your inline callbacks at nvme-rdma driver. The driver
> communicates with
> 
> the HW via a general ib_verbs layer, so I had to extend ib_verbs and mlx5_ib
> drivers. Those
> 
> patches are at internal review and I will send the nvme-rdma patches
> afterwards.
> 
> > 
> > Can you also elaborate on how wrapped keys work in your case?  In particular,
> > are they compatible with the whole design which I've documented in detail in my
> > patchset?  That would include implementing the ->import_key, ->generate_key,
> > ->prepare_key, and ->derive_sw_secret operations.  All the different parts are
> > important.  If something needs to be optional, that's something I can consider
> > incorporating into the design, but it would restrict the use cases.
> 
> In my case, the user should create wrapped keys by himself via a user space 
> tool based
> 
> on openssl library. Therefore, the ->import_key, ->generate_key and
> ->prepare_key are
> 
> not necessary at my case. Regarding ->derive_sw_secret, it is not supported
> right now by
> 
> ConnectX6 DX firmware (we plan to add support for this).  To test fscrypt
> with wrapped keys,
> 
> a temporary WA was added to the ->derive_sw_secret at nvme-rdma driver. The
> other callbacks
> 
> you mentioned were left empty.

So, what we need to think about is how userspace is expected to actually use and
test the hardware-wrapped keys feature.

My patchset proposed a design where if a block device declares support for
hardware-wrapped keys, then the BLKCRYPTOCREATEKEY and BLKCRYPTOPREPAREKEY
ioctls are available.

Additionally, a specific hardware-internal key hierarchy and KDF is assumed (due
to the need to support ->derive_sw_secret).  While userspace doesn't need to
know these details to use the feature normally, they *must* be known in order to
test that it's actually working correctly.

If we were to support variants of the feature, such as:

* Hardware-wrapped keys must be created/prepared in some way other than
  BLKCRYPTOCREATEKEY and BLKCRYPTOPREPAREKEY.  (Could you elaborate on 
  what this way actually is, in your case?)

* Hardware's key hierarchy or KDF is different, so userspace must do something
  else when verifying the on-disk ciphertext.

... then we would need to precisely specify these other variants, and define a
way for userspace to discover what it should do.

In https://lore.kernel.org/r/20211208013534.136590-1-ebiggers@kernel.org, I'm
proposing exposing the crypto capabilities of block devices via sysfs.  That
could lead to a partial solution, e.g. the kernel could provide a file

	/sys/block/$disk/queue/crypto/wrapped_keys_variant

... which would allow userspace to discover the supported variant of
hardware-wrapped keys.  I was really hoping that one variant could be
standardized, but that is one possibility.

> 
> > 
> > Also, will your driver only support wrapped keys, or will it support standard
> > keys too?
> 
> Our driver will support both modes. The first step is to support the
> standard keys, because of

Okay, great.  If you're adding inline encryption support to dm-crypt (or to
dm-inline-crypt or to the block layer, depending on what people decide is the
best approach), perhaps you should start with standard keys only, to avoid a
dependency on the hardware-wrapped keys feature which is still being worked on?

> 
> the gap I mentioned above. As I understand, ->derive_sw_secret is not needed
> for dm-crypt.
> 
> Is that right?

That's correct.  The larger issue is that if you don't support
->derive_sw_secret, then it's likely that your key hierarchy is different
(probably you don't have a "hierarchy", but rather just one key), which would
imply that the feature needs to be tested differently.

As per the above, this could be accounted for in the design by allowing multiple
variants of wrapped keys.  Of course, that would add complexity.

- Eric


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

Re: [dm-devel] [RFC PATCH 0/1] Add inline encryption support for dm-crypt

[Eric Biggers]

On Tue, Jan 18, 2022 at 08:45:25AM -0800, Christoph Hellwig wrote:
> On Mon, Jan 17, 2022 at 04:00:59PM +0200, Israel Rukshin wrote:
> > DM extension gives us several capabilities:
> > 
> > 1. Use the Linux keyring and other key management tools.
> > 
> >     - I used "keyctl padd user test-key @u < /tmp/wrapped_dek" at my tests
> 
> Well, and kernel consumer can do that.
> 
> > 2. Split a single block device into several DMs. Allow us to use a different
> > encryption key and encryption mode per DM.
> 
> If we allow setting a default key for every block device you can still
> do that using normal dm-linear.
> 
> > 
> > 3. Replace a key during I/O by using "dmsetup suspend /dev/dm-0" and
> > "dmsetup  resume /dev/dm-0".
> 
> With a block layer ioctl that also works easily.
> 

A while ago, I had looked into adding an ioctl to set a default key for a block
device.  There were a few things that led me to choose a dm target instead.  I'm
not sure how many of these are still relevant, but these are what I considered:

* The block device for a partition doesn't have its own request_queue or
  queue_limits; those are properties of the disk, not the partition.  But,
  setting an encryption key may require changes to the queue_limits.  For
  example, discard_zeroes_data will no longer work, and the logical_block_size
  will need to become the crypto data unit size which may be larger than the
  original logical_block_size.

* The block_device for a given partition didn't stay around while no one has it
  opened or mounted.  This may have been addressed by Christoph's changes last
  year that merged block_device and hd_struct, but this used to be an issue.

* There was some issue caused by the way the block layer maps partitions to
  disks; the knowledge of the original block device (and thus the key) was lost
  at this point.  I'm not sure whether this is still an issue or not.

* A block device ioctl to set a key would need to handle cases where the block
  device is already open (fail with EBUSY?), or already has pages cached in the
  pagecache (invalidate them?).  A dm target avoids these concerns since a key
  would only be set up when the disk and block device are originally created.

Finally, there's also the fact that this would really be more than "setting a
default key".  To precisely specify the encryption format, you also have to
specify the algorithm, the key type, and the data unit size.  (Also potentially
more details about IV generation, if blk-crypto ever starts to support more IV
generation methods, which I'd like to avoid but it might eventually happen.)

These could all be passed in an ioctl, but dm-crypt already has a syntax defined
for specifying encryption formats.  So it could make sense to reuse it.

Also as Israel indicated, people will want support for Linux keyring keys as an
alternative to raw keys.  A new ioctl could support this, though dm-crypt
already has a defined way to specify such keys.

If all these issues can be solved, then I'd be fine with the block device ioctl.

- Eric


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

Re: [dm-devel] [RFC PATCH 0/1] Add inline encryption support for dm-crypt

[Christoph Hellwig]

On Tue, Jan 18, 2022 at 12:27:33PM -0800, Eric Biggers wrote:
> * The block device for a partition doesn't have its own request_queue or
>   queue_limits; those are properties of the disk, not the partition.  But,
>   setting an encryption key may require changes to the queue_limits.  For
>   example, discard_zeroes_data will no longer work, and the logical_block_size
>   will need to become the crypto data unit size which may be larger than the
>   original logical_block_size.

If we need changes to the queue limits we're doing something wrong I
think, as all these limitation only actually apply to bios that use
inline encryption and thus should be dynamic decisions.

Note that discard_zeroes_data is gone already, all zeroing must use
REQ_OP_WRITE_ZEROES.

> 
> * The block_device for a given partition didn't stay around while no one has it
>   opened or mounted.  This may have been addressed by Christoph's changes last
>   year that merged block_device and hd_struct, but this used to be an issue.

Yes, this is fixed now.

> * There was some issue caused by the way the block layer maps partitions to
>   disks; the knowledge of the original block device (and thus the key) was lost
>   at this point.  I'm not sure whether this is still an issue or not.

Also fixed by the block_device/hd_struct merged as the lookup is gone
entirely now.

> * A block device ioctl to set a key would need to handle cases where the block
>   device is already open (fail with EBUSY?), or already has pages cached in the
>   pagecache (invalidate them?).  A dm target avoids these concerns since a key
>   would only be set up when the disk and block device are originally created.

An ioctl is by definition perfomed on an open file handle, so it will by
definition be open.  But I don't think that check really is needed to be
so strict.  We can require the ioctl to be on an FMODE_EXCL file handle
which is a good sanity check and otherwise you get what you ask for.

> 
> Finally, there's also the fact that this would really be more than "setting a
> default key".  To precisely specify the encryption format, you also have to
> specify the algorithm, the key type, and the data unit size.  (Also potentially
> more details about IV generation, if blk-crypto ever starts to support more IV
> generation methods, which I'd like to avoid but it might eventually happen.)
> 
> These could all be passed in an ioctl, but dm-crypt already has a syntax defined
> for specifying encryption formats.  So it could make sense to reuse it.

We could of course find a way to mostly reuse the dm-crypt text based setup
syntax even on a block device if that helps.

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

Re: [dm-devel] [RFC PATCH 0/1] Add inline encryption support for dm-crypt

[Christoph Hellwig]

On Tue, Jan 18, 2022 at 11:38:29AM -0800, Eric Biggers wrote:
> I doubt that people would find Android's dm-default-key to be acceptable, given
> that it's a layering violation, and a similar approach was rejected in the past
> (https://lore.kernel.org/dm-devel/20170614234040.4326-1-mhalcrow@google.com/T/#u).
> dm-default-key's purpose is filesystem metadata encryption; it encrypts all
> blocks that aren't already part of an encrypted file's contents.  It differs
> from dm-crypt + fscrypt together (which the upstream kernel already supports) in
> that file contents aren't encrypted twice; this was a non-negotiable performance
> requirement.  Obviously, this required a new flag in struct bio to indicate
> which bios are reading/writing from an encrypted file's contents.  I doubt the
> block layer people would find that to be acceptable.

Well, it was rejected because it pokes a hole into dm-crypt.  In a
purely inline crypto world a way to assign a key context if there is
none before is a little different, especially if it requires a different
setup than an unconditional encryption for the device.  It would also
not even require a flag.

> 
> In principle, the filesystem is the right place to implement metadata encryption
> in this way.  This would also allow the kernel to enforce (via the key
> hierarchy) that fscrypt keys are never weaker than the metadata encryption key.

Yes.  Especially in the inline crypto world it would seem just as
trivial to assign a default key in the file system itself.

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

Re: [dm-devel] [RFC PATCH 0/1] Add inline encryption support for dm-crypt

[Israel Rukshin]

On 1/18/2022 10:02 PM, Eric Biggers wrote:
> On Sun, Jan 16, 2022 at 12:15:22PM +0200, Israel Rukshin wrote:
>> Yes, I implemented your inline callbacks at nvme-rdma driver. The driver
>> communicates with
>>
>> the HW via a general ib_verbs layer, so I had to extend ib_verbs and mlx5_ib
>> drivers. Those
>>
>> patches are at internal review and I will send the nvme-rdma patches
>> afterwards.
>>
>>> Can you also elaborate on how wrapped keys work in your case?  In particular,
>>> are they compatible with the whole design which I've documented in detail in my
>>> patchset?  That would include implementing the ->import_key, ->generate_key,
>>> ->prepare_key, and ->derive_sw_secret operations.  All the different parts are
>>> important.  If something needs to be optional, that's something I can consider
>>> incorporating into the design, but it would restrict the use cases.
>> In my case, the user should create wrapped keys by himself via a user space
>> tool based
>>
>> on openssl library. Therefore, the ->import_key, ->generate_key and
>> ->prepare_key are
>>
>> not necessary at my case. Regarding ->derive_sw_secret, it is not supported
>> right now by
>>
>> ConnectX6 DX firmware (we plan to add support for this).  To test fscrypt
>> with wrapped keys,
>>
>> a temporary WA was added to the ->derive_sw_secret at nvme-rdma driver. The
>> other callbacks
>>
>> you mentioned were left empty.
> So, what we need to think about is how userspace is expected to actually use and
> test the hardware-wrapped keys feature.
>
> My patchset proposed a design where if a block device declares support for
> hardware-wrapped keys, then the BLKCRYPTOCREATEKEY and BLKCRYPTOPREPAREKEY
> ioctls are available.
>
> Additionally, a specific hardware-internal key hierarchy and KDF is assumed (due
> to the need to support ->derive_sw_secret).  While userspace doesn't need to
> know these details to use the feature normally, they *must* be known in order to
> test that it's actually working correctly.
>
> If we were to support variants of the feature, such as:
>
> * Hardware-wrapped keys must be created/prepared in some way other than
>    BLKCRYPTOCREATEKEY and BLKCRYPTOPREPAREKEY.  (Could you elaborate on
>    what this way actually is, in your case?)

We currently assume that the same entity that configured the device the first time
and pushed the first wrapping key to it is able to produce DEKs (Data Encryption keys)
which are passed to the software in a wrapped form, to be loaded to that particular
device as-is.
At our case, we produce the wrapped DEKs at a different machine which may be placed
at a more secured environment.

>
> * Hardware's key hierarchy or KDF is different, so userspace must do something
>    else when verifying the on-disk ciphertext.
>
> ... then we would need to precisely specify these other variants, and define a
> way for userspace to discover what it should do.
>
> In https://lore.kernel.org/r/20211208013534.136590-1-ebiggers@kernel.org, I'm
> proposing exposing the crypto capabilities of block devices via sysfs.  That
> could lead to a partial solution, e.g. the kernel could provide a file
>
> 	/sys/block/$disk/queue/crypto/wrapped_keys_variant
>
> ... which would allow userspace to discover the supported variant of
> hardware-wrapped keys.  I was really hoping that one variant could be
> standardized, but that is one possibility.
>
>>> Also, will your driver only support wrapped keys, or will it support standard
>>> keys too?
>> Our driver will support both modes. The first step is to support the
>> standard keys, because of
> Okay, great.  If you're adding inline encryption support to dm-crypt (or to
> dm-inline-crypt or to the block layer, depending on what people decide is the
> best approach), perhaps you should start with standard keys only, to avoid a
> dependency on the hardware-wrapped keys feature which is still being worked on?
Yes, I will do it.
>
>> the gap I mentioned above. As I understand, ->derive_sw_secret is not needed
>> for dm-crypt.
>>
>> Is that right?
> That's correct.  The larger issue is that if you don't support
> ->derive_sw_secret, then it's likely that your key hierarchy is different
> (probably you don't have a "hierarchy", but rather just one key), which would
> imply that the feature needs to be tested differently.
We plan to support also ->derive_sw_secret.
>
> As per the above, this could be accounted for in the design by allowing multiple
> variants of wrapped keys.  Of course, that would add complexity.
>
> - Eric


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