Re: [PATCH 2/2] scsi: ufs: add inline crypto support to UFS HCD

[Ladvine D Almeida]

On 12/12/18 5:52 AM, Parshuram Raju Thombare wrote:
> Hello Eric,
> 
> Thank you for a comment.
> 
>> -----Original Message-----
>> From: Eric Biggers <ebiggers@xxxxxxxxxx>
>> Sent: Tuesday, December 11, 2018 11:47 PM
>> To: Parshuram Raju Thombare <pthombar@xxxxxxxxxxx>
>> Cc: axboe@xxxxxxxxx; vinholikatti@xxxxxxxxx; jejb@xxxxxxxxxxxxxxxxxx;
>> martin.petersen@xxxxxxxxxx; mchehab+samsung@xxxxxxxxxx;
>> gregkh@xxxxxxxxxxxxxxxxxxx; davem@xxxxxxxxxxxxx; akpm@linux-
>> foundation.org; nicolas.ferre@xxxxxxxxxxxxx; arnd@xxxxxxxx; linux-
>> kernel@xxxxxxxxxxxxxxx; linux-block@xxxxxxxxxxxxxxx; linux-
>> scsi@xxxxxxxxxxxxxxx; Alan Douglas <adouglas@xxxxxxxxxxx>; Janek Kotas
>> <jank@xxxxxxxxxxx>; Rafal Ciepiela <rafalc@xxxxxxxxxxx>; AnilKumar
>> Chimata <anilc@xxxxxxxxxxxxxx>; Ladvine D Almeida <ladvine@xxxxxxxxxxxx>;
>> Satya Tangirala <satyat@xxxxxxxxxx>; Paul Crowley
>> <paulcrowley@xxxxxxxxxx>
>> Subject: Re: [PATCH 2/2] scsi: ufs: add inline crypto support to UFS HCD
>>
>> EXTERNAL MAIL
>>
>>
>> [+Cc other people who have been working on this]
Eric, Thanks for cc-ing me to the mail chain.

Parshuram,
Glad to know that you are working on the Inline Encryption support.
My concerns are mentioned inline below.

>>
>>
>>
>> Hi Parshuram,
>>
>>
>>
>> On Tue, Dec 11, 2018 at 09:50:27AM +0000, Parshuram Thombare wrote:
>>
>>> Add real time crypto support to UFS HCD using new device
>>
>>> mapper 'crypto-ufs'. dmsetup tool can be used to enable
>>
>>> real time / inline crypto support using device mapper
>>
>>> 'crypt-ufs'.

Where is Crypto target 'crypto-ufs' implementation available? Did you
submitted any other patch for the same?
Also, it is better to provide a generic name as the target is valid for
all other block devices.

>>
>>>
>>
>>> Signed-off-by: Parshuram Thombare <pthombar@xxxxxxxxxxx>
>>
>>> ---
>>
>>>  MAINTAINERS                      |    7 +
>>
>>>  block/Kconfig                    |    5 +
>>
>>>  drivers/scsi/ufs/Kconfig         |   12 +
>>
>>>  drivers/scsi/ufs/Makefile        |    1 +
>>
>>>  drivers/scsi/ufs/ufshcd-crypto.c |  453
>> ++++++++++++++++++++++++++++++++++++++
>>
>>>  drivers/scsi/ufs/ufshcd-crypto.h |  102 +++++++++
>>
>>>  drivers/scsi/ufs/ufshcd.c        |   27 +++-
>>
>>>  drivers/scsi/ufs/ufshcd.h        |    6 +
>>
>>>  drivers/scsi/ufs/ufshci.h        |    1 +
>>
>>>  9 files changed, 613 insertions(+), 1 deletions(-)
>>
>>>  create mode 100644 drivers/scsi/ufs/ufshcd-crypto.c
>>
>>>  create mode 100644 drivers/scsi/ufs/ufshcd-crypto.h
>>
>>>
>>
>>> diff --git a/MAINTAINERS b/MAINTAINERS
>>
>>> index f485597..3a68126 100644
>>
>>> --- a/MAINTAINERS
>>
>>> +++ b/MAINTAINERS
>>
>>> @@ -15340,6 +15340,13 @@ S:	Supported
>>
>>>  F:	Documentation/scsi/ufs.txt
>>
>>>  F:	drivers/scsi/ufs/
>>
>>>
>>
>>> +UNIVERSAL FLASH STORAGE HOST CONTROLLER CRYPTO DRIVER
>>
>>> +M:	Parshuram Thombare <pthombar@xxxxxxxxxxx>
>>
>>> +L:	linux-scsi@xxxxxxxxxxxxxxx
>>
>>> +S:	Supported
>>
>>> +F:	drivers/scsi/ufs/ufshcd-crypto.c
>>
>>> +F:	drivers/scsi/ufs/ufshcd-crypto.h
>>
>>> +
>>
>>>  UNIVERSAL FLASH STORAGE HOST CONTROLLER DRIVER DWC HOOKS
>>
>>>  M:	Joao Pinto <jpinto@xxxxxxxxxxxx>
>>
>>>  L:	linux-scsi@xxxxxxxxxxxxxxx
>>
>>> diff --git a/block/Kconfig b/block/Kconfig
>>
>>> index f7045aa..6afe131 100644
>>
>>> --- a/block/Kconfig
>>
>>> +++ b/block/Kconfig
>>
>>> @@ -224,4 +224,9 @@ config BLK_MQ_RDMA
>>
>>>  config BLK_PM
>>
>>>  	def_bool BLOCK && PM
>>
>>>
>>
>>> +config BLK_DEV_HW_RT_ENCRYPTION
>>
>>> +	bool
>>
>>> +	depends on SCSI_UFSHCD_RT_ENCRYPTION
>>
>>> +	default n
>>
>>> +
>>
>>>  source block/Kconfig.iosched
>>
>>> diff --git a/drivers/scsi/ufs/Kconfig b/drivers/scsi/ufs/Kconfig
>>
>>> index 2ddbb26..09a3ec0 100644
>>
>>> --- a/drivers/scsi/ufs/Kconfig
>>
>>> +++ b/drivers/scsi/ufs/Kconfig
>>
>>> @@ -136,3 +136,15 @@ config SCSI_UFS_BSG
>>
>>>
>>
>>>  	  Select this if you need a bsg device node for your UFS controller.
>>
>>>  	  If unsure, say N.
>>
>>> +
>>
>>> +config SCSI_UFSHCD_RT_ENCRYPTION
>>
>>> +	bool "Universal Flash Storage Controller RT encryption support"
>>
>>> +	depends on SCSI_UFSHCD
>>
>>> +	default n
>>
>>> +	select BLK_DEV_HW_RT_ENCRYPTION if SCSI_UFSHCD_RT_ENCRYPTION
>>
>>> +	select BLK_DEV_DM if SCSI_UFSHCD_RT_ENCRYPTION
>>
>>> +	help
>>
>>> +	Universal Flash Storage Controller RT encryption support
>>
>>> +
>>
>>> +	Select this if you want to enable real time encryption on UFS controller
>>
>>> +	If unsure, say N.
>>
>>> diff --git a/drivers/scsi/ufs/Makefile b/drivers/scsi/ufs/Makefile
>>
>>> index a3bd70c..6169096 100644
>>
>>> --- a/drivers/scsi/ufs/Makefile
>>
>>> +++ b/drivers/scsi/ufs/Makefile
>>
>>> @@ -7,6 +7,7 @@ obj-$(CONFIG_SCSI_UFS_QCOM) += ufs-qcom.o
>>
>>>  obj-$(CONFIG_SCSI_UFSHCD) += ufshcd-core.o
>>
>>>  ufshcd-core-y				+= ufshcd.o ufs-sysfs.o
>>
>>>  ufshcd-core-$(CONFIG_SCSI_UFS_BSG)	+= ufs_bsg.o
>>
>>> +ufshcd-core-$(CONFIG_SCSI_UFSHCD_RT_ENCRYPTION) += ufshcd-crypto.o
>>
>>>  obj-$(CONFIG_SCSI_UFSHCD_PCI) += ufshcd-pci.o
>>
>>>  obj-$(CONFIG_SCSI_UFSHCD_PLATFORM) += ufshcd-pltfrm.o
>>
>>>  obj-$(CONFIG_SCSI_UFS_HISI) += ufs-hisi.o
>>
>>> diff --git a/drivers/scsi/ufs/ufshcd-crypto.c b/drivers/scsi/ufs/ufshcd-crypto.c
>>
>>> new file mode 100644
>>
>>> index 0000000..9c95ff3
>>
>>> --- /dev/null
>>
>>> +++ b/drivers/scsi/ufs/ufshcd-crypto.c
>>
>>> @@ -0,0 +1,453 @@
>>
>>> +// SPDX-License-Identifier: GPL-2.0
>>
>>> +/*
>>
>>> + * UFS Host controller crypto driver
>>
>>> + *
>>
>>> + * Copyright (C) 2018 Cadence Design Systems, Inc.
>>
>>> + *
>>
>>> + * Authors:
>>
>>> + *	Parshuram Thombare <pthombar@xxxxxxxxxxx>
>>
>>> + *
>>
>>> + */
>>
>>> +
>>
>>> +#include <linux/module.h>
>>
>>> +#include <linux/kernel.h>
>>
>>> +#include <crypto/aes.h>
>>
>>> +#include <linux/device-mapper.h>
>>
>>> +#include "ufshcd.h"
>>
>>> +#include "ufshcd-crypto.h"
>>
>>> +#include "scsi/scsi_device.h"
>>
>>> +#include "scsi/scsi_host.h"
>>
>>> +
>>
>>> +struct ufshcd_dm_ctx {
>>
>>> +	struct dm_dev *dev;
>>
>>> +	sector_t start;
>>
>>> +	unsigned short int sector_size;
>>
>>> +	unsigned char sector_shift;
>>
>>> +	int cci;
>>
>>> +	int cap_idx;
>>
>>> +	char key[AES_MAX_KEY_SIZE];
>>
>>> +	struct ufs_hba *hba;
>>
>>> +};
>>
>>> +
>>
>>> +static int dm_registered;
>>
>>> +
>>
>>> +static inline int
>>
>>> +ufshcd_key_id_to_len(int key_id)
>>
>>> +{
>>
>>> +	int key_len = -1;
>>
>>> +
>>
>>> +	switch (key_id) {
>>
>>> +	case UFS_CRYPTO_KEY_ID_128BITS:
>>
>>> +		key_len = AES_KEYSIZE_128;
>>
>>> +		break;
>>
>>> +	case UFS_CRYPTO_KEY_ID_192BITS:
>>
>>> +		key_len = AES_KEYSIZE_192;
>>
>>> +		break;
>>
>>> +	case UFS_CRYPTO_KEY_ID_256BITS:
>>
>>> +		key_len = AES_KEYSIZE_256;
>>
>>> +		break;
>>
>>> +	default:
>>
>>> +		break;
>>
>>> +	}
>>
>>> +	return key_len;
>>
>>> +}

Why not -EINVAL for invalid key length?

>>
>>> +
>>
>>> +static inline int
>>
>>> +ufshcd_key_len_to_id(int key_len)
>>
>>> +{
>>
>>> +	int key_id = -1;
>>
>>> +
>>
>>> +	switch (key_len) {
>>
>>> +	case AES_KEYSIZE_128:
>>
>>> +		key_id = UFS_CRYPTO_KEY_ID_128BITS;
>>
>>> +		break;
>>
>>> +	case AES_KEYSIZE_192:
>>
>>> +		key_id = UFS_CRYPTO_KEY_ID_192BITS;
>>
>>> +		break;
>>
>>> +	case AES_KEYSIZE_256:
>>
>>> +		key_id = UFS_CRYPTO_KEY_ID_256BITS;
>>
>>> +		break;
>>
>>> +	default:
>>
>>> +		break;
>>
>>> +	}
>>
>>> +	return key_id;
>>
>>> +}
>>
>>> +
>>
>>> +static void
>>
>>> +ufshcd_read_crypto_capabilities(struct ufs_hba *hba)
>>
>>> +{
>>
>>> +	u32 tmp, i;
>>
>>> +	struct ufshcd_crypto_ctx *cctx = hba->cctx;
>>
>>> +
>>
>>> +	for (i = 0; i < cctx->cap_cnt; i++) {
>>
>>> +		tmp = ufshcd_readl(hba, REG_UFS_CRYPTOCAP + i);
>>
>>> +		cctx->ccaps[i].key_id = (tmp & CRYPTO_CAPS_KS_MASK) >>
>>
>>> +						CRYPTO_CAPS_KS_SHIFT;
>>
>>> +		cctx->ccaps[i].sdusb = (tmp & CRYPTO_CAPS_SDUSB_MASK) >>
>>
>>> +						CRYPTO_CAPS_SDUSB_SHIFT;
>>
>>> +		cctx->ccaps[i].alg_id = (tmp & CRYPTO_CAPS_ALG_ID_MASK) >>
>>
>>> +						CRYPTO_CAPS_ALG_ID_SHIFT;
>>
>>> +	}
>>
>>> +}
>>
>>> +
>>
>>> +static inline int
>>
>>> +ufshcd_get_cap_idx(struct ufshcd_crypto_ctx *cctx, int alg_id,
>>
>>> +	int key_id)
>>
>>> +{
>>
>>> +	int cap_idx;
>>
>>> +
>>
>>> +	for (cap_idx = 0; cap_idx < cctx->cap_cnt; cap_idx++) {
>>
>>> +		if (((cctx->ccaps[cap_idx].alg_id == alg_id) &&
>>
>>> +			cctx->ccaps[cap_idx].key_id == key_id))
>>
>>> +			break;
>>
>>> +	}
>>
>>> +	return ((cap_idx < cctx->cap_cnt) ? cap_idx : -1);
>>
>>> +}
>>
>>> +
>>
>>> +static inline int
>>
>>> +ufshcd_get_cci_slot(struct ufshcd_crypto_ctx *cctx)
>>
>>> +{
>>
>>> +	int  cci;
>>
>>> +
>>
>>> +	for (cci = 0; cci < cctx->config_cnt; cci++) {
>>
>>> +		if (!cctx->cconfigs[cci].cfge) {
>>
>>> +			cctx->cconfigs[cci].cfge = 1;
>>
>>> +			break;
>>
>>> +		}
>>
>>> +	}
>>
>>> +	return ((cci < cctx->config_cnt) ? cci : -1);
>>
>>> +}
>>
>>> +
>>
>>> +static void
>>
>>> +ufshcd_aes_ecb_set_key(struct ufshcd_dm_ctx *ctx)
>>
>>> +{
>>
>>> +	int i, key_size;
>>
>>> +	u32 val, cconfig16, crypto_config_addr;
>>
>>> +	struct ufshcd_crypto_ctx *cctx;
>>
>>> +	struct ufshcd_crypto_config *cconfig;
>>
>>> +	struct ufshcd_crypto_cap ccap;
>>
>>> +
>>
>>> +	cctx = ctx->hba->cctx;
>>
>>> +	if (ctx->cci <= 0)
>>
>>> +		ctx->cci = ufshcd_get_cci_slot(cctx);
>>
>>> +	/* If no slot is available, slot 0 is shared */
>>
>>> +	ctx->cci = ctx->cci < 0 ? 0 : ctx->cci;
>>
>>> +	cconfig = &(cctx->cconfigs[ctx->cci]);
>>
>>> +	ccap = cctx->ccaps[ctx->cap_idx];
>>
>>> +	key_size = ufshcd_key_id_to_len(ccap.key_id);
>>
>>> +
>>
>>> +	if ((cconfig->cap_idx != ctx->cap_idx) ||
>>
>>> +		((key_size > 0) &&
>>
>>> +		memcmp(cconfig->key, ctx->key, key_size))) {
>>
>>> +		cconfig->cap_idx = ctx->cap_idx;
>>
>>> +		memcpy(cconfig->key, ctx->key, key_size);
>>
>>> +		crypto_config_addr = cctx->crypto_config_base_addr +
>>
>>> +				ctx->cci * CRYPTO_CONFIG_SIZE;
>>
>>> +		cconfig16 = ccap.sdusb | (1 <<
>> CRYPTO_CCONFIG16_CFGE_SHIFT);
>>
>>> +		cconfig16 |= ((ctx->cap_idx <<
>> CRYPTO_CCONFIG16_CAP_IDX_SHIFT) &
>>
>>> +				CRYPTO_CCONFIG16_CAP_IDX_MASK);
>>
>>> +		spin_lock(&cctx->crypto_lock);
>>
>>> +		for (i = 0; i < key_size; i += 4) {
>>
>>> +			val = (ctx->key[i] | (ctx->key[i + 1] << 8) |
>>
>>> +				(ctx->key[i + 2] << 16) |
>>
>>> +				(ctx->key[i + 3] << 24));
>>
>>> +			ufshcd_writel(ctx->hba, val, crypto_config_addr + i);
>>
>>> +		}
>>
>>> +		ufshcd_writel(ctx->hba, cpu_to_le32(cconfig16),
>>
>>> +				crypto_config_addr + (4 * 16));
>>
>>> +		spin_unlock(&cctx->crypto_lock);
>>
>>> +		/* Make sure keys are programmed */
>>
>>> +		mb();
>>
>>> +	}
>>
>>> +}
>>
>>
>>
>> First of all, thanks for working on this.  A lot of Android device vendors would
>>
>> like to have upstream support for inline encryption.  However, are you aware of
>>
>> previous (unsuccessful) patchsets by other people working on this?  Have you
>>
>> addressed the concerns and improved on their work, or is this just yet another
>>
>> new effort starting from scratch?
>>
>>
>>
>> AnilKumar Chimata <anilc@xxxxxxxxxxxxxx> (Qualcomm) in October 2018:
>>
>>
>>
>> 	https://urldefense.proofpoint.com/v2/url?u=https-
>> 3A__patchwork.kernel.org_cover_10645739_&d=DwIBAg&c=aUq983L2pue2FqKF
>> oP6PGHMJQyoJ7kl3s3GZ-_haXqY&r=GTefrem3hiBCnsjCOqAuapQHRN8-
>> rKC1FRbk0it-
>> LDs&m=L9VLjsZ31dZ4TP4LdveuvcjPFzdZWGlZaZnzqGZH3zc&s=ZzYaAaVic5TB4RUS
>> cR5kzcM_8gvLYdlNAzuY80_ASzI&e=
>>
>>
>>
>> Ladvine D Almeida <ladvine@xxxxxxxxxxxx> in May 2018:
>>
>>
>>
>> 	https://urldefense.proofpoint.com/v2/url?u=http-3A__lists-
>> 2Darchives.com_linux-2Dkernel_29135393-2Dscsi-2Dufs-2Dufs-2Dhost-
>> 2Dcontroller-2Dcrypto-
>> 2Dchanges.html&d=DwIBAg&c=aUq983L2pue2FqKFoP6PGHMJQyoJ7kl3s3GZ-
>> _haXqY&r=GTefrem3hiBCnsjCOqAuapQHRN8-rKC1FRbk0it-
>> LDs&m=L9VLjsZ31dZ4TP4LdveuvcjPFzdZWGlZaZnzqGZH3zc&s=3pxSBZrt_DpDSz-
>> ZXrM7_bj0QXmRzcbasPl_wB259Us&e=
>>
>>
>>
>> Satya Tangirala <satyat@xxxxxxxxxx> is also working on it but I don't believe
>>
>> he's sent out a patchset yet.
>>
>>
>>
>> It would be nice to coordinate to get a proper solution upstream that works for
>>
>> everyone, rather than having everyone try independently and fail repeatedly :-)
> I had look at Ladvine's submission and think the approach of using Linux crypto API and
> adding algorithm which is supposed to work inline (and with UFS devices only) in global
> pool of algorithms (which is supposed to be generic) makes it risky, if selected/used for
> other type of device not supporting inline encryption. Also apparently it is not possible to
> support multiple UFS controllers in the system with that approach.
> There was suggestion from Milan to use separate device mapper which seems cleaner way
> of enabling inline encryption. Hence new device mapper is used.
> I think this is better idea to coordinate and come up with a generic solution.

Suggest to take a look into the article https://lwn.net/Articles/717754
My real concern is how to achieve it without any modifications to the
bio.(because key slot information has to finally reach the target block
device)

>>
>>
>>
>> Also, note that ECB mode is not appropriate for disk encryption.  So this patch
>>
>> (and the hardware you tested it on, if that's all it supports) is effectively
>>
>> useless as-is.  You need to support XTS mode.
> For now only AES-ECB is supported, we are working on adding other modes.
>>
>>
>>
>> Thanks!
>>
>>
>>
>> - Eric
> 
> Regards,
> Parshuram Thombare
> 

Thanks,
Ladvine