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

[Parshuram Raju Thombare]

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]
>
>
>
>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'.
>
>>
>
>> 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;
>
>> +}
>
>> +
>
>> +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.
>
>
>
>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