| Message ID | 20211103231840.115521-1-quic_gaurkash@quicinc.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | Adds wrapped key support for inline storage encryption | expand |
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
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
On Wed, Dec 08, 2021 at 12:09:03AM +0000, Gaurav Kashyap wrote: > 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. Do the SoCs support both key types though, just not at the same time? E.g. when the ufs_qcom driver loads on SM8350, could it choose to expose either standard key support or wrapped key support, or is it predetermined by the hardware and/or firmware? If the driver has a choice, then there should be a kernel module parameter (module_param()) that controls it, so that the user can choose which key type they want when they boot their kernel. - Eric
Warm Regards, Gaurav Kashyap -----Original Message----- From: Eric Biggers <ebiggers@kernel.org> Sent: Tuesday, December 7, 2021 4:23 PM To: Gaurav Kashyap <gaurkash@qti.qualcomm.com> Cc: Gaurav Kashyap (QUIC) <quic_gaurkash@quicinc.com>; 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. On Wed, Dec 08, 2021 at 12:09:03AM +0000, Gaurav Kashyap wrote: > 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. > Do the SoCs support both key types though, just not at the same time? E.g. when the ufs_qcom driver loads on SM8350, could it choose to expose either standard key support or wrapped key support, or is it predetermined by the hardware and/or firmware? If the driver has a choice, > then there should be a kernel module parameter (module_param()) that controls it, so that the user can choose which key type they want when they boot their kernel. As of now, it is predetermined in TZ firmware. As in, if TZ has booted up with HWKM support, only wrapped keys are supported. But it is not impossible for HWKM to support standard keys as well, it is just that currently there is no path in TZ for standard keys when HWKM is being used. - Eric