On 12/12/18 5:52 AM, Parshuram Raju Thombare wrote:
> Hello Eric,
>
> Thank you for a comment.
>
>> -----Original Message-----
>> From: Eric Biggers <[email protected]>
>> Sent: Tuesday, December 11, 2018 11:47 PM
>> To: Parshuram Raju Thombare <[email protected]>
>> Cc: [email protected]; [email protected]; [email protected];
>> [email protected]; [email protected];
>> [email protected]; [email protected]; akpm@linux-
>> foundation.org; [email protected]; [email protected]; linux-
>> [email protected]; [email protected]; linux-
>> [email protected]; Alan Douglas <[email protected]>; Janek Kotas
>> <[email protected]>; Rafal Ciepiela <[email protected]>; AnilKumar
>> Chimata <[email protected]>; Ladvine D Almeida <[email protected]>;
>> Satya Tangirala <[email protected]>; Paul Crowley
>> <[email protected]>
>> 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 <[email protected]>
>>
>>> ---
>>
>>> 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 <[email protected]>
>>
>>> +L: [email protected]
>>
>>> +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 <[email protected]>
>>
>>> L: [email protected]
>>
>>> 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 <[email protected]>
>>
>>> + *
>>
>>> + */
>>
>>> +
>>
>>> +#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 <[email protected]> (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 <[email protected]> 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 <[email protected]> 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
Hi Ladvine,
>From: Ladvine D Almeida <[email protected]>
>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 <[email protected]>
>Sent: Friday, December 14, 2018 1:10 AM
>To: Parshuram Raju Thombare <[email protected]>; Eric Biggers
><[email protected]>
>Cc: [email protected]; [email protected]; [email protected];
>[email protected]; [email protected];
>[email protected]; [email protected]; akpm@linux-
>foundation.org; [email protected]; [email protected]; linux-
>[email protected]; [email protected]; linux-
>[email protected]; Alan Douglas <[email protected]>; Janek Kotas
><[email protected]>; Rafal Ciepiela <[email protected]>; AnilKumar
>Chimata <[email protected]>; Ladvine D Almeida
><[email protected]>; Satya Tangirala <[email protected]>; Paul
>Crowley <[email protected]>; Manjunath M Bettegowda
><[email protected]>; Tejas Joglekar
><[email protected]>; Joao Pinto <[email protected]>; linux-
>[email protected]
>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 <[email protected]>
>>> Sent: Tuesday, December 11, 2018 11:47 PM
>>> To: Parshuram Raju Thombare <[email protected]>
>>> Cc: [email protected]; [email protected]; [email protected];
>>> [email protected]; [email protected];
>>> [email protected]; [email protected]; akpm@linux-
>>> foundation.org; [email protected]; [email protected]; linux-
>>> [email protected]; [email protected]; linux-
>>> [email protected]; Alan Douglas <[email protected]>; Janek
>>> Kotas <[email protected]>; Rafal Ciepiela <[email protected]>;
>>> AnilKumar Chimata <[email protected]>; Ladvine D Almeida
>>> <[email protected]>; Satya Tangirala <[email protected]>; Paul
>>> Crowley <[email protected]>
>>> 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 <[email protected]>
>>>
>>>> ---
>>>
>>>> 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 <[email protected]>
>>>
>>>> +L: [email protected]
>>>
>>>> +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 <[email protected]>
>>>
>>>> L: [email protected]
>>>
>>>> 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 <[email protected]>
>>>
>>>> + *
>>>
>>>> + */
>>>
>>>> +
>>>
>>>> +#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 <[email protected]> (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 <[email protected]> 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 <[email protected]> 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
On 12/13/18 12:39 PM, Ladvine D Almeida wrote:
> 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)
Guys, you both need to edit when you reply, wading through 650 lines of
text to get to this...
You obviously can't modify the bio if you don't own it, but you could
clone it and then you have storage in ->bi_private.
--
Jens Axboe