| Message ID | 20240617005825.1443206-1-quic_gaurkash@quicinc.com |
|---|---|
| Headers | show |
| Series | Hardware wrapped key support for qcom ice and ufs | expand |
Hi, On 17/06/2024 02:51, Gaurav Kashyap wrote: > The Inline Crypto Engine (ICE) for UFS/EMMC supports the > Hardware Key Manager (HWKM) to securely manage storage > keys. Enable using this hardware on sm8650. > > This requires two changes: > 1. Register size increase: HWKM is an additional piece of hardware > sitting alongside ICE, and extends the old ICE's register space. > 2. Explicitly tell the ICE driver to use HWKM with ICE so that > wrapped keys are used in sm8650. > > Reviewed-by: Om Prakash Singh <quic_omprsing@quicinc.com> > Tested-by: Neil Armstrong <neil.armstrong@linaro.org> > Signed-off-by: Gaurav Kashyap <quic_gaurkash@quicinc.com> > --- > arch/arm64/boot/dts/qcom/sm8650.dtsi | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > diff --git a/arch/arm64/boot/dts/qcom/sm8650.dtsi b/arch/arm64/boot/dts/qcom/sm8650.dtsi > index bb0b3c48ee4b..a34c4b7ccbac 100644 > --- a/arch/arm64/boot/dts/qcom/sm8650.dtsi > +++ b/arch/arm64/boot/dts/qcom/sm8650.dtsi > @@ -2593,9 +2593,11 @@ &mc_virt SLAVE_EBI1 QCOM_ICC_TAG_ALWAYS>, > ice: crypto@1d88000 { > compatible = "qcom,sm8650-inline-crypto-engine", > "qcom,inline-crypto-engine"; > - reg = <0 0x01d88000 0 0x8000>; > + reg = <0 0x01d88000 0 0x10000>; > > clocks = <&gcc GCC_UFS_PHY_ICE_CORE_CLK>; > + > + qcom,ice-use-hwkm; > }; > > tcsr_mutex: hwlock@1f40000 { Please split this (and next) in two patches: - one extending the register size + Fixes tag so it can backported to stable kernels - one adding qcom,ice-use-hwkm (if bindings maintainers agrees with this property) And please send sm8550 before sm8650... Thanks, Neil
On 6/17/24 02:51, Gaurav Kashyap wrote: > Block crypto allows storage controllers like UFS to > register an op derive a software secret from wrapped > keys added to the kernel. > > Wrapped keys in most cases will have vendor specific > implementations, which means this op would need to have > a corresponding UFS variant op. > This change adds hooks in UFS core to support this variant > ops and tie them to the blk crypto op. > > Tested-by: Neil Armstrong <neil.armstrong@linaro.org> > Reviewed-by: Om Prakash Singh <quic_omprsing@quicinc.com> > Signed-off-by: Gaurav Kashyap <quic_gaurkash@quicinc.com> > --- > drivers/ufs/core/ufshcd-crypto.c | 15 +++++++++++++++ > include/ufs/ufshcd.h | 4 ++++ > 2 files changed, 19 insertions(+) > > diff --git a/drivers/ufs/core/ufshcd-crypto.c b/drivers/ufs/core/ufshcd-crypto.c > index 399b55d67b3b..c14800eac1ff 100644 > --- a/drivers/ufs/core/ufshcd-crypto.c > +++ b/drivers/ufs/core/ufshcd-crypto.c > @@ -119,6 +119,20 @@ static int ufshcd_crypto_keyslot_evict(struct blk_crypto_profile *profile, > return ufshcd_clear_keyslot(hba, slot); > } > > +static int ufshcd_crypto_derive_sw_secret(struct blk_crypto_profile *profile, > + const u8 wkey[], size_t wkey_size, > + u8 sw_secret[BLK_CRYPTO_SW_SECRET_SIZE]) > +{ > + struct ufs_hba *hba = > + container_of(profile, struct ufs_hba, crypto_profile); > + > + if (hba->vops && hba->vops->derive_sw_secret) > + return hba->vops->derive_sw_secret(hba, wkey, wkey_size, Double space Konrad
On 17/06/2024 02:50, Gaurav Kashyap wrote: > Qualcomm's ICE (Inline Crypto Engine) contains a proprietary > key management hardware called Hardware Key Manager (HWKM). > This patch integrates HWKM support in ICE when it is > available. HWKM primarily provides hardware wrapped key support > where the ICE (storage) keys are not available in software and > protected in hardware. > > When HWKM software support is not fully available (from Trustzone), > there can be a scenario where the ICE hardware supports HWKM, but > it cannot be used for wrapped keys. In this case, standard keys have > to be used without using HWKM. Hence, providing a toggle controlled > by a devicetree entry to use HWKM or not. > > Tested-by: Neil Armstrong <neil.armstrong@linaro.org> > Signed-off-by: Gaurav Kashyap <quic_gaurkash@quicinc.com> > --- > drivers/soc/qcom/ice.c | 153 +++++++++++++++++++++++++++++++++++++++-- > include/soc/qcom/ice.h | 1 + > 2 files changed, 150 insertions(+), 4 deletions(-) > > diff --git a/drivers/soc/qcom/ice.c b/drivers/soc/qcom/ice.c > index 6f941d32fffb..d5e74cf2946b 100644 > --- a/drivers/soc/qcom/ice.c > +++ b/drivers/soc/qcom/ice.c > @@ -26,6 +26,40 @@ <snip> > + > static struct qcom_ice *qcom_ice_create(struct device *dev, > void __iomem *base) > { > @@ -239,6 +382,8 @@ static struct qcom_ice *qcom_ice_create(struct device *dev, > engine->core_clk = devm_clk_get_enabled(dev, NULL); > if (IS_ERR(engine->core_clk)) > return ERR_CAST(engine->core_clk); > + engine->use_hwkm = of_property_read_bool(dev->of_node, > + "qcom,ice-use-hwkm"); Please drop this property and instead add an scm function calling: __qcom_scm_is_call_available(QCOM_SCM_SVC_ES, QCOM_SCM_ES_DERIVE_SW_SECRET) like bool qcom_scm_derive_sw_secret_available(void) { if (!__qcom_scm_is_call_available(__scm->dev, QCOM_SCM_SVC_ES, QCOM_SCM_ES_DERIVE_SW_SECRET)) return false; return true; } You may perhaps only call qcom_scm_derive_sw_secret_available() for some ICE versions. Neil > > if (!qcom_ice_check_supported(engine)) > return ERR_PTR(-EOPNOTSUPP); > diff --git a/include/soc/qcom/ice.h b/include/soc/qcom/ice.h > index 9dd835dba2a7..1f52e82e3e1c 100644 > --- a/include/soc/qcom/ice.h > +++ b/include/soc/qcom/ice.h > @@ -34,5 +34,6 @@ int qcom_ice_program_key(struct qcom_ice *ice, > const struct blk_crypto_key *bkey, > u8 data_unit_size, int slot); > int qcom_ice_evict_key(struct qcom_ice *ice, int slot); > +bool qcom_ice_hwkm_supported(struct qcom_ice *ice); > struct qcom_ice *of_qcom_ice_get(struct device *dev); > #endif /* __QCOM_ICE_H__ */
Hello Dmitry, On 06/17/2024 12:55 AM PDT, Dmitry Baryshkov wrote: > On Sun, Jun 16, 2024 at 05:50:59PM GMT, Gaurav Kashyap wrote: > > Qualcomm's ICE (Inline Crypto Engine) contains a proprietary key > > management hardware called Hardware Key Manager (HWKM). > > This patch integrates HWKM support in ICE when it is available. HWKM > > primarily provides hardware wrapped key support where the ICE > > (storage) keys are not available in software and protected in > > hardware. > > > > When HWKM software support is not fully available (from Trustzone), > > there can be a scenario where the ICE hardware supports HWKM, but it > > cannot be used for wrapped keys. In this case, standard keys have to > > be used without using HWKM. Hence, providing a toggle controlled by a > > devicetree entry to use HWKM or not. > > > > Tested-by: Neil Armstrong <neil.armstrong@linaro.org> > > Signed-off-by: Gaurav Kashyap <quic_gaurkash@quicinc.com> > > --- > > drivers/soc/qcom/ice.c | 153 > +++++++++++++++++++++++++++++++++++++++-- > > include/soc/qcom/ice.h | 1 + > > 2 files changed, 150 insertions(+), 4 deletions(-) > > > > diff --git a/drivers/soc/qcom/ice.c b/drivers/soc/qcom/ice.c index > > 6f941d32fffb..d5e74cf2946b 100644 > > --- a/drivers/soc/qcom/ice.c > > +++ b/drivers/soc/qcom/ice.c > > @@ -26,6 +26,40 @@ > > #define QCOM_ICE_REG_FUSE_SETTING 0x0010 > > #define QCOM_ICE_REG_BIST_STATUS 0x0070 > > #define QCOM_ICE_REG_ADVANCED_CONTROL 0x1000 > > +#define QCOM_ICE_REG_CONTROL 0x0 > > +/* QCOM ICE HWKM registers */ > > +#define QCOM_ICE_REG_HWKM_TZ_KM_CTL 0x1000 > > +#define QCOM_ICE_REG_HWKM_TZ_KM_STATUS 0x1004 > > +#define QCOM_ICE_REG_HWKM_BANK0_BANKN_IRQ_STATUS 0x2008 > > +#define QCOM_ICE_REG_HWKM_BANK0_BBAC_0 0x5000 > > +#define QCOM_ICE_REG_HWKM_BANK0_BBAC_1 0x5004 > > +#define QCOM_ICE_REG_HWKM_BANK0_BBAC_2 0x5008 > > +#define QCOM_ICE_REG_HWKM_BANK0_BBAC_3 0x500C > > +#define QCOM_ICE_REG_HWKM_BANK0_BBAC_4 0x5010 > > + > > +/* QCOM ICE HWKM reg vals */ > > +#define QCOM_ICE_HWKM_BIST_DONE_V1 BIT(16) > > +#define QCOM_ICE_HWKM_BIST_DONE_V2 BIT(9) > > +#define QCOM_ICE_HWKM_BIST_DONE(ver) > QCOM_ICE_HWKM_BIST_DONE_V##ver > > + > > +#define QCOM_ICE_HWKM_CRYPTO_BIST_DONE_V1 BIT(14) > > +#define QCOM_ICE_HWKM_CRYPTO_BIST_DONE_V2 BIT(7) > > +#define QCOM_ICE_HWKM_CRYPTO_BIST_DONE(v) > QCOM_ICE_HWKM_CRYPTO_BIST_DONE_V##v > > + > > +#define QCOM_ICE_HWKM_BOOT_CMD_LIST1_DONE BIT(2) > > +#define QCOM_ICE_HWKM_BOOT_CMD_LIST0_DONE BIT(1) > > +#define QCOM_ICE_HWKM_KT_CLEAR_DONE BIT(0) > > + > > +#define QCOM_ICE_HWKM_BIST_VAL(v) > (QCOM_ICE_HWKM_BIST_DONE(v) | \ > > + QCOM_ICE_HWKM_CRYPTO_BIST_DONE(v) | \ > > + QCOM_ICE_HWKM_BOOT_CMD_LIST1_DONE | \ > > + QCOM_ICE_HWKM_BOOT_CMD_LIST0_DONE | \ > > + QCOM_ICE_HWKM_KT_CLEAR_DONE) > > + > > +#define QCOM_ICE_HWKM_V1_STANDARD_MODE_VAL (BIT(0) | BIT(1) > | BIT(2)) > > +#define QCOM_ICE_HWKM_V2_STANDARD_MODE_MASK > GENMASK(31, 1) #define > > +QCOM_ICE_HWKM_DISABLE_CRC_CHECKS_VAL (BIT(1) | BIT(2)) > > +#define QCOM_ICE_HWKM_RSP_FIFO_CLEAR_VAL BIT(3) > > > > /* BIST ("built-in self-test") status flags */ > > #define QCOM_ICE_BIST_STATUS_MASK GENMASK(31, 28) > > @@ -34,6 +68,9 @@ > > #define QCOM_ICE_FORCE_HW_KEY0_SETTING_MASK 0x2 #define > > QCOM_ICE_FORCE_HW_KEY1_SETTING_MASK 0x4 > > > > +#define QCOM_ICE_HWKM_REG_OFFSET 0x8000 > > +#define HWKM_OFFSET(reg) ((reg) + > QCOM_ICE_HWKM_REG_OFFSET) > > + > > #define qcom_ice_writel(engine, val, reg) \ > > writel((val), (engine)->base + (reg)) > > > > @@ -46,6 +83,9 @@ struct qcom_ice { > > struct device_link *link; > > > > struct clk *core_clk; > > + u8 hwkm_version; > > + bool use_hwkm; > > + bool hwkm_init_complete; > > }; > > > > static bool qcom_ice_check_supported(struct qcom_ice *ice) @@ -63,8 > > +103,21 @@ static bool qcom_ice_check_supported(struct qcom_ice *ice) > > return false; > > } > > > > - dev_info(dev, "Found QC Inline Crypto Engine (ICE) v%d.%d.%d\n", > > - major, minor, step); > > + if (major >= 4 || (major == 3 && minor == 2 && step >= 1)) > > + ice->hwkm_version = 2; > > + else if (major == 3 && minor == 2) > > + ice->hwkm_version = 1; > > + else > > + ice->hwkm_version = 0; > > + > > + if (ice->hwkm_version == 0) > > + ice->use_hwkm = false; > > + > > + dev_info(dev, "Found QC Inline Crypto Engine (ICE) v%d.%d.%d, > HWKM v%d\n", > > + major, minor, step, ice->hwkm_version); > > + > > + if (!ice->use_hwkm) > > + dev_info(dev, "QC ICE HWKM (Hardware Key Manager) not > > + used/supported"); > > > > /* If fuses are blown, ICE might not work in the standard way. */ > > regval = qcom_ice_readl(ice, QCOM_ICE_REG_FUSE_SETTING); @@ > > -113,27 +166,106 @@ static void qcom_ice_optimization_enable(struct > qcom_ice *ice) > > * fails, so we needn't do it in software too, and (c) properly testing > > * storage encryption requires testing the full storage stack anyway, > > * and not relying on hardware-level self-tests. > > + * > > + * However, we still care about if HWKM BIST failed (when supported) > > + as > > + * important functionality would fail later, so disable hwkm on failure. > > */ > > static int qcom_ice_wait_bist_status(struct qcom_ice *ice) { > > u32 regval; > > + u32 bist_done_val; > > int err; > > > > err = readl_poll_timeout(ice->base + QCOM_ICE_REG_BIST_STATUS, > > regval, !(regval & QCOM_ICE_BIST_STATUS_MASK), > > 50, 5000); > > - if (err) > > + if (err) { > > dev_err(ice->dev, "Timed out waiting for ICE self-test > > to complete\n"); > > + return err; > > + } > > > > + if (ice->use_hwkm) { > > + bist_done_val = ice->hwkm_version == 1 ? > > + QCOM_ICE_HWKM_BIST_VAL(1) : > > + QCOM_ICE_HWKM_BIST_VAL(2); > > + if (qcom_ice_readl(ice, > > + > HWKM_OFFSET(QCOM_ICE_REG_HWKM_TZ_KM_STATUS)) != > > + bist_done_val) { > > + dev_err(ice->dev, "HWKM BIST error\n"); > > + ice->use_hwkm = false; > > + err = -ENODEV; > > + } > > + } > > return err; > > } > > > > +static void qcom_ice_enable_standard_mode(struct qcom_ice *ice) { > > + u32 val = 0; > > + > > + /* > > + * When ICE is in standard (hwkm) mode, it supports HW wrapped > > + * keys, and when it is in legacy mode, it only supports standard > > + * (non HW wrapped) keys. > > I can't say this is very logical. > > standard mode => HW wrapped keys > legacy mode => standard keys > > Consider changing the terms. > Ack, will make this clearer > > + * > > + * Put ICE in standard mode, ICE defaults to legacy mode. > > + * Legacy mode - ICE HWKM slave not supported. > > + * Standard mode - ICE HWKM slave supported. > > s/slave/some other term/ > Ack - will address this. > Is it possible to use both kind of keys when working on standard mode? > If not, it should be the user who selects what type of keys to be used. > Enforcing this via DT is not a way to go. > Unfortunately, that support is not there yet. When you say user, do you mean to have it as a filesystem mount option? The way the UFS/EMMC crypto layer is designed currently is that, this information is needed when the modules are loaded. https://lore.kernel.org/all/20231104211259.17448-2-ebiggers@kernel.org/#Z31drivers:ufs:core:ufshcd-crypto.c I am thinking of a way now to do this with DT, but without having a new vendor property. Is it acceptable to use the addressable range as the deciding factor? Say use legacy mode of ICE when the addressable size is 0x8000 and use HWKM mode of ICE when the addressable size is 0x10000. > > + * > > + * Depending on the version of HWKM, it is controlled by different > > + * registers in ICE. > > + */ > > + if (ice->hwkm_version >= 2) { > > + val = qcom_ice_readl(ice, QCOM_ICE_REG_CONTROL); > > + val = val & QCOM_ICE_HWKM_V2_STANDARD_MODE_MASK; > > + qcom_ice_writel(ice, val, QCOM_ICE_REG_CONTROL); > > + } else { > > + qcom_ice_writel(ice, > QCOM_ICE_HWKM_V1_STANDARD_MODE_VAL, > > + HWKM_OFFSET(QCOM_ICE_REG_HWKM_TZ_KM_CTL)); > > + } > > +} > > + > > +static void qcom_ice_hwkm_init(struct qcom_ice *ice) { > > + /* Disable CRC checks. This HWKM feature is not used. */ > > + qcom_ice_writel(ice, QCOM_ICE_HWKM_DISABLE_CRC_CHECKS_VAL, > > + HWKM_OFFSET(QCOM_ICE_REG_HWKM_TZ_KM_CTL)); > > + > > + /* > > + * 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. > > + */ > > + qcom_ice_writel(ice, GENMASK(31, 0), > HWKM_OFFSET(QCOM_ICE_REG_HWKM_BANK0_BBAC_0)); > > + qcom_ice_writel(ice, GENMASK(31, 0), > HWKM_OFFSET(QCOM_ICE_REG_HWKM_BANK0_BBAC_1)); > > + qcom_ice_writel(ice, GENMASK(31, 0), > HWKM_OFFSET(QCOM_ICE_REG_HWKM_BANK0_BBAC_2)); > > + qcom_ice_writel(ice, GENMASK(31, 0), > HWKM_OFFSET(QCOM_ICE_REG_HWKM_BANK0_BBAC_3)); > > + qcom_ice_writel(ice, GENMASK(31, 0), > > + HWKM_OFFSET(QCOM_ICE_REG_HWKM_BANK0_BBAC_4)); > > + > > + /* Clear HWKM response FIFO before doing anything */ > > + qcom_ice_writel(ice, QCOM_ICE_HWKM_RSP_FIFO_CLEAR_VAL, > > + > HWKM_OFFSET(QCOM_ICE_REG_HWKM_BANK0_BANKN_IRQ_STATUS)); > > + ice->hwkm_init_complete = true; > > +} > > + > > int qcom_ice_enable(struct qcom_ice *ice) { > > + int err; > > + > > qcom_ice_low_power_mode_enable(ice); > > qcom_ice_optimization_enable(ice); > > > > - return qcom_ice_wait_bist_status(ice); > > + if (ice->use_hwkm) > > + qcom_ice_enable_standard_mode(ice); > > + > > + err = qcom_ice_wait_bist_status(ice); > > + if (err) > > + return err; > > + > > + if (ice->use_hwkm) > > + qcom_ice_hwkm_init(ice); > > + > > + return err; > > } > > EXPORT_SYMBOL_GPL(qcom_ice_enable); > > > > @@ -149,6 +281,10 @@ int qcom_ice_resume(struct qcom_ice *ice) > > return err; > > } > > > > + if (ice->use_hwkm) { > > + qcom_ice_enable_standard_mode(ice); > > + qcom_ice_hwkm_init(ice); > > + } > > return qcom_ice_wait_bist_status(ice); } > > EXPORT_SYMBOL_GPL(qcom_ice_resume); > > @@ -156,6 +292,7 @@ EXPORT_SYMBOL_GPL(qcom_ice_resume); > > int qcom_ice_suspend(struct qcom_ice *ice) { > > clk_disable_unprepare(ice->core_clk); > > + ice->hwkm_init_complete = false; > > > > return 0; > > } > > @@ -205,6 +342,12 @@ int qcom_ice_evict_key(struct qcom_ice *ice, int > > slot) } EXPORT_SYMBOL_GPL(qcom_ice_evict_key); > > > > Documentation? > > > +bool qcom_ice_hwkm_supported(struct qcom_ice *ice) > > +{ > > + return ice->use_hwkm; > > I see that use_hwkm can change during runtime. Will it have an impact on > a driver that calls this first? > > > +} > > +EXPORT_SYMBOL_GPL(qcom_ice_hwkm_supported); > > + > > static struct qcom_ice *qcom_ice_create(struct device *dev, > > void __iomem *base) > > { > > @@ -239,6 +382,8 @@ static struct qcom_ice *qcom_ice_create(struct > device *dev, > > engine->core_clk = devm_clk_get_enabled(dev, NULL); > > if (IS_ERR(engine->core_clk)) > > return ERR_CAST(engine->core_clk); > > + engine->use_hwkm = of_property_read_bool(dev->of_node, > > + "qcom,ice-use-hwkm"); > > DT bindings should come before driver changes. > > > > > if (!qcom_ice_check_supported(engine)) > > return ERR_PTR(-EOPNOTSUPP); > > diff --git a/include/soc/qcom/ice.h b/include/soc/qcom/ice.h > > index 9dd835dba2a7..1f52e82e3e1c 100644 > > --- a/include/soc/qcom/ice.h > > +++ b/include/soc/qcom/ice.h > > @@ -34,5 +34,6 @@ int qcom_ice_program_key(struct qcom_ice *ice, > > const struct blk_crypto_key *bkey, > > u8 data_unit_size, int slot); > > int qcom_ice_evict_key(struct qcom_ice *ice, int slot); > > +bool qcom_ice_hwkm_supported(struct qcom_ice *ice); > > struct qcom_ice *of_qcom_ice_get(struct device *dev); > > #endif /* __QCOM_ICE_H__ */ > > -- > > 2.43.0 > > > > -- > With best wishes > Dmitry Regards, Gaurav
On Wed, 19 Jun 2024 at 01:07, Gaurav Kashyap (QUIC) <quic_gaurkash@quicinc.com> wrote: > > Hello Dmitry, > > On 06/17/2024 12:55 AM PDT, Dmitry Baryshkov wrote: > > On Sun, Jun 16, 2024 at 05:50:59PM GMT, Gaurav Kashyap wrote: > > > Qualcomm's ICE (Inline Crypto Engine) contains a proprietary key > > > management hardware called Hardware Key Manager (HWKM). > > > This patch integrates HWKM support in ICE when it is available. HWKM > > > primarily provides hardware wrapped key support where the ICE > > > (storage) keys are not available in software and protected in > > > hardware. > > > > > > When HWKM software support is not fully available (from Trustzone), > > > there can be a scenario where the ICE hardware supports HWKM, but it > > > cannot be used for wrapped keys. In this case, standard keys have to > > > be used without using HWKM. Hence, providing a toggle controlled by a > > > devicetree entry to use HWKM or not. > > > > > > Tested-by: Neil Armstrong <neil.armstrong@linaro.org> > > > Signed-off-by: Gaurav Kashyap <quic_gaurkash@quicinc.com> > > > --- > > > drivers/soc/qcom/ice.c | 153 > > +++++++++++++++++++++++++++++++++++++++-- > > > include/soc/qcom/ice.h | 1 + > > > 2 files changed, 150 insertions(+), 4 deletions(-) > > > > > > diff --git a/drivers/soc/qcom/ice.c b/drivers/soc/qcom/ice.c index > > > 6f941d32fffb..d5e74cf2946b 100644 > > > --- a/drivers/soc/qcom/ice.c > > > +++ b/drivers/soc/qcom/ice.c > > > @@ -26,6 +26,40 @@ > > > #define QCOM_ICE_REG_FUSE_SETTING 0x0010 > > > #define QCOM_ICE_REG_BIST_STATUS 0x0070 > > > #define QCOM_ICE_REG_ADVANCED_CONTROL 0x1000 > > > +#define QCOM_ICE_REG_CONTROL 0x0 > > > +/* QCOM ICE HWKM registers */ > > > +#define QCOM_ICE_REG_HWKM_TZ_KM_CTL 0x1000 > > > +#define QCOM_ICE_REG_HWKM_TZ_KM_STATUS 0x1004 > > > +#define QCOM_ICE_REG_HWKM_BANK0_BANKN_IRQ_STATUS 0x2008 > > > +#define QCOM_ICE_REG_HWKM_BANK0_BBAC_0 0x5000 > > > +#define QCOM_ICE_REG_HWKM_BANK0_BBAC_1 0x5004 > > > +#define QCOM_ICE_REG_HWKM_BANK0_BBAC_2 0x5008 > > > +#define QCOM_ICE_REG_HWKM_BANK0_BBAC_3 0x500C > > > +#define QCOM_ICE_REG_HWKM_BANK0_BBAC_4 0x5010 > > > + > > > +/* QCOM ICE HWKM reg vals */ > > > +#define QCOM_ICE_HWKM_BIST_DONE_V1 BIT(16) > > > +#define QCOM_ICE_HWKM_BIST_DONE_V2 BIT(9) > > > +#define QCOM_ICE_HWKM_BIST_DONE(ver) > > QCOM_ICE_HWKM_BIST_DONE_V##ver > > > + > > > +#define QCOM_ICE_HWKM_CRYPTO_BIST_DONE_V1 BIT(14) > > > +#define QCOM_ICE_HWKM_CRYPTO_BIST_DONE_V2 BIT(7) > > > +#define QCOM_ICE_HWKM_CRYPTO_BIST_DONE(v) > > QCOM_ICE_HWKM_CRYPTO_BIST_DONE_V##v > > > + > > > +#define QCOM_ICE_HWKM_BOOT_CMD_LIST1_DONE BIT(2) > > > +#define QCOM_ICE_HWKM_BOOT_CMD_LIST0_DONE BIT(1) > > > +#define QCOM_ICE_HWKM_KT_CLEAR_DONE BIT(0) > > > + > > > +#define QCOM_ICE_HWKM_BIST_VAL(v) > > (QCOM_ICE_HWKM_BIST_DONE(v) | \ > > > + QCOM_ICE_HWKM_CRYPTO_BIST_DONE(v) | \ > > > + QCOM_ICE_HWKM_BOOT_CMD_LIST1_DONE | \ > > > + QCOM_ICE_HWKM_BOOT_CMD_LIST0_DONE | \ > > > + QCOM_ICE_HWKM_KT_CLEAR_DONE) > > > + > > > +#define QCOM_ICE_HWKM_V1_STANDARD_MODE_VAL (BIT(0) | BIT(1) > > | BIT(2)) > > > +#define QCOM_ICE_HWKM_V2_STANDARD_MODE_MASK > > GENMASK(31, 1) #define > > > +QCOM_ICE_HWKM_DISABLE_CRC_CHECKS_VAL (BIT(1) | BIT(2)) > > > +#define QCOM_ICE_HWKM_RSP_FIFO_CLEAR_VAL BIT(3) > > > > > > /* BIST ("built-in self-test") status flags */ > > > #define QCOM_ICE_BIST_STATUS_MASK GENMASK(31, 28) > > > @@ -34,6 +68,9 @@ > > > #define QCOM_ICE_FORCE_HW_KEY0_SETTING_MASK 0x2 #define > > > QCOM_ICE_FORCE_HW_KEY1_SETTING_MASK 0x4 > > > > > > +#define QCOM_ICE_HWKM_REG_OFFSET 0x8000 > > > +#define HWKM_OFFSET(reg) ((reg) + > > QCOM_ICE_HWKM_REG_OFFSET) > > > + > > > #define qcom_ice_writel(engine, val, reg) \ > > > writel((val), (engine)->base + (reg)) > > > > > > @@ -46,6 +83,9 @@ struct qcom_ice { > > > struct device_link *link; > > > > > > struct clk *core_clk; > > > + u8 hwkm_version; > > > + bool use_hwkm; > > > + bool hwkm_init_complete; > > > }; > > > > > > static bool qcom_ice_check_supported(struct qcom_ice *ice) @@ -63,8 > > > +103,21 @@ static bool qcom_ice_check_supported(struct qcom_ice *ice) > > > return false; > > > } > > > > > > - dev_info(dev, "Found QC Inline Crypto Engine (ICE) v%d.%d.%d\n", > > > - major, minor, step); > > > + if (major >= 4 || (major == 3 && minor == 2 && step >= 1)) > > > + ice->hwkm_version = 2; > > > + else if (major == 3 && minor == 2) > > > + ice->hwkm_version = 1; > > > + else > > > + ice->hwkm_version = 0; > > > + > > > + if (ice->hwkm_version == 0) > > > + ice->use_hwkm = false; > > > + > > > + dev_info(dev, "Found QC Inline Crypto Engine (ICE) v%d.%d.%d, > > HWKM v%d\n", > > > + major, minor, step, ice->hwkm_version); > > > + > > > + if (!ice->use_hwkm) > > > + dev_info(dev, "QC ICE HWKM (Hardware Key Manager) not > > > + used/supported"); > > > > > > /* If fuses are blown, ICE might not work in the standard way. */ > > > regval = qcom_ice_readl(ice, QCOM_ICE_REG_FUSE_SETTING); @@ > > > -113,27 +166,106 @@ static void qcom_ice_optimization_enable(struct > > qcom_ice *ice) > > > * fails, so we needn't do it in software too, and (c) properly testing > > > * storage encryption requires testing the full storage stack anyway, > > > * and not relying on hardware-level self-tests. > > > + * > > > + * However, we still care about if HWKM BIST failed (when supported) > > > + as > > > + * important functionality would fail later, so disable hwkm on failure. > > > */ > > > static int qcom_ice_wait_bist_status(struct qcom_ice *ice) { > > > u32 regval; > > > + u32 bist_done_val; > > > int err; > > > > > > err = readl_poll_timeout(ice->base + QCOM_ICE_REG_BIST_STATUS, > > > regval, !(regval & QCOM_ICE_BIST_STATUS_MASK), > > > 50, 5000); > > > - if (err) > > > + if (err) { > > > dev_err(ice->dev, "Timed out waiting for ICE self-test > > > to complete\n"); > > > + return err; > > > + } > > > > > > + if (ice->use_hwkm) { > > > + bist_done_val = ice->hwkm_version == 1 ? > > > + QCOM_ICE_HWKM_BIST_VAL(1) : > > > + QCOM_ICE_HWKM_BIST_VAL(2); > > > + if (qcom_ice_readl(ice, > > > + > > HWKM_OFFSET(QCOM_ICE_REG_HWKM_TZ_KM_STATUS)) != > > > + bist_done_val) { > > > + dev_err(ice->dev, "HWKM BIST error\n"); > > > + ice->use_hwkm = false; > > > + err = -ENODEV; > > > + } > > > + } > > > return err; > > > } > > > > > > +static void qcom_ice_enable_standard_mode(struct qcom_ice *ice) { > > > + u32 val = 0; > > > + > > > + /* > > > + * When ICE is in standard (hwkm) mode, it supports HW wrapped > > > + * keys, and when it is in legacy mode, it only supports standard > > > + * (non HW wrapped) keys. > > > > I can't say this is very logical. > > > > standard mode => HW wrapped keys > > legacy mode => standard keys > > > > Consider changing the terms. > > > > Ack, will make this clearer > > > > + * > > > + * Put ICE in standard mode, ICE defaults to legacy mode. > > > + * Legacy mode - ICE HWKM slave not supported. > > > + * Standard mode - ICE HWKM slave supported. > > > > s/slave/some other term/ > > > Ack - will address this. > > > Is it possible to use both kind of keys when working on standard mode? > > If not, it should be the user who selects what type of keys to be used. > > Enforcing this via DT is not a way to go. > > > > Unfortunately, that support is not there yet. When you say user, do you mean to have it as a filesystem > mount option? During cryptsetup time. When running e.g. cryptsetup I, as a user, would like to be able to use either a hardware-wrapped key or a standard key. > The way the UFS/EMMC crypto layer is designed currently is that, this information > is needed when the modules are loaded. > > https://lore.kernel.org/all/20231104211259.17448-2-ebiggers@kernel.org/#Z31drivers:ufs:core:ufshcd-crypto.c I see that the driver lists capabilities here. E.g. that it supports HW-wrapped keys. But the line doesn't specify that standard keys are not supported. Also, I'd have expected that hw-wrapped keys are handled using trusted keys mechanism (see security/keys/trusted-keys/). Could you please point out why that's not the case? > I am thinking of a way now to do this with DT, but without having a new vendor property. > Is it acceptable to use the addressable range as the deciding factor? Say use legacy mode of ICE > when the addressable size is 0x8000 and use HWKM mode of ICE when the addressable size is > 0x10000. Definitely, this is a NAK. It's a very unobvious hack. You have been asked to use compatible strings to detect whether HW keys are supported or not.
Hello Krzysztof On 06/18/2024 11:17 PM PDT, Krzysztof Kozlowski wrote: > On 19/06/2024 00:08, Gaurav Kashyap (QUIC) wrote: > >> > >> You may perhaps only call qcom_scm_derive_sw_secret_available() for > >> some ICE versions. > >> > >> Neil > > > > The issue here is that for the same ICE version, based on the chipset, > > there might be different configurations. > > That's not what your DTS said. To remind: your DTS said that all SM8550 and > all SM8650 have it. Choice is obvious then: it's deducible from compatible. > > I still do not understand why your call cannot return you correct > "configuration". > ICE version and chipsets are disjoint, meaning for the same ICE HW present in SM8650 vs SMxxxx target, SM8650 will have necessary TZ support, but SM8xxxx may not, that is the reason I was trying to indicate all SM8550 and SM8650 have the necessary TZ support. There might have been a miscommunication there. However , availability of QCOM_SCM_ES_GENERATE_ICE_KEY will directly translate to having the necessary firmware support. So, I will pursue going that route and upload another set of patches to remove the DT property. > > > > Is it acceptable to use the addressable size from DTSI instead? > > Meaning, if it 0x8000, it would take the legacy route, and only when > > it has been updated to 0x10000, we would use HWKM and wrapped keys. > > No. > Ack > Best regards, > Krzysztof Regards, Gaurav
Hello Dmitry On 06/18/2024 3:17 PM PDT, Dmitry Baryshkov wrote: > On Wed, 19 Jun 2024 at 01:07, Gaurav Kashyap (QUIC) > <quic_gaurkash@quicinc.com> wrote: > > > > Hello Dmitry, > > > > On 06/17/2024 12:55 AM PDT, Dmitry Baryshkov wrote: > > > On Sun, Jun 16, 2024 at 05:50:59PM GMT, Gaurav Kashyap wrote: > > > > Qualcomm's ICE (Inline Crypto Engine) contains a proprietary key > > > > management hardware called Hardware Key Manager (HWKM). > > > > This patch integrates HWKM support in ICE when it is available. > > > > HWKM primarily provides hardware wrapped key support where the > ICE > > > > (storage) keys are not available in software and protected in > > > > hardware. > > > > > > > > When HWKM software support is not fully available (from > > > > Trustzone), there can be a scenario where the ICE hardware > > > > supports HWKM, but it cannot be used for wrapped keys. In this > > > > case, standard keys have to be used without using HWKM. Hence, > > > > providing a toggle controlled by a devicetree entry to use HWKM or not. > > > > > > > > Tested-by: Neil Armstrong <neil.armstrong@linaro.org> > > > > Signed-off-by: Gaurav Kashyap <quic_gaurkash@quicinc.com> > > > > --- > > > > drivers/soc/qcom/ice.c | 153 > > > +++++++++++++++++++++++++++++++++++++++-- > > > > include/soc/qcom/ice.h | 1 + > > > > 2 files changed, 150 insertions(+), 4 deletions(-) > > > > > > > > diff --git a/drivers/soc/qcom/ice.c b/drivers/soc/qcom/ice.c index > > > > 6f941d32fffb..d5e74cf2946b 100644 > > > > --- a/drivers/soc/qcom/ice.c > > > > +++ b/drivers/soc/qcom/ice.c > > > > @@ -26,6 +26,40 @@ > > > > #define QCOM_ICE_REG_FUSE_SETTING 0x0010 > > > > #define QCOM_ICE_REG_BIST_STATUS 0x0070 > > > > #define QCOM_ICE_REG_ADVANCED_CONTROL 0x1000 > > > > +#define QCOM_ICE_REG_CONTROL 0x0 > > > > +/* QCOM ICE HWKM registers */ > > > > +#define QCOM_ICE_REG_HWKM_TZ_KM_CTL 0x1000 > > > > +#define QCOM_ICE_REG_HWKM_TZ_KM_STATUS 0x1004 > > > > +#define QCOM_ICE_REG_HWKM_BANK0_BANKN_IRQ_STATUS > 0x2008 > > > > +#define QCOM_ICE_REG_HWKM_BANK0_BBAC_0 0x5000 > > > > +#define QCOM_ICE_REG_HWKM_BANK0_BBAC_1 0x5004 > > > > +#define QCOM_ICE_REG_HWKM_BANK0_BBAC_2 0x5008 > > > > +#define QCOM_ICE_REG_HWKM_BANK0_BBAC_3 0x500C > > > > +#define QCOM_ICE_REG_HWKM_BANK0_BBAC_4 0x5010 > > > > + > > > > +/* QCOM ICE HWKM reg vals */ > > > > +#define QCOM_ICE_HWKM_BIST_DONE_V1 BIT(16) > > > > +#define QCOM_ICE_HWKM_BIST_DONE_V2 BIT(9) > > > > +#define QCOM_ICE_HWKM_BIST_DONE(ver) > > > QCOM_ICE_HWKM_BIST_DONE_V##ver > > > > + > > > > +#define QCOM_ICE_HWKM_CRYPTO_BIST_DONE_V1 BIT(14) > > > > +#define QCOM_ICE_HWKM_CRYPTO_BIST_DONE_V2 BIT(7) > > > > +#define QCOM_ICE_HWKM_CRYPTO_BIST_DONE(v) > > > QCOM_ICE_HWKM_CRYPTO_BIST_DONE_V##v > > > > + > > > > +#define QCOM_ICE_HWKM_BOOT_CMD_LIST1_DONE BIT(2) > > > > +#define QCOM_ICE_HWKM_BOOT_CMD_LIST0_DONE BIT(1) > > > > +#define QCOM_ICE_HWKM_KT_CLEAR_DONE BIT(0) > > > > + > > > > +#define QCOM_ICE_HWKM_BIST_VAL(v) > > > (QCOM_ICE_HWKM_BIST_DONE(v) | \ > > > > + QCOM_ICE_HWKM_CRYPTO_BIST_DONE(v) | \ > > > > + QCOM_ICE_HWKM_BOOT_CMD_LIST1_DONE | \ > > > > + QCOM_ICE_HWKM_BOOT_CMD_LIST0_DONE | \ > > > > + QCOM_ICE_HWKM_KT_CLEAR_DONE) > > > > + > > > > +#define QCOM_ICE_HWKM_V1_STANDARD_MODE_VAL (BIT(0) | > BIT(1) > > > | BIT(2)) > > > > +#define QCOM_ICE_HWKM_V2_STANDARD_MODE_MASK > > > GENMASK(31, 1) #define > > > > +QCOM_ICE_HWKM_DISABLE_CRC_CHECKS_VAL (BIT(1) | BIT(2)) > > > > +#define QCOM_ICE_HWKM_RSP_FIFO_CLEAR_VAL BIT(3) > > > > > > > > /* BIST ("built-in self-test") status flags */ > > > > #define QCOM_ICE_BIST_STATUS_MASK GENMASK(31, 28) > > > > @@ -34,6 +68,9 @@ > > > > #define QCOM_ICE_FORCE_HW_KEY0_SETTING_MASK 0x2 #define > > > > QCOM_ICE_FORCE_HW_KEY1_SETTING_MASK 0x4 > > > > > > > > +#define QCOM_ICE_HWKM_REG_OFFSET 0x8000 > > > > +#define HWKM_OFFSET(reg) ((reg) + > > > QCOM_ICE_HWKM_REG_OFFSET) > > > > + > > > > #define qcom_ice_writel(engine, val, reg) \ > > > > writel((val), (engine)->base + (reg)) > > > > > > > > @@ -46,6 +83,9 @@ struct qcom_ice { > > > > struct device_link *link; > > > > > > > > struct clk *core_clk; > > > > + u8 hwkm_version; > > > > + bool use_hwkm; > > > > + bool hwkm_init_complete; > > > > }; > > > > > > > > static bool qcom_ice_check_supported(struct qcom_ice *ice) @@ > > > > -63,8 > > > > +103,21 @@ static bool qcom_ice_check_supported(struct qcom_ice > > > > +*ice) > > > > return false; > > > > } > > > > > > > > - dev_info(dev, "Found QC Inline Crypto Engine (ICE) v%d.%d.%d\n", > > > > - major, minor, step); > > > > + if (major >= 4 || (major == 3 && minor == 2 && step >= 1)) > > > > + ice->hwkm_version = 2; > > > > + else if (major == 3 && minor == 2) > > > > + ice->hwkm_version = 1; > > > > + else > > > > + ice->hwkm_version = 0; > > > > + > > > > + if (ice->hwkm_version == 0) > > > > + ice->use_hwkm = false; > > > > + > > > > + dev_info(dev, "Found QC Inline Crypto Engine (ICE) > > > > + v%d.%d.%d, > > > HWKM v%d\n", > > > > + major, minor, step, ice->hwkm_version); > > > > + > > > > + if (!ice->use_hwkm) > > > > + dev_info(dev, "QC ICE HWKM (Hardware Key Manager) > > > > + not used/supported"); > > > > > > > > /* If fuses are blown, ICE might not work in the standard way. */ > > > > regval = qcom_ice_readl(ice, QCOM_ICE_REG_FUSE_SETTING); @@ > > > > -113,27 +166,106 @@ static void > > > > qcom_ice_optimization_enable(struct > > > qcom_ice *ice) > > > > * fails, so we needn't do it in software too, and (c) properly testing > > > > * storage encryption requires testing the full storage stack anyway, > > > > * and not relying on hardware-level self-tests. > > > > + * > > > > + * However, we still care about if HWKM BIST failed (when > > > > + supported) as > > > > + * important functionality would fail later, so disable hwkm on failure. > > > > */ > > > > static int qcom_ice_wait_bist_status(struct qcom_ice *ice) { > > > > u32 regval; > > > > + u32 bist_done_val; > > > > int err; > > > > > > > > err = readl_poll_timeout(ice->base + > QCOM_ICE_REG_BIST_STATUS, > > > > regval, !(regval & QCOM_ICE_BIST_STATUS_MASK), > > > > 50, 5000); > > > > - if (err) > > > > + if (err) { > > > > dev_err(ice->dev, "Timed out waiting for ICE > > > > self-test to complete\n"); > > > > + return err; > > > > + } > > > > > > > > + if (ice->use_hwkm) { > > > > + bist_done_val = ice->hwkm_version == 1 ? > > > > + QCOM_ICE_HWKM_BIST_VAL(1) : > > > > + QCOM_ICE_HWKM_BIST_VAL(2); > > > > + if (qcom_ice_readl(ice, > > > > + > > > HWKM_OFFSET(QCOM_ICE_REG_HWKM_TZ_KM_STATUS)) != > > > > + bist_done_val) { > > > > + dev_err(ice->dev, "HWKM BIST error\n"); > > > > + ice->use_hwkm = false; > > > > + err = -ENODEV; > > > > + } > > > > + } > > > > return err; > > > > } > > > > > > > > +static void qcom_ice_enable_standard_mode(struct qcom_ice *ice) { > > > > + u32 val = 0; > > > > + > > > > + /* > > > > + * When ICE is in standard (hwkm) mode, it supports HW wrapped > > > > + * keys, and when it is in legacy mode, it only supports standard > > > > + * (non HW wrapped) keys. > > > > > > I can't say this is very logical. > > > > > > standard mode => HW wrapped keys > > > legacy mode => standard keys > > > > > > Consider changing the terms. > > > > > > > Ack, will make this clearer > > > > > > + * > > > > + * Put ICE in standard mode, ICE defaults to legacy mode. > > > > + * Legacy mode - ICE HWKM slave not supported. > > > > + * Standard mode - ICE HWKM slave supported. > > > > > > s/slave/some other term/ > > > > > Ack - will address this. > > > > > Is it possible to use both kind of keys when working on standard mode? > > > If not, it should be the user who selects what type of keys to be used. > > > Enforcing this via DT is not a way to go. > > > > > > > Unfortunately, that support is not there yet. When you say user, do > > you mean to have it as a filesystem mount option? > > During cryptsetup time. When running e.g. cryptsetup I, as a user, would like > to be able to use either a hardware-wrapped key or a standard key. > What we are looking for with these patches is for per-file/folder encryption using fscrypt policies. Cryptsetup to my understanding supports only full-disk , and does not support FBE (File-Based) Hence the idea here is that we mount an unencrypted device (with the inlinecrypt option that indicates inline encryption is supported) And specify policies (links to keys) for different folders. > > The way the UFS/EMMC crypto layer is designed currently is that, this > > information is needed when the modules are loaded. > > > > https://lore.kernel.org/all/20231104211259.17448-2-ebiggers@kernel.org > > /#Z31drivers:ufs:core:ufshcd-crypto.c > > I see that the driver lists capabilities here. E.g. that it supports HW-wrapped > keys. But the line doesn't specify that standard keys are not supported. > Those are capabilities that are read from the storage controller. However, wrapped keys Are not a standard in the ICE JEDEC specification, and in most cases, is a value add coming from the SoC. QCOM SOC and firmware currently does not support both kinds of keys in the HWKM mode. That is something we are internally working on, but not available yet. > Also, I'd have expected that hw-wrapped keys are handled using trusted > keys mechanism (see security/keys/trusted-keys/). Could you please point > out why that's not the case? > I will evaluate this. But my initial response is that we currently cannot communicate to our TPM directly from HLOS, but goes through QTEE, and I don't think our qtee currently interfaces with the open source tee driver. The interface is through QCOM SCM driver. > > I am thinking of a way now to do this with DT, but without having a new > vendor property. > > Is it acceptable to use the addressable range as the deciding factor? > > Say use legacy mode of ICE when the addressable size is 0x8000 and use > > HWKM mode of ICE when the addressable size is 0x10000. > > Definitely, this is a NAK. It's a very unobvious hack. You have been asked to > use compatible strings to detect whether HW keys are supported or not. > > -- > With best wishes > Dmitry Regards, Gaurav
Apologies, fixed incorrect email > Hello Eric > > On 06/20/2024, 9:48 PM PDT, Eric Biggers wrote: > > On Thu, Jun 20, 2024 at 02:57:40PM +0300, Dmitry Baryshkov wrote: > > > > > > > > > > > > > Is it possible to use both kind of keys when working on > > > > > > > standard > > mode? > > > > > > > If not, it should be the user who selects what type of keys > > > > > > > to be > > used. > > > > > > > Enforcing this via DT is not a way to go. > > > > > > > > > > > > > > > > > > > Unfortunately, that support is not there yet. When you say > > > > > > user, do you mean to have it as a filesystem mount option? > > > > > > > > > > During cryptsetup time. When running e.g. cryptsetup I, as a > > > > > user, would like to be able to use either a hardware-wrapped key > > > > > or a > > standard key. > > > > > > > > > > > > > What we are looking for with these patches is for per-file/folder > > encryption using fscrypt policies. > > > > Cryptsetup to my understanding supports only full-disk , and does > > > > not support FBE (File-Based) > > > > > > I must admit, I mostly used dm-crypt beforehand, so I had to look at > > > fscrypt now. Some of my previous comments might not be fully > > > applicable. > > > > > > > Hence the idea here is that we mount an unencrypted device (with > > > > the inlinecrypt option that indicates inline encryption is > > > > supported) And > > specify policies (links to keys) for different folders. > > > > > > > > > > The way the UFS/EMMC crypto layer is designed currently is > > > > > > that, this information is needed when the modules are loaded. > > > > > > > > > > > > https://lore.kernel.org/all/20231104211259.17448-2-ebiggers@ke > > > > > > rn el.org /#Z31drivers:ufs:core:ufshcd-crypto.c > > > > > > > > > > I see that the driver lists capabilities here. E.g. that it > > > > > supports HW-wrapped keys. But the line doesn't specify that > > > > > standard > > keys are not supported. > > > > > > > > > > > > > Those are capabilities that are read from the storage controller. > > > > However, wrapped keys Are not a standard in the ICE JEDEC > > > > specification, and in most cases, is a value add coming from the SoC. > > > > > > > > QCOM SOC and firmware currently does not support both kinds of > > > > keys in > > the HWKM mode. > > > > That is something we are internally working on, but not available yet. > > > > > > I'd say this is a significant obstacle, at least from my point of > > > view. I understand that the default might be to use hw-wrapped keys, > > > but it should be possible for the user to select non-HW keys if the > > > ability to recover the data is considered to be important. Note, I'm > > > really pointing to the user here, not to the system integrator. So > > > using DT property or specifying kernel arguments to switch between > > > these modes is not really an option. > > > > > > But I'd really love to hear some feedback from linux-security and/or > > > linux-fscrypt here. > > > > > > In my humble opinion the user should be able to specify that the key > > > is wrapped using the hardware KMK. Then if the hardware has already > > > started using the other kind of keys, it should be able to respond > > > with -EINVAL / whatever else. Then the user can evict previously > > > programmed key and program a desired one. > > > > > > > > Also, I'd have expected that hw-wrapped keys are handled using > > > > > trusted keys mechanism (see security/keys/trusted-keys/). Could > > > > > you please point out why that's not the case? > > > > > > > > > > > > > I will evaluate this. > > > > But my initial response is that we currently cannot communicate to > > > > our TPM directly from HLOS, but goes through QTEE, and I don't > > > > think our qtee currently interfaces with the open source tee > > > > driver. The > > interface is through QCOM SCM driver. > > > > > > Note, this is just an API interface, see how it is implemented for > > > the CAAM hardware. > > > > > > > The problem is that this patchset was sent out without the patches > > that add the block and filesystem-level framework for hardware-wrapped > > inline encryption keys, which it depends on. So it's lacking context. > > The proposed framework can be found at https://lore.kernel.org/linux- > > block/20231104211259.17448-1-ebiggers@kernel.org/T/#u > > > > I have only been adding the fscryp patch link as part of the cover letter - as a > dependency. > https://lore.kernel.org/all/20240617005825.1443206-1- > quic_gaurkash@quicinc.com/ > If you would like me to include it in the patch series itself, I can do that as > well. > > > As for why "trusted keys" aren't used, they just aren't helpful here. > > "Trusted keys" are based around a model where the kernel can request > > that keys be sealed and unsealed using a trust source, and the kernel > > gets access to the raw unsealed keys. Hardware-wrapped inline > > encryption keys use a different model where the kernel never gets > > access to the raw keys. They also have the concept of ephemeral > > wrapping which does not exist in "trusted keys". And they need to be > > properly integrated with the inline encryption framework in the block layer. > > > > - Eric > > Regards, > Gaurav
On Fri, Jun 21, 2024 at 03:35:40PM +0000, Gaurav Kashyap wrote: > Hello Eric > > On 06/20/2024, 9:48 PM PDT, Eric Biggers wrote: > > On Thu, Jun 20, 2024 at 02:57:40PM +0300, Dmitry Baryshkov wrote: > > > > > > > > > > > > > Is it possible to use both kind of keys when working on standard > > mode? > > > > > > > If not, it should be the user who selects what type of keys to be > > used. > > > > > > > Enforcing this via DT is not a way to go. > > > > > > > > > > > > > > > > > > > Unfortunately, that support is not there yet. When you say user, > > > > > > do you mean to have it as a filesystem mount option? > > > > > > > > > > During cryptsetup time. When running e.g. cryptsetup I, as a user, > > > > > would like to be able to use either a hardware-wrapped key or a > > standard key. > > > > > > > > > > > > > What we are looking for with these patches is for per-file/folder > > encryption using fscrypt policies. > > > > Cryptsetup to my understanding supports only full-disk , and does > > > > not support FBE (File-Based) > > > > > > I must admit, I mostly used dm-crypt beforehand, so I had to look at > > > fscrypt now. Some of my previous comments might not be fully > > > applicable. > > > > > > > Hence the idea here is that we mount an unencrypted device (with the > > > > inlinecrypt option that indicates inline encryption is supported) And > > specify policies (links to keys) for different folders. > > > > > > > > > > The way the UFS/EMMC crypto layer is designed currently is that, > > > > > > this information is needed when the modules are loaded. > > > > > > > > > > > > https://lore.kernel.org/all/20231104211259.17448-2-ebiggers@kern > > > > > > el.org /#Z31drivers:ufs:core:ufshcd-crypto.c > > > > > > > > > > I see that the driver lists capabilities here. E.g. that it > > > > > supports HW-wrapped keys. But the line doesn't specify that standard > > keys are not supported. > > > > > > > > > > > > > Those are capabilities that are read from the storage controller. > > > > However, wrapped keys Are not a standard in the ICE JEDEC > > > > specification, and in most cases, is a value add coming from the SoC. > > > > > > > > QCOM SOC and firmware currently does not support both kinds of keys in > > the HWKM mode. > > > > That is something we are internally working on, but not available yet. > > > > > > I'd say this is a significant obstacle, at least from my point of > > > view. I understand that the default might be to use hw-wrapped keys, > > > but it should be possible for the user to select non-HW keys if the > > > ability to recover the data is considered to be important. Note, I'm > > > really pointing to the user here, not to the system integrator. So > > > using DT property or specifying kernel arguments to switch between > > > these modes is not really an option. > > > > > > But I'd really love to hear some feedback from linux-security and/or > > > linux-fscrypt here. > > > > > > In my humble opinion the user should be able to specify that the key > > > is wrapped using the hardware KMK. Then if the hardware has already > > > started using the other kind of keys, it should be able to respond > > > with -EINVAL / whatever else. Then the user can evict previously > > > programmed key and program a desired one. > > > > > > > > Also, I'd have expected that hw-wrapped keys are handled using > > > > > trusted keys mechanism (see security/keys/trusted-keys/). Could > > > > > you please point out why that's not the case? > > > > > > > > > > > > > I will evaluate this. > > > > But my initial response is that we currently cannot communicate to > > > > our TPM directly from HLOS, but goes through QTEE, and I don't think > > > > our qtee currently interfaces with the open source tee driver. The > > interface is through QCOM SCM driver. > > > > > > Note, this is just an API interface, see how it is implemented for the > > > CAAM hardware. > > > > > > > The problem is that this patchset was sent out without the patches that add > > the block and filesystem-level framework for hardware-wrapped inline > > encryption keys, which it depends on. So it's lacking context. The proposed > > framework can be found at https://lore.kernel.org/linux- > > block/20231104211259.17448-1-ebiggers@kernel.org/T/#u > > > > I have only been adding the fscryp patch link as part of the cover letter - as a dependency. > https://lore.kernel.org/all/20240617005825.1443206-1-quic_gaurkash@quicinc.com/ > If you would like me to include it in the patch series itself, I can do that as well. > I think including all prerequisite patches would be helpful for reviewers. Thanks for continuing to work on this! I still need to get ahold of a sm8650 based device and test this out. Is the SM8650 HDK the only option, or is there a sm8650 based phone with upstream support yet? - Eric
On Fri, Jun 21, 2024 at 07:06:25PM +0300, Dmitry Baryshkov wrote: > On Fri, 21 Jun 2024 at 18:39, Eric Biggers <ebiggers@kernel.org> wrote: > > > > On Fri, Jun 21, 2024 at 06:16:37PM +0300, Dmitry Baryshkov wrote: > > > On Fri, 21 Jun 2024 at 07:47, Eric Biggers <ebiggers@kernel.org> wrote: > > > > > > > > On Thu, Jun 20, 2024 at 02:57:40PM +0300, Dmitry Baryshkov wrote: > > > > > > > > > > > > > > > > > Is it possible to use both kind of keys when working on standard mode? > > > > > > > > > If not, it should be the user who selects what type of keys to be used. > > > > > > > > > Enforcing this via DT is not a way to go. > > > > > > > > > > > > > > > > > > > > > > > > > Unfortunately, that support is not there yet. When you say user, do > > > > > > > > you mean to have it as a filesystem mount option? > > > > > > > > > > > > > > During cryptsetup time. When running e.g. cryptsetup I, as a user, would like > > > > > > > to be able to use either a hardware-wrapped key or a standard key. > > > > > > > > > > > > > > > > > > > What we are looking for with these patches is for per-file/folder encryption using fscrypt policies. > > > > > > Cryptsetup to my understanding supports only full-disk , and does not support FBE (File-Based) > > > > > > > > > > I must admit, I mostly used dm-crypt beforehand, so I had to look at > > > > > fscrypt now. Some of my previous comments might not be fully > > > > > applicable. > > > > > > > > > > > Hence the idea here is that we mount an unencrypted device (with the inlinecrypt option that indicates inline encryption is supported) > > > > > > And specify policies (links to keys) for different folders. > > > > > > > > > > > > > > The way the UFS/EMMC crypto layer is designed currently is that, this > > > > > > > > information is needed when the modules are loaded. > > > > > > > > > > > > > > > > https://lore.kernel.org/all/20231104211259.17448-2-ebiggers@kernel.org > > > > > > > > /#Z31drivers:ufs:core:ufshcd-crypto.c > > > > > > > > > > > > > > I see that the driver lists capabilities here. E.g. that it supports HW-wrapped > > > > > > > keys. But the line doesn't specify that standard keys are not supported. > > > > > > > > > > > > > > > > > > > Those are capabilities that are read from the storage controller. However, wrapped keys > > > > > > Are not a standard in the ICE JEDEC specification, and in most cases, is a value add coming > > > > > > from the SoC. > > > > > > > > > > > > QCOM SOC and firmware currently does not support both kinds of keys in the HWKM mode. > > > > > > That is something we are internally working on, but not available yet. > > > > > > > > > > I'd say this is a significant obstacle, at least from my point of > > > > > view. I understand that the default might be to use hw-wrapped keys, > > > > > but it should be possible for the user to select non-HW keys if the > > > > > ability to recover the data is considered to be important. Note, I'm > > > > > really pointing to the user here, not to the system integrator. So > > > > > using DT property or specifying kernel arguments to switch between > > > > > these modes is not really an option. > > > > > > > > > > But I'd really love to hear some feedback from linux-security and/or > > > > > linux-fscrypt here. > > > > > > > > > > In my humble opinion the user should be able to specify that the key > > > > > is wrapped using the hardware KMK. Then if the hardware has already > > > > > started using the other kind of keys, it should be able to respond > > > > > with -EINVAL / whatever else. Then the user can evict previously > > > > > programmed key and program a desired one. > > > > > > > > > > > > Also, I'd have expected that hw-wrapped keys are handled using trusted > > > > > > > keys mechanism (see security/keys/trusted-keys/). Could you please point > > > > > > > out why that's not the case? > > > > > > > > > > > > > > > > > > > I will evaluate this. > > > > > > But my initial response is that we currently cannot communicate to our TPM directly from HLOS, but > > > > > > goes through QTEE, and I don't think our qtee currently interfaces with the open source tee > > > > > > driver. The interface is through QCOM SCM driver. > > > > > > > > > > Note, this is just an API interface, see how it is implemented for the > > > > > CAAM hardware. > > > > > > > > > > > > > The problem is that this patchset was sent out without the patches that add the > > > > block and filesystem-level framework for hardware-wrapped inline encryption > > > > keys, which it depends on. So it's lacking context. The proposed framework can > > > > be found at > > > > https://lore.kernel.org/linux-block/20231104211259.17448-1-ebiggers@kernel.org/T/#u > > > > > > Thank you. I have quickly skimmed through the patches, but I didn't > > > review them thoroughly. Maybe the patchset already implements the > > > interfaces that I'm thinking about. In such a case please excuse me. I > > > will give it a more thorough look later today. > > > > > > > As for why "trusted keys" aren't used, they just aren't helpful here. "Trusted > > > > keys" are based around a model where the kernel can request that keys be sealed > > > > and unsealed using a trust source, and the kernel gets access to the raw > > > > unsealed keys. Hardware-wrapped inline encryption keys use a different model > > > > where the kernel never gets access to the raw keys. They also have the concept > > > > of ephemeral wrapping which does not exist in "trusted keys". And they need to > > > > be properly integrated with the inline encryption framework in the block layer. > > > > > > Then what exactly does qcom_scm_derive_sw_secret() do? Does it rewrap > > > the key under some other key? > > > > It derives a secret for functionality such as filenames encryption that can't > > use inline encryption. > > > > > I had the feeling that there are two separate pieces of functionality > > > being stuffed into a single patchset and into a single solution. > > > > > > First one is handling the keys. I keep on thinking that there should > > > be a separate software interface to unseal the key and rewrap it under > > > an ephemeral key. > > > > There is. That's what the BLKCRYPTOPREPAREKEY ioctl is for. > > > > > Some hardware might permit importing raw keys. > > > > That's what BLKCRYPTOIMPORTKEY is for. > > > > > Other hardware might insist on generating the keys on-chip so that raw keys > > > can never be used. > > > > And that's what BLKCRYPTOGENERATEKEY is for. > > Again, this might be answered somewhere, but why can't we use keyctl > for handling the keys and then use a single IOCTL to point the block > device to the key in the keyring? All the same functionality would need to be supported, and I think that shoehorning it into the keyrings service instead of just adding new ioctls would be more difficult. The keyrings service was not designed for this use case. We've already had a lot of problems trying to take advantage of the keyrings service in fscrypt previously. The keyrings service is something that sounds useful but really isn't all that useful. By "a single IOCTL to point the block device to the key in the keyring", you seem to be referring to configuring full block device encryption with a single key. That's not something that's supported by the upstream kernel yet, and it's not related to this patchset; currently only fscrypt supports inline encryption. Support for it will be added at some point, which will likely indeed take the form of an ioctl to set a key on a block device. But that would be the case even without HW-wrapped keys. And *requiring* the key to be given in a keyring (instead of just in a byte array passed to the ioctl) isn't very helpful, as it just makes the API harder to use. We've learned this from the fscrypt API already where we actually had to move away from the keyrings service in order to fix all the issues caused by it (see FS_IOC_ADD_ENCRYPTION_KEY). > > > > > Second part is the actual block interface. Gaurav wrote about > > > targeting fscrypt, but there should be no actual difference between > > > crypto targets. FDE or having a single partition encrypted should > > > probably work in the same way. Convert the key into blk_crypto_key > > > (including the cookie for the ephemeral key), program the key into the > > > slot, use the slot to en/decrypt hardware blocks. > > > > > > My main point is that the decision on the key type should be coming > > > from the user. > > > > That's exactly how it works. There is a block interface for specifying an > > inline encryption key along with each bio. The submitter of the bio can specify > > either a standard key or a HW-wrapped key. > > Not in this patchset. The ICE driver decides whether it can support > HW-wrapped keys or not and then fails to support other type of keys. > Sure, that's just a matter of hardware capabilities though, right? The block layer provides a way for drivers to declare which inline encryption capabilities they support. They can declare they support standard keys, HW-wrapped keys, both, or neither. If Qualcomm SoCs can't support both types of keys at the same time, that's unfortunate, but I'm not sure what your point is. The user (e.g. fscrypt) still has control over whether they use the functionality that the hardware provides. - Eric
On 25/06/2024 06:58, Gaurav Kashyap (QUIC) wrote: > > Hey Eric > > On 06/21/2024, 9:02 AM PDT, Eric Biggers wrote: >> On Fri, Jun 21, 2024 at 03:35:40PM +0000, Gaurav Kashyap wrote: >>> Hello Eric >>> >>> On 06/20/2024, 9:48 PM PDT, Eric Biggers wrote: >>>> On Thu, Jun 20, 2024 at 02:57:40PM +0300, Dmitry Baryshkov wrote: >>>>>>>> >>>>>>>>> Is it possible to use both kind of keys when working on >>>>>>>>> standard >>>> mode? >>>>>>>>> If not, it should be the user who selects what type of >>>>>>>>> keys to be >>>> used. >>>>>>>>> Enforcing this via DT is not a way to go. >>>>>>>>> >>>>>>>> >>>>>>>> Unfortunately, that support is not there yet. When you say >>>>>>>> user, do you mean to have it as a filesystem mount option? >>>>>>> >>>>>>> During cryptsetup time. When running e.g. cryptsetup I, as a >>>>>>> user, would like to be able to use either a hardware-wrapped >>>>>>> key or a >>>> standard key. >>>>>>> >>>>>> >>>>>> What we are looking for with these patches is for >>>>>> per-file/folder >>>> encryption using fscrypt policies. >>>>>> Cryptsetup to my understanding supports only full-disk , and >>>>>> does not support FBE (File-Based) >>>>> >>>>> I must admit, I mostly used dm-crypt beforehand, so I had to look >>>>> at fscrypt now. Some of my previous comments might not be fully >>>>> applicable. >>>>> >>>>>> Hence the idea here is that we mount an unencrypted device (with >>>>>> the inlinecrypt option that indicates inline encryption is >>>>>> supported) And >>>> specify policies (links to keys) for different folders. >>>>>> >>>>>>>> The way the UFS/EMMC crypto layer is designed currently is >>>>>>>> that, this information is needed when the modules are loaded. >>>>>>>> >>>>>>>> https://lore.kernel.org/all/20231104211259.17448-2-ebiggers@ >>>>>>>> kern el.org /#Z31drivers:ufs:core:ufshcd-crypto.c >>>>>>> >>>>>>> I see that the driver lists capabilities here. E.g. that it >>>>>>> supports HW-wrapped keys. But the line doesn't specify that >>>>>>> standard >>>> keys are not supported. >>>>>>> >>>>>> >>>>>> Those are capabilities that are read from the storage controller. >>>>>> However, wrapped keys Are not a standard in the ICE JEDEC >>>>>> specification, and in most cases, is a value add coming from the SoC. >>>>>> >>>>>> QCOM SOC and firmware currently does not support both kinds of >>>>>> keys in >>>> the HWKM mode. >>>>>> That is something we are internally working on, but not available yet. >>>>> >>>>> I'd say this is a significant obstacle, at least from my point of >>>>> view. I understand that the default might be to use hw-wrapped >>>>> keys, but it should be possible for the user to select non-HW keys >>>>> if the ability to recover the data is considered to be important. >>>>> Note, I'm really pointing to the user here, not to the system >>>>> integrator. So using DT property or specifying kernel arguments to >>>>> switch between these modes is not really an option. >>>>> >>>>> But I'd really love to hear some feedback from linux-security >>>>> and/or linux-fscrypt here. >>>>> >>>>> In my humble opinion the user should be able to specify that the >>>>> key is wrapped using the hardware KMK. Then if the hardware has >>>>> already started using the other kind of keys, it should be able to >>>>> respond with -EINVAL / whatever else. Then the user can evict >>>>> previously programmed key and program a desired one. >>>>> >>>>>>> Also, I'd have expected that hw-wrapped keys are handled using >>>>>>> trusted keys mechanism (see security/keys/trusted-keys/). >>>>>>> Could you please point out why that's not the case? >>>>>>> >>>>>> >>>>>> I will evaluate this. >>>>>> But my initial response is that we currently cannot communicate >>>>>> to our TPM directly from HLOS, but goes through QTEE, and I >>>>>> don't think our qtee currently interfaces with the open source >>>>>> tee driver. The >>>> interface is through QCOM SCM driver. >>>>> >>>>> Note, this is just an API interface, see how it is implemented for >>>>> the CAAM hardware. >>>>> >>>> >>>> The problem is that this patchset was sent out without the patches >>>> that add the block and filesystem-level framework for >>>> hardware-wrapped inline encryption keys, which it depends on. So >>>> it's lacking context. The proposed framework can be found at >>>> https://lore.kernel.org/linux- >>>> block/20231104211259.17448-1-ebiggers@kernel.org/T/#u >>>> >>> >>> I have only been adding the fscryp patch link as part of the cover letter - as >> a dependency. >>> https://lore.kernel.org/all/20240617005825.1443206-1-quic_gaurkash@qui >>> cinc.com/ If you would like me to include it in the patch series >>> itself, I can do that as well. >>> >> >> I think including all prerequisite patches would be helpful for reviewers. > > Noted. I'll do that for the next patch. > >> >> Thanks for continuing to work on this! >> >> I still need to get ahold of a sm8650 based device and test this out. Is the >> SM8650 HDK the only option, or is there a sm8650 based phone with >> upstream support yet? Yes you should be able to buy the SM8650 HDK from lantronix: https://www.lantronix.com/products/snapdragon-8-gen-3-mobile-hardware-development-kit/ It should be supported in v6.11 Neil > > There are some devices released with SM8650 (Snapdragon 8 Gen 3). Sorry, I have > not kept track of which. I know the S24s were released with that. But there should be > more in the market. > >> >> - Eric