| Message ID | 20211206225725.77512-1-quic_gaurkash@quicinc.com |
|---|---|
| Headers | show |
| Series | Add wrapped key support for Qualcomm ICE | expand |
On Mon, Dec 06, 2021 at 02:57:19PM -0800, Gaurav Kashyap wrote: > Wrapped keys should utilize hardware to protect the keys > used for storage encryption. Qualcomm's Inline Crypto Engine "should utilize" => "utilize"? > supports a hardware block called Hardware Key Manager (HWKM) > for key management. > > Although most of the interactions to this hardware block happens > via a secure execution environment, some initializations for the > slave present in ICE can be done from the kernel. > > This can also be a placeholder for when the hardware provides more > capabilities to be accessed from the linux kernel in the future. > This commit message doesn't explain what this commit actually does. Can you make that clearer? > diff --git a/drivers/soc/qcom/qti-ice-hwkm.c b/drivers/soc/qcom/qti-ice-hwkm.c > new file mode 100644 > index 000000000000..3be6b350cd88 > --- /dev/null > +++ b/drivers/soc/qcom/qti-ice-hwkm.c > @@ -0,0 +1,111 @@ > +// SPDX-License-Identifier: GPL-2.0-only > +/* > + * HWKM ICE library for storage encryption. > + * > + * Copyright (c) 2021, Qualcomm Innovation Center. All rights reserved. > + */ > + > +#include <linux/qti-ice-common.h> > +#include "qti-ice-regs.h" > + > +static int qti_ice_hwkm_bist_status(const struct ice_mmio_data *mmio, int version) > +{ > + if (!qti_ice_hwkm_testb(mmio->ice_hwkm_mmio, QTI_HWKM_ICE_RG_TZ_KM_STATUS, > + (version == 1) ? BIST_DONE_V1 : BIST_DONE_V2) || > + !qti_ice_hwkm_testb(mmio->ice_hwkm_mmio, QTI_HWKM_ICE_RG_TZ_KM_STATUS, > + (version == 1) ? CRYPTO_LIB_BIST_DONE_V1 : > + CRYPTO_LIB_BIST_DONE_V2) || > + !qti_ice_hwkm_testb(mmio->ice_hwkm_mmio, QTI_HWKM_ICE_RG_TZ_KM_STATUS, > + (version == 1) ? BOOT_CMD_LIST1_DONE_V1 : > + BOOT_CMD_LIST1_DONE_V2) || > + !qti_ice_hwkm_testb(mmio->ice_hwkm_mmio, QTI_HWKM_ICE_RG_TZ_KM_STATUS, > + (version == 1) ? BOOT_CMD_LIST0_DONE_V1 : > + BOOT_CMD_LIST0_DONE_V2) || > + !qti_ice_hwkm_testb(mmio->ice_hwkm_mmio, QTI_HWKM_ICE_RG_TZ_KM_STATUS, > + (version == 1) ? KT_CLEAR_DONE_V1 : > + KT_CLEAR_DONE_V2)) > + return -EINVAL; > + return 0; > +} Long "if" statements are hard to read. It would be more readable to have a separate "if" and "return -EINVAL" for each of these checks. > +static void qti_ice_hwkm_configure_ice_registers( > + const struct ice_mmio_data *mmio) > +{ > + qti_ice_hwkm_writel(mmio->ice_hwkm_mmio, 0xffffffff, > + QTI_HWKM_ICE_RG_BANK0_AC_BANKN_BBAC_0); > + qti_ice_hwkm_writel(mmio->ice_hwkm_mmio, 0xffffffff, > + QTI_HWKM_ICE_RG_BANK0_AC_BANKN_BBAC_1); > + qti_ice_hwkm_writel(mmio->ice_hwkm_mmio, 0xffffffff, > + QTI_HWKM_ICE_RG_BANK0_AC_BANKN_BBAC_2); > + qti_ice_hwkm_writel(mmio->ice_hwkm_mmio, 0xffffffff, > + QTI_HWKM_ICE_RG_BANK0_AC_BANKN_BBAC_3); > + qti_ice_hwkm_writel(mmio->ice_hwkm_mmio, 0xffffffff, > + QTI_HWKM_ICE_RG_BANK0_AC_BANKN_BBAC_4); > +} > + > +static int qti_ice_hwkm_init_sequence(const struct ice_mmio_data *mmio, > + int version) > +{ > + u32 val = 0; > + > + /* > + * Put ICE in standard mode, ICE defaults to legacy mode. > + * Legacy mode - ICE HWKM slave not supported. > + * Standard mode - ICE HWKM slave supported. > + * > + * Depending on the version of HWKM, it is controlled by different > + * registers in ICE. > + */ > + if (version >= 2) { > + val = qti_ice_readl(mmio->ice_mmio, QTI_ICE_REGS_CONTROL); > + val = val & 0xFFFFFFFE; > + qti_ice_writel(mmio->ice_mmio, val, QTI_ICE_REGS_CONTROL); > + } else { > + qti_ice_hwkm_writel(mmio->ice_hwkm_mmio, 0x7, > + QTI_HWKM_ICE_RG_TZ_KM_CTL); > + } So to use wrapped keys, ICE needs to be in "standard mode", and to use standard keys it needs to be in "legacy mode"? That's confusing. It would be helpful to explain this more clearly in a comment. > + > + /* Check BIST status */ > + if (qti_ice_hwkm_bist_status(mmio, version)) > + return -EINVAL; Please spell out what BIST stands for. It's "Built-In Self Test", right? > + > + /* > + * Give register bank of the HWKM slave access to read and modify > + * the keyslots in ICE HWKM slave. Without this, trustzone will not > + * be able to program keys into ICE. > + */ > + qti_ice_hwkm_configure_ice_registers(mmio); > + > + /* Disable CRC check */ > + qti_ice_hwkm_clearb(mmio->ice_hwkm_mmio, QTI_HWKM_ICE_RG_TZ_KM_CTL, > + CRC_CHECK_EN); > + > + /* Set RSP_FIFO_FULL bit */ > + qti_ice_hwkm_setb(mmio->ice_hwkm_mmio, > + QTI_HWKM_ICE_RG_BANK0_BANKN_IRQ_STATUS, RSP_FIFO_FULL); Please expand on comments as much as possible, so that people can understand not just what the code is doing but why it is doing it. E.g., why does the CRC check need to be disabled, and why does the RSP_FIFO_FULL bit need to be set? > +/** > + * qti_ice_hwkm_init() - Initialize ICE HWKM hardware > + * @ice_mmio_data: contains ICE register mapping for i/o > + * @version: version of hwkm hardware > + * > + * Perform HWKM initialization in the ICE slave by going > + * through the slave initialization routine.The registers > + * can vary slightly based on the version. > + * > + * Return: 0 on success; err on failure. > + */ > + > +int qti_ice_hwkm_init(const struct ice_mmio_data *mmio, int version) > +{ > + if (!mmio->ice_hwkm_mmio) > + return -EINVAL; > + > + return qti_ice_hwkm_init_sequence(mmio, version); > +} > +EXPORT_SYMBOL_GPL(qti_ice_hwkm_init); This function is only called from within the same kernel module, so it doesn't need to be an exported symbol. > +MODULE_LICENSE("GPL v2"); The main source file for this module (qti-ice-common.c) already has a MODULE_LICENSE, so there shouldn't be a duplicate one here. MODULE_LICENSE is for the whole module, not for individual source files. > + > +#define qti_ice_hwkm_readl(hwkm_mmio, reg) \ > + (readl_relaxed(hwkm_mmio + (reg))) > +#define qti_ice_hwkm_writel(hwkm_mmio, val, reg) \ > + (writel_relaxed((val), hwkm_mmio + (reg))) Why readl_relaxed() and writel_relaxed(), instead of readl() and writel()? > +static inline bool qti_ice_hwkm_testb(void __iomem *ice_hwkm_mmio, > + u32 reg, u8 nr) > +{ > + u32 val = qti_ice_hwkm_readl(ice_hwkm_mmio, reg); > + > + val = (val >> nr) & 0x1; > + if (val == 0) > + return false; > + return true; > +} This is unnecessarily verbose. It could be just: return qti_ice_hwkm_readl(ice_hwkm_mmio, reg) & (1U << nr); - Eric
On Mon, Dec 06, 2021 at 02:57:21PM -0800, Gaurav Kashyap wrote: > Add support for wrapped keys in ufs and common ICE library. > Qualcomm's ICE solution uses a hardware block called Hardware > Key Manager (HWKM) to handle wrapped keys. > > This patch adds the following changes to support this. > 1. Link to HWKM library for initialization. > 2. Most of the key management is done from Trustzone via scm calls. > Added calls to this from the ICE library. > 3. Added support for this framework in ufs qcom. > 4. Added support for deriving SW secret as it cannot be done in > linux kernel for wrapped keys. > > Signed-off-by: Gaurav Kashyap <quic_gaurkash@quicinc.com> > --- > drivers/scsi/ufs/ufs-qcom-ice.c | 48 +++++++-- > drivers/scsi/ufs/ufs-qcom.c | 1 + > drivers/scsi/ufs/ufs-qcom.h | 7 +- > drivers/soc/qcom/qti-ice-common.c | 158 +++++++++++++++++++++++++++--- > include/linux/qti-ice-common.h | 11 ++- > 5 files changed, 198 insertions(+), 27 deletions(-) If possible, the qti-ice-common changes should be a separate patch that goes before the ufs-qcom changes. Are there interdependencies that make this impossible? I see the prototype change in qti_ice_keyslot_evict() (see below), but that could be avoided by using the right prototype from the beginning. > diff --git a/drivers/scsi/ufs/ufs-qcom-ice.c b/drivers/scsi/ufs/ufs-qcom-ice.c > index 3826643bf537..c8305aab6714 100644 > --- a/drivers/scsi/ufs/ufs-qcom-ice.c > +++ b/drivers/scsi/ufs/ufs-qcom-ice.c > @@ -43,6 +43,24 @@ int ufs_qcom_ice_init(struct ufs_qcom_host *host) > return err; > } > > + res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "ice_hwkm"); > + if (!res) { > + dev_warn(dev, "ICE HWKM registers not found\n"); > + host->ice_data.hw_wrapped_keys_supported = false; > + goto init; > + } I don't think a warning is appropriate here, as this is expected behavior on SoCs that support standard keys rather than wrapped keys. > + host->ice_data.ice_hwkm_mmio = devm_ioremap_resource(dev, res); > + if (IS_ERR(host->ice_data.ice_hwkm_mmio)) { > + err = PTR_ERR(host->ice_data.ice_hwkm_mmio); > + dev_err(dev, "Failed to map HWKM registers; err=%d\n", err); > + return err; > + } > + host->ice_data.hw_wrapped_keys_supported = true; Similar to what I suggested on the previous patch: "hw_wrapped_keys_supported" should be called something like "using_hw_wrapped_keys", since it means that standard keys are *not* supported, not just that wrapped keys are supported. > int ufs_qcom_ice_program_key(struct ufs_hba *hba, > - const union ufs_crypto_cfg_entry *cfg, int slot) > + const struct blk_crypto_key *key, int slot, > + u8 data_unit_size, int capid, bool evict) > { > union ufs_crypto_cap_entry cap; > struct ufs_qcom_host *host = ufshcd_get_variant(hba); > > - if (!(cfg->config_enable & UFS_CRYPTO_CONFIGURATION_ENABLE)) > - return qti_ice_keyslot_evict(slot); > + if (evict) > + return qti_ice_keyslot_evict(&host->ice_data, slot); It probably would be easier if qti_ice_keyslot_evict() took the ice_data parameter from the beginning. Then its prototype wouldn't need to change halfway through the patch series. > static bool qti_ice_supported(const struct ice_mmio_data *mmio) > { > u32 regval = qti_ice_readl(mmio->ice_mmio, QTI_ICE_REGS_VERSION); > @@ -28,6 +45,11 @@ static bool qti_ice_supported(const struct ice_mmio_data *mmio) > return false; > } > > + if ((major >= 4) || ((major == 3) && (minor == 2) && (step >= 1))) > + hwkm_version = 2; > + else > + hwkm_version = 1; Is this trying to check whether the ICE version is greater than or equal to 3.2.1? If so, this isn't quite correct, as it doesn't handle 3.X correctly where X is greater than 2. > + /* > + * HWKM slave in ICE should be initialized before the first > + * time we perform ICE HWKM related operations. This is because > + * ICE by default comes up in legacy mode where HWKM operations > + * won't work. > + */ > + if (!qti_hwkm_init_done) { > + err = qti_ice_hwkm_init(mmio, hwkm_version); > + if (err) { > + pr_err("%s: Error initializing hwkm, err = %d", > + __func__, err); > + return -EINVAL; > + } > + qti_hwkm_init_done = true; > + } This code is duplicated in two places. Can it be consolidated into a helper function? Also, "should be initialized" => "must be initialized"? > + > + memset(&cfg, 0, sizeof(cfg)); > + cfg.dusize = data_unit_mask; > + cfg.capidx = capid; > + cfg.cfge = 0x80; > + > + /* Make sure CFGE is cleared */ > + qti_ice_writel(mmio->ice_mmio, 0x0, (QTI_ICE_LUT_KEYS_CRYPTOCFG_R_16 + > + QTI_ICE_LUT_KEYS_CRYPTOCFG_OFFSET*slot)); > + /* Memory barrier - to ensure write completion before next transaction */ > + wmb(); Is this memory barrier necessary only because writel_relaxed() is being used? Using writel() would probably be a better idea, as I've mentioned elsewhere. Also, what does a "transaction" mean in this context? > + > + /* Call trustzone to program the wrapped key using hwkm */ > + err = qcom_scm_ice_set_key(slot, crypto_key->raw, crypto_key->size, > + capid, data_unit_mask); There's a weird double space here. > + if (err) > + pr_err("%s:SCM call Error: 0x%x slot %d\n", > + __func__, err, slot); > + > + /* Make sure CFGE is enabled after programming the key */ > + qti_ice_writel(mmio->ice_mmio, cfg.regval, > + (QTI_ICE_LUT_KEYS_CRYPTOCFG_R_16 + > + QTI_ICE_LUT_KEYS_CRYPTOCFG_OFFSET*slot)); Shouln't CFGE not be set on failure? > +int qti_ice_keyslot_evict(const struct ice_mmio_data *mmio, unsigned int slot) > { > + /* > + * Ignore calls to evict key when wrapped keys are supported and > + * hwkm init is not yet done. This is to avoid the clearing all slots > + * call that comes from ufs during ufs reset. HWKM slave in ICE takes > + * care of zeroing out the keytable on reset. > + */ > + if (mmio->hw_wrapped_keys_supported && !qti_hwkm_init_done) > + return 0; I guess this is reasonable. (The alternative would be to not clear the keyslots in the first place.) But this is going to be used by MMC too, so the comment should be worded in a generic way and not be tied to UFS. > +/** > + * qti_ice_derive_sw_secret() - Derive SW secret from wrapped key > + * @wrapped_key: wrapped key from which secret should be derived > + * @wrapped_key_size: size of the wrapped key > + * @sw_secret: secret to be returned, which is atmost BLK_CRYPTO_SW_SECRET_SIZE The resulting sw_secret needs to be *exactly* BLK_CRYPTO_SW_SECRET_SIZE bytes, not "at most" that many bytes. - Eric
On Mon, Dec 06, 2021 at 02:57:23PM -0800, Gaurav Kashyap wrote: > This patch contains two changes in UFS for wrapped keys. > 1. Implements the blk_crypto_profile ops for generate, import > and prepare key apis. > 2. Adds UFS vops for generate, import and prepare keys so > that vendors can hooks to them. > > Signed-off-by: Gaurav Kashyap <quic_gaurkash@quicinc.com> When adding things to ufs_hba_variant_ops, it would helpful to explain why they belong there. It's because this stuff isn't part of the UFS standard, right? - Eric
Hi Gaurav, On Thu, Jan 06, 2022 at 09:14:22PM +0000, Gaurav Kashyap wrote: > Hey Eric > > > Have you tested that QCOM_SCM_ES_DERIVE_SW_SECRET is working properly? > > - You will need updated trustzone build for that (as I was missing a minor detail in the original one pertaining to SW secret) , please request again on the same ticket for the updated build. > - I have reminded the people in Qualcomm too to provide you the build. > - Note that with the new build you should be using the correct directions, i.e QCOM_SCM_RO where intended > > Warm Regards > Gaurav Kashyap > I verified that the latest TrustZone build is working; thanks for the help! Note, these are the branches I'm using for now: * Kernel patches: https://git.kernel.org/pub/scm/fs/fscrypt/fscrypt.git/log/?h=wip-wrapped-keys * fscryptctl tool and test scripts: https://github.com/ebiggers/fscryptctl/tree/wip-wrapped-keys The kernel branch is based on v5.17-rc1. I haven't changed your patches from your latest series other than rebasing them and adding a fix "qcom_scm: fix return values" on top. Note that v5.16-rc5 and later have a bug where the UFS controller on SM8350 isn't recognized. Therefore, my branch contains a fix from Bjorn Andersson for that bug, which I applied from the mailing list. One oddity I noticed is that if I import the same raw key twice, the long-term wrapped key blob is the same. This implies that the key encryption algorithm used by the Qualcomm hardware is deterministic, which is unexpected. I would expect the wrapped key to contain a random nonce. Do you know why deterministic encryption is used? This probably isn't much of a problem, but it's unexpected. Besides that, I think the next steps are for you to address the comments I've left on this series, and for me to get started on adding ciphertext verification tests for this to xfstests (alongside the other fscrypt ciphertext verification tests that are already there) so that we can prove this feature is actually encrypting the data as intended. - Eric