Hi Ladvine, >From: Ladvine D Almeida <ladvine.dalmeida@xxxxxxxxxxxx> >Sent: Friday, December 14, 2018 1:10 AM >Subject: Re: [PATCH 2/2] scsi: ufs: add inline crypto support to UFS HCD >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. [PATCH 2/2] have crypto-ufs implementation. [PATCH 1/2] add variable to struct bio. Device mapper name is not generic because there are ufs specific things but we should be able to make it generic. Regards, Parshuram Thombare >-----Original Message----- >From: Ladvine D Almeida <ladvine.dalmeida@xxxxxxxxxxxx> >Sent: Friday, December 14, 2018 1:10 AM >To: Parshuram Raju Thombare <pthombar@xxxxxxxxxxx>; Eric Biggers ><ebiggers@xxxxxxxxxx> >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.dalmeida@xxxxxxxxxxxx>; Satya Tangirala <satyat@xxxxxxxxxx>; Paul >Crowley <paulcrowley@xxxxxxxxxx>; Manjunath M Bettegowda ><manjunath.mb@xxxxxxxxxxxx>; Tejas Joglekar ><tejas.joglekar@xxxxxxxxxxxx>; Joao Pinto <Joao.Pinto@xxxxxxxxxxxx>; linux- >crypto@xxxxxxxxxxxxxxx >Subject: Re: [PATCH 2/2] scsi: ufs: add inline crypto support to UFS HCD > >EXTERNAL MAIL > > >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://urldefense.proofpoint.com/v2/url?u=https- >3A__lwn.net_Articles_717754&d=DwIFAg&c=aUq983L2pue2FqKFoP6PGHMJQyoJ >7kl3s3GZ-_haXqY&r=GTefrem3hiBCnsjCOqAuapQHRN8-rKC1FRbk0it- >LDs&m=rOQpiy7- >jae72qATLWxXtCnoEXCjZLklWrHo5NtcUqo&s=xVHqE7JfiKf0rCQozZrYr6eeFn5D6U >OCvXJMP-mjYX8&e= >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