RE: [PATCH 0/4] Adds wrapped key support for inline storage encryption

[Gaurav Kashyap <gaurkash@qti.qualcomm.com>]

Hey Eric, here are the answers to some of the questions across all the patches

Also, at runtime, does any of the Qualcomm hardware support multiple key types, and if so can they be used at the same time?

- 	Currently, with hardware key manager data path, there is no support for standard keys. So, when HWKM is being used, only wrapped keys are supported.
	If standard keys need to be supported, it can be, but modifications are required within trustzone.

> However, when keys are hardware wrapped, it can be only unwrapped by 
> Qualcomm Trustzone.
Qualcomm Trustzone is software.  There is a mode where it passes the key to the actual HWKM hardware as intended, right?

-	That's right, trustzone does not perform any unwrap, it is done by the hardware. It was a wrong explanation from my end.

Please make the semantics of the @secret_size parameter clear.  Will the output be at least @secret_size, exactly @secret_size, or at most @secret_size?
- 	Updated this in the latest patches.

Warm Regards,
Gaurav Kashyap

-----Original Message-----
From: Eric Biggers <ebiggers@kernel.org> 
Sent: Thursday, November 4, 2021 3:49 PM
To: Gaurav Kashyap (QUIC) <quic_gaurkash@quicinc.com>
Cc: linux-scsi@vger.kernel.org; linux-arm-msm@vger.kernel.org; linux-mmc@vger.kernel.org; linux-block@vger.kernel.org; linux-fscrypt@vger.kernel.org; thara.gopinath@linaro.org; asutoshd@codeaurora.org
Subject: Re: [PATCH 0/4] Adds wrapped key support for inline storage encryption

WARNING: This email originated from outside of Qualcomm. Please be wary of any links or attachments, and do not enable macros.

Hi Gaurav,

On Wed, Nov 03, 2021 at 04:18:36PM -0700, Gaurav Kashyap wrote:
> This currently has 4 patches with another coming in shortly for MMC.
>
> 1. Moves ICE functionality to a common library, so that different storage controllers can use it.
> 2. Adds a SCM call for derive raw secret needed for wrapped keys.
> 3. Adds a hardware key manager library needed for wrapped keys.
> 4. Adds wrapped key support in ufs for storage encryption
>
> Gaurav Kashyap (4):
>   ufs: move ICE functionality to a common library
>   qcom_scm: scm call for deriving a software secret
>   soc: qcom: add HWKM library for storage encryption
>   soc: qcom: add wrapped key support for ICE
>
>  drivers/firmware/qcom_scm.c       |  61 +++++++
>  drivers/firmware/qcom_scm.h       |   1 +
>  drivers/scsi/ufs/ufs-qcom-ice.c   | 200 ++++++-----------------
>  drivers/scsi/ufs/ufs-qcom.c       |   1 +
>  drivers/scsi/ufs/ufs-qcom.h       |   5 +
>  drivers/scsi/ufs/ufshcd-crypto.c  |  47 ++++--
>  drivers/scsi/ufs/ufshcd.h         |   5 +
>  drivers/soc/qcom/Kconfig          |  14 ++
>  drivers/soc/qcom/Makefile         |   2 +
>  drivers/soc/qcom/qti-ice-common.c | 215 +++++++++++++++++++++++++
>  drivers/soc/qcom/qti-ice-hwkm.c   |  77 +++++++++
>  drivers/soc/qcom/qti-ice-regs.h   | 257 ++++++++++++++++++++++++++++++
>  include/linux/qcom_scm.h          |   5 +
>  include/linux/qti-ice-common.h    |  37 +++++
>  14 files changed, 766 insertions(+), 161 deletions(-)  create mode 
> 100644 drivers/soc/qcom/qti-ice-common.c  create mode 100644 
> drivers/soc/qcom/qti-ice-hwkm.c  create mode 100644 
> drivers/soc/qcom/qti-ice-regs.h  create mode 100644 
> include/linux/qti-ice-common.h

Thanks for the patches!  These are on top of my patchset "[RFC PATCH v2 0/5] Support for hardware-wrapped inline encryption keys"
(https://lore.kernel.org/linux-block/20210916174928.65529-1-ebiggers@kernel.org),
right?  You should mention that in your cover letter, so that it's possible for people to apply your patches for reviewing or testing, and also to provide context about what this feature is and why it is important.

As part of that, it would be helpful to specifically mention the documentation for hardware-wrapped keys in Documentation/block/inline-encryption.rst that I included in my patchset.  It provides a lot of background information that your patches are hard to understand without (at least your patches 2-4; your first patch isn't dependent on the hardware-wrapped keys feature).

Can you include information about how your patches were tested?  That's really important to include.

Please run './scripts/checkpatch.pl' on your patches, as recommended in Documentation/process/submitting-patches.rst.  It can catch a lot of issues.

Please use the imperative tense, like "add wrapped key support" rather than "adds wrapped key support".

I'll leave some more comments on the individual patches.

- Eric