| Message ID | 20240906-wrapped-keys-v6-9-d59e61bc0cb4@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Hardware wrapped key support for QCom ICE and UFS core | expand |
On 07/09/2024 00:07, Dmitry Baryshkov wrote: > On Fri, Sep 06, 2024 at 08:07:12PM GMT, Bartosz Golaszewski wrote: >> From: Gaurav Kashyap <quic_gaurkash@quicinc.com> >> >> Qualcomm's ICE (Inline Crypto Engine) contains a proprietary key >> management hardware called Hardware Key Manager (HWKM). Add HWKM support >> to the ICE driver if it is available on the platform. HWKM primarily >> provides hardware wrapped key support where the ICE (storage) keys are >> not available in software and instead 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, raw keys have to be used without >> using the HWKM. We query the TZ at run-time to find out whether wrapped >> keys support is available. >> >> Tested-by: Neil Armstrong <neil.armstrong@linaro.org> >> Signed-off-by: Gaurav Kashyap <quic_gaurkash@quicinc.com> >> Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> >> --- >> drivers/soc/qcom/ice.c | 152 +++++++++++++++++++++++++++++++++++++++++++++++-- >> include/soc/qcom/ice.h | 1 + >> 2 files changed, 149 insertions(+), 4 deletions(-) >> >> 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); >> >> @@ -150,6 +282,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); >> @@ -157,6 +293,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; >> } >> @@ -206,6 +343,12 @@ int qcom_ice_evict_key(struct qcom_ice *ice, int slot) >> } >> EXPORT_SYMBOL_GPL(qcom_ice_evict_key); >> >> +bool qcom_ice_hwkm_supported(struct qcom_ice *ice) >> +{ >> + return ice->use_hwkm; >> +} >> +EXPORT_SYMBOL_GPL(qcom_ice_hwkm_supported); >> + >> static struct qcom_ice *qcom_ice_create(struct device *dev, >> void __iomem *base) >> { >> @@ -240,6 +383,7 @@ 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 = qcom_scm_has_wrapped_key_support(); > > This still makes the decision on whether to use HW-wrapped keys on > behalf of a user. I suppose this is incorrect. The user must be able to > use raw keys even if HW-wrapped keys are available on the platform. One > of the examples for such use-cases is if a user prefers to be able to > recover stored information in case of a device failure (such recovery > will be impossible if SoC is damaged and HW-wrapped keys are used). Isn't that already the case ? the BLK_CRYPTO_KEY_TYPE_HW_WRAPPED size is here to select HW-wrapped key, otherwise the ol' raw key is passed. Just look the next patch. Or did I miss something ? 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__ */ >> >> -- >> 2.43.0 >> >
On Mon, Sep 09, 2024 at 10:58:30AM GMT, Neil Armstrong wrote: > On 07/09/2024 00:07, Dmitry Baryshkov wrote: > > On Fri, Sep 06, 2024 at 08:07:12PM GMT, Bartosz Golaszewski wrote: > > > From: Gaurav Kashyap <quic_gaurkash@quicinc.com> > > > > > > Qualcomm's ICE (Inline Crypto Engine) contains a proprietary key > > > management hardware called Hardware Key Manager (HWKM). Add HWKM support > > > to the ICE driver if it is available on the platform. HWKM primarily > > > provides hardware wrapped key support where the ICE (storage) keys are > > > not available in software and instead 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, raw keys have to be used without > > > using the HWKM. We query the TZ at run-time to find out whether wrapped > > > keys support is available. > > > > > > Tested-by: Neil Armstrong <neil.armstrong@linaro.org> > > > Signed-off-by: Gaurav Kashyap <quic_gaurkash@quicinc.com> > > > Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> > > > --- > > > drivers/soc/qcom/ice.c | 152 +++++++++++++++++++++++++++++++++++++++++++++++-- > > > include/soc/qcom/ice.h | 1 + > > > 2 files changed, 149 insertions(+), 4 deletions(-) > > > > > > 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); > > > @@ -150,6 +282,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); > > > @@ -157,6 +293,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; > > > } > > > @@ -206,6 +343,12 @@ int qcom_ice_evict_key(struct qcom_ice *ice, int slot) > > > } > > > EXPORT_SYMBOL_GPL(qcom_ice_evict_key); > > > +bool qcom_ice_hwkm_supported(struct qcom_ice *ice) > > > +{ > > > + return ice->use_hwkm; > > > +} > > > +EXPORT_SYMBOL_GPL(qcom_ice_hwkm_supported); > > > + > > > static struct qcom_ice *qcom_ice_create(struct device *dev, > > > void __iomem *base) > > > { > > > @@ -240,6 +383,7 @@ 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 = qcom_scm_has_wrapped_key_support(); > > > > This still makes the decision on whether to use HW-wrapped keys on > > behalf of a user. I suppose this is incorrect. The user must be able to > > use raw keys even if HW-wrapped keys are available on the platform. One > > of the examples for such use-cases is if a user prefers to be able to > > recover stored information in case of a device failure (such recovery > > will be impossible if SoC is damaged and HW-wrapped keys are used). > > Isn't that already the case ? the BLK_CRYPTO_KEY_TYPE_HW_WRAPPED size is > here to select HW-wrapped key, otherwise the ol' raw key is passed. > Just look the next patch. > > Or did I miss something ? That's a good question. If use_hwkm is set, ICE gets programmed to use hwkm (see qcom_ice_hwkm_init() call above). I'm not sure if it is expected to work properly if after such a call we pass raw key. > > 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__ */ > > > > > > -- > > > 2.43.0 > > > > > >
Hello Dmitry and Neil On Monday, September 9, 2024 2:44 AM PDT, Dmitry Baryshkov wrote: > On Mon, Sep 09, 2024 at 10:58:30AM GMT, Neil Armstrong wrote: > > On 07/09/2024 00:07, Dmitry Baryshkov wrote: > > > On Fri, Sep 06, 2024 at 08:07:12PM GMT, Bartosz Golaszewski wrote: > > > > From: Gaurav Kashyap <quic_gaurkash@quicinc.com> > > > > > > > > Qualcomm's ICE (Inline Crypto Engine) contains a proprietary key > > > > management hardware called Hardware Key Manager (HWKM). Add > HWKM > > > > support to the ICE driver if it is available on the platform. HWKM > > > > primarily provides hardware wrapped key support where the ICE > > > > (storage) keys are not available in software and instead 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, raw keys have to be used without using the HWKM. We query > > > > the TZ at run-time to find out whether wrapped keys support is > available. > > > > > > > > Tested-by: Neil Armstrong <neil.armstrong@linaro.org> > > > > Signed-off-by: Gaurav Kashyap <quic_gaurkash@quicinc.com> > > > > Signed-off-by: Bartosz Golaszewski > > > > <bartosz.golaszewski@linaro.org> > > > > --- > > > > drivers/soc/qcom/ice.c | 152 > +++++++++++++++++++++++++++++++++++++++++++++++-- > > > > include/soc/qcom/ice.h | 1 + > > > > 2 files changed, 149 insertions(+), 4 deletions(-) > > > > > > > > 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); > > > > @@ -150,6 +282,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); > > > > @@ -157,6 +293,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; > > > > } > > > > @@ -206,6 +343,12 @@ int qcom_ice_evict_key(struct qcom_ice *ice, > int slot) > > > > } > > > > EXPORT_SYMBOL_GPL(qcom_ice_evict_key); > > > > +bool qcom_ice_hwkm_supported(struct qcom_ice *ice) { return > > > > +ice->use_hwkm; } > EXPORT_SYMBOL_GPL(qcom_ice_hwkm_supported); > > > > + > > > > static struct qcom_ice *qcom_ice_create(struct device *dev, > > > > void __iomem *base) > > > > { > > > > @@ -240,6 +383,7 @@ 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 = qcom_scm_has_wrapped_key_support(); > > > > > > This still makes the decision on whether to use HW-wrapped keys on > > > behalf of a user. I suppose this is incorrect. The user must be able > > > to use raw keys even if HW-wrapped keys are available on the > > > platform. One of the examples for such use-cases is if a user > > > prefers to be able to recover stored information in case of a device > > > failure (such recovery will be impossible if SoC is damaged and HW- > wrapped keys are used). > > > > Isn't that already the case ? the BLK_CRYPTO_KEY_TYPE_HW_WRAPPED > size > > is here to select HW-wrapped key, otherwise the ol' raw key is passed. > > Just look the next patch. > > > > Or did I miss something ? > > That's a good question. If use_hwkm is set, ICE gets programmed to use > hwkm (see qcom_ice_hwkm_init() call above). I'm not sure if it is expected > to work properly if after such a call we pass raw key. > Once ICE has moved to a HWKM mode, the firmware key programming currently does not support raw keys. This support is being added for the next Qualcomm chipset in Trustzone to support both at he same time, but that will take another year or two to hit the market. Until that time, due to TZ (firmware) limitations , the driver can only support one or the other. We also cannot keep moving ICE modes, due to the HWKM enablement being a one-time configurable value at boot. > > > > 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__ */ > > > > > > > > -- > > > > 2.43.0 > > > > > > > > > > > -- > With best wishes > Dmitry Regards, Gaurav
On Tue, 10 Sept 2024 at 03:51, Gaurav Kashyap (QUIC) <quic_gaurkash@quicinc.com> wrote: > > Hello Dmitry and Neil > > On Monday, September 9, 2024 2:44 AM PDT, Dmitry Baryshkov wrote: > > On Mon, Sep 09, 2024 at 10:58:30AM GMT, Neil Armstrong wrote: > > > On 07/09/2024 00:07, Dmitry Baryshkov wrote: > > > > On Fri, Sep 06, 2024 at 08:07:12PM GMT, Bartosz Golaszewski wrote: > > > > > From: Gaurav Kashyap <quic_gaurkash@quicinc.com> > > > > > > > > > > Qualcomm's ICE (Inline Crypto Engine) contains a proprietary key > > > > > management hardware called Hardware Key Manager (HWKM). Add > > HWKM > > > > > support to the ICE driver if it is available on the platform. HWKM > > > > > primarily provides hardware wrapped key support where the ICE > > > > > (storage) keys are not available in software and instead 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, raw keys have to be used without using the HWKM. We query > > > > > the TZ at run-time to find out whether wrapped keys support is > > available. > > > > > > > > > > Tested-by: Neil Armstrong <neil.armstrong@linaro.org> > > > > > Signed-off-by: Gaurav Kashyap <quic_gaurkash@quicinc.com> > > > > > Signed-off-by: Bartosz Golaszewski > > > > > <bartosz.golaszewski@linaro.org> > > > > > --- > > > > > drivers/soc/qcom/ice.c | 152 > > +++++++++++++++++++++++++++++++++++++++++++++++-- > > > > > include/soc/qcom/ice.h | 1 + > > > > > 2 files changed, 149 insertions(+), 4 deletions(-) > > > > > > > > > > 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); > > > > > @@ -150,6 +282,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); > > > > > @@ -157,6 +293,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; > > > > > } > > > > > @@ -206,6 +343,12 @@ int qcom_ice_evict_key(struct qcom_ice *ice, > > int slot) > > > > > } > > > > > EXPORT_SYMBOL_GPL(qcom_ice_evict_key); > > > > > +bool qcom_ice_hwkm_supported(struct qcom_ice *ice) { return > > > > > +ice->use_hwkm; } > > EXPORT_SYMBOL_GPL(qcom_ice_hwkm_supported); > > > > > + > > > > > static struct qcom_ice *qcom_ice_create(struct device *dev, > > > > > void __iomem *base) > > > > > { > > > > > @@ -240,6 +383,7 @@ 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 = qcom_scm_has_wrapped_key_support(); > > > > > > > > This still makes the decision on whether to use HW-wrapped keys on > > > > behalf of a user. I suppose this is incorrect. The user must be able > > > > to use raw keys even if HW-wrapped keys are available on the > > > > platform. One of the examples for such use-cases is if a user > > > > prefers to be able to recover stored information in case of a device > > > > failure (such recovery will be impossible if SoC is damaged and HW- > > wrapped keys are used). > > > > > > Isn't that already the case ? the BLK_CRYPTO_KEY_TYPE_HW_WRAPPED > > size > > > is here to select HW-wrapped key, otherwise the ol' raw key is passed. > > > Just look the next patch. > > > > > > Or did I miss something ? > > > > That's a good question. If use_hwkm is set, ICE gets programmed to use > > hwkm (see qcom_ice_hwkm_init() call above). I'm not sure if it is expected > > to work properly if after such a call we pass raw key. > > > > Once ICE has moved to a HWKM mode, the firmware key programming currently does not support raw keys. > This support is being added for the next Qualcomm chipset in Trustzone to support both at he same time, but that will take another year or two to hit the market. > Until that time, due to TZ (firmware) limitations , the driver can only support one or the other. > > We also cannot keep moving ICE modes, due to the HWKM enablement being a one-time configurable value at boot. So the init of HWKM should be delayed until the point where the user tells if HWKM or raw keys should be used. > > > > > > > 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__ */ > > > > > > > > > > -- > > > > > 2.43.0 > > > > > > > > > > > > > > > > -- > > With best wishes > > Dmitry > > Regards, > Gaurav
On Monday, September 9, 2024 11:29 PM PDT, Dmitry Baryshkov wrote: > On Tue, 10 Sept 2024 at 03:51, Gaurav Kashyap (QUIC) > <quic_gaurkash@quicinc.com> wrote: > > > > Hello Dmitry and Neil > > > > On Monday, September 9, 2024 2:44 AM PDT, Dmitry Baryshkov wrote: > > > On Mon, Sep 09, 2024 at 10:58:30AM GMT, Neil Armstrong wrote: > > > > On 07/09/2024 00:07, Dmitry Baryshkov wrote: > > > > > On Fri, Sep 06, 2024 at 08:07:12PM GMT, Bartosz Golaszewski wrote: > > > > > > From: Gaurav Kashyap <quic_gaurkash@quicinc.com> > > > > > > > > > > > > Qualcomm's ICE (Inline Crypto Engine) contains a proprietary > > > > > > key management hardware called Hardware Key Manager (HWKM). > > > > > > Add > > > HWKM > > > > > > support to the ICE driver if it is available on the platform. > > > > > > HWKM primarily provides hardware wrapped key support where > the > > > > > > ICE > > > > > > (storage) keys are not available in software and instead > > > > > > 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, raw keys have to be used without using the HWKM. We > > > > > > query the TZ at run-time to find out whether wrapped keys > > > > > > support is > > > available. > > > > > > > > > > > > Tested-by: Neil Armstrong <neil.armstrong@linaro.org> > > > > > > Signed-off-by: Gaurav Kashyap <quic_gaurkash@quicinc.com> > > > > > > Signed-off-by: Bartosz Golaszewski > > > > > > <bartosz.golaszewski@linaro.org> > > > > > > --- > > > > > > drivers/soc/qcom/ice.c | 152 > > > +++++++++++++++++++++++++++++++++++++++++++++++-- > > > > > > include/soc/qcom/ice.h | 1 + > > > > > > 2 files changed, 149 insertions(+), 4 deletions(-) > > > > > > > > > > > > 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); > > > > > > @@ -150,6 +282,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); > > > > > > @@ -157,6 +293,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; > > > > > > } > > > > > > @@ -206,6 +343,12 @@ int qcom_ice_evict_key(struct qcom_ice > > > > > > *ice, > > > int slot) > > > > > > } > > > > > > EXPORT_SYMBOL_GPL(qcom_ice_evict_key); > > > > > > +bool qcom_ice_hwkm_supported(struct qcom_ice *ice) { return > > > > > > +ice->use_hwkm; } > > > EXPORT_SYMBOL_GPL(qcom_ice_hwkm_supported); > > > > > > + > > > > > > static struct qcom_ice *qcom_ice_create(struct device *dev, > > > > > > void __iomem *base) > > > > > > { > > > > > > @@ -240,6 +383,7 @@ 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 = qcom_scm_has_wrapped_key_support(); > > > > > > > > > > This still makes the decision on whether to use HW-wrapped keys > > > > > on behalf of a user. I suppose this is incorrect. The user must > > > > > be able to use raw keys even if HW-wrapped keys are available on > > > > > the platform. One of the examples for such use-cases is if a > > > > > user prefers to be able to recover stored information in case of > > > > > a device failure (such recovery will be impossible if SoC is > > > > > damaged and HW- > > > wrapped keys are used). > > > > > > > > Isn't that already the case ? the > BLK_CRYPTO_KEY_TYPE_HW_WRAPPED > > > size > > > > is here to select HW-wrapped key, otherwise the ol' raw key is passed. > > > > Just look the next patch. > > > > > > > > Or did I miss something ? > > > > > > That's a good question. If use_hwkm is set, ICE gets programmed to > > > use hwkm (see qcom_ice_hwkm_init() call above). I'm not sure if it > > > is expected to work properly if after such a call we pass raw key. > > > > > > > Once ICE has moved to a HWKM mode, the firmware key programming > currently does not support raw keys. > > This support is being added for the next Qualcomm chipset in Trustzone to > support both at he same time, but that will take another year or two to hit > the market. > > Until that time, due to TZ (firmware) limitations , the driver can only > support one or the other. > > > > We also cannot keep moving ICE modes, due to the HWKM enablement > being a one-time configurable value at boot. > > So the init of HWKM should be delayed until the point where the user tells if > HWKM or raw keys should be used. Ack. I'll work with Bartosz to look into moving to HWKM mode only during the first key program request > > > > > > > > > > > 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__ */ > > > > > > > > > > > > -- > > > > > > 2.43.0 > > > > > > > > > > > > > > > > > > > > > -- > > > With best wishes > > > Dmitry > > > > Regards, > > Gaurav > > > > -- > With best wishes > Dmitry Regards, Gaurav
On Thu, Sep 12, 2024 at 10:17:03PM +0000, Gaurav Kashyap (QUIC) wrote: > > On Monday, September 9, 2024 11:29 PM PDT, Dmitry Baryshkov wrote: > > On Tue, 10 Sept 2024 at 03:51, Gaurav Kashyap (QUIC) > > <quic_gaurkash@quicinc.com> wrote: > > > > > > Hello Dmitry and Neil > > > > > > On Monday, September 9, 2024 2:44 AM PDT, Dmitry Baryshkov wrote: > > > > On Mon, Sep 09, 2024 at 10:58:30AM GMT, Neil Armstrong wrote: > > > > > On 07/09/2024 00:07, Dmitry Baryshkov wrote: > > > > > > On Fri, Sep 06, 2024 at 08:07:12PM GMT, Bartosz Golaszewski wrote: > > > > > > > From: Gaurav Kashyap <quic_gaurkash@quicinc.com> > > > > > > > > > > > > > > Qualcomm's ICE (Inline Crypto Engine) contains a proprietary > > > > > > > key management hardware called Hardware Key Manager (HWKM). > > > > > > > Add > > > > HWKM > > > > > > > support to the ICE driver if it is available on the platform. > > > > > > > HWKM primarily provides hardware wrapped key support where > > the > > > > > > > ICE > > > > > > > (storage) keys are not available in software and instead > > > > > > > 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, raw keys have to be used without using the HWKM. We > > > > > > > query the TZ at run-time to find out whether wrapped keys > > > > > > > support is > > > > available. > > > > > > > > > > > > > > Tested-by: Neil Armstrong <neil.armstrong@linaro.org> > > > > > > > Signed-off-by: Gaurav Kashyap <quic_gaurkash@quicinc.com> > > > > > > > Signed-off-by: Bartosz Golaszewski > > > > > > > <bartosz.golaszewski@linaro.org> > > > > > > > --- > > > > > > > drivers/soc/qcom/ice.c | 152 > > > > +++++++++++++++++++++++++++++++++++++++++++++++-- > > > > > > > include/soc/qcom/ice.h | 1 + > > > > > > > 2 files changed, 149 insertions(+), 4 deletions(-) > > > > > > > > > > > > > > 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); > > > > > > > @@ -150,6 +282,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); > > > > > > > @@ -157,6 +293,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; > > > > > > > } > > > > > > > @@ -206,6 +343,12 @@ int qcom_ice_evict_key(struct qcom_ice > > > > > > > *ice, > > > > int slot) > > > > > > > } > > > > > > > EXPORT_SYMBOL_GPL(qcom_ice_evict_key); > > > > > > > +bool qcom_ice_hwkm_supported(struct qcom_ice *ice) { return > > > > > > > +ice->use_hwkm; } > > > > EXPORT_SYMBOL_GPL(qcom_ice_hwkm_supported); > > > > > > > + > > > > > > > static struct qcom_ice *qcom_ice_create(struct device *dev, > > > > > > > void __iomem *base) > > > > > > > { > > > > > > > @@ -240,6 +383,7 @@ 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 = qcom_scm_has_wrapped_key_support(); > > > > > > > > > > > > This still makes the decision on whether to use HW-wrapped keys > > > > > > on behalf of a user. I suppose this is incorrect. The user must > > > > > > be able to use raw keys even if HW-wrapped keys are available on > > > > > > the platform. One of the examples for such use-cases is if a > > > > > > user prefers to be able to recover stored information in case of > > > > > > a device failure (such recovery will be impossible if SoC is > > > > > > damaged and HW- > > > > wrapped keys are used). > > > > > > > > > > Isn't that already the case ? the > > BLK_CRYPTO_KEY_TYPE_HW_WRAPPED > > > > size > > > > > is here to select HW-wrapped key, otherwise the ol' raw key is passed. > > > > > Just look the next patch. > > > > > > > > > > Or did I miss something ? > > > > > > > > That's a good question. If use_hwkm is set, ICE gets programmed to > > > > use hwkm (see qcom_ice_hwkm_init() call above). I'm not sure if it > > > > is expected to work properly if after such a call we pass raw key. > > > > > > > > > > Once ICE has moved to a HWKM mode, the firmware key programming > > currently does not support raw keys. > > > This support is being added for the next Qualcomm chipset in Trustzone to > > support both at he same time, but that will take another year or two to hit > > the market. > > > Until that time, due to TZ (firmware) limitations , the driver can only > > support one or the other. > > > > > > We also cannot keep moving ICE modes, due to the HWKM enablement > > being a one-time configurable value at boot. > > > > So the init of HWKM should be delayed until the point where the user tells if > > HWKM or raw keys should be used. > > Ack. > I'll work with Bartosz to look into moving to HWKM mode only during the first key program request > That would mean the driver would have to initially advertise support for both HW-wrapped keys and raw keys, and then it would revoke the support for one of them later (due to the other one being used). However, runtime revocation of crypto capabilities is not supported by the blk-crypto framework (Documentation/block/inline-encryption.rst), and there is no clear path to adding such support. Upper layers may have already checked the crypto capabilities and decided to use them. It's too late to find out that the support was revoked in the middle of an I/O request. Upper layer code (blk-crypto, fscrypt, etc.) is not prepared for this. And even if it was, the best it could do is cleanly fail the I/O, which is too late as e.g. it may happen during background writeback and cause user data to be thrown away. So, the choice of support for HW-wrapped vs. raw will need to be made ahead of time, rather than being implicitly set by the first use. That is most easily done using a module parameter like qcom_ice.hw_wrapped_keys=1. Yes, it's a bit inconvenient, but there's no realistic way around this currently. - Eric
On Fri, 13 Sept 2024 at 02:17, Eric Biggers <ebiggers@kernel.org> wrote: > > On Thu, Sep 12, 2024 at 10:17:03PM +0000, Gaurav Kashyap (QUIC) wrote: > > > > On Monday, September 9, 2024 11:29 PM PDT, Dmitry Baryshkov wrote: > > > On Tue, 10 Sept 2024 at 03:51, Gaurav Kashyap (QUIC) > > > <quic_gaurkash@quicinc.com> wrote: > > > > > > > > Hello Dmitry and Neil > > > > > > > > On Monday, September 9, 2024 2:44 AM PDT, Dmitry Baryshkov wrote: > > > > > On Mon, Sep 09, 2024 at 10:58:30AM GMT, Neil Armstrong wrote: > > > > > > On 07/09/2024 00:07, Dmitry Baryshkov wrote: > > > > > > > On Fri, Sep 06, 2024 at 08:07:12PM GMT, Bartosz Golaszewski wrote: > > > > > > > > From: Gaurav Kashyap <quic_gaurkash@quicinc.com> > > > > > > > > > > > > > > > > Qualcomm's ICE (Inline Crypto Engine) contains a proprietary > > > > > > > > key management hardware called Hardware Key Manager (HWKM). > > > > > > > > Add > > > > > HWKM > > > > > > > > support to the ICE driver if it is available on the platform. > > > > > > > > HWKM primarily provides hardware wrapped key support where > > > the > > > > > > > > ICE > > > > > > > > (storage) keys are not available in software and instead > > > > > > > > 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, raw keys have to be used without using the HWKM. We > > > > > > > > query the TZ at run-time to find out whether wrapped keys > > > > > > > > support is > > > > > available. > > > > > > > > > > > > > > > > Tested-by: Neil Armstrong <neil.armstrong@linaro.org> > > > > > > > > Signed-off-by: Gaurav Kashyap <quic_gaurkash@quicinc.com> > > > > > > > > Signed-off-by: Bartosz Golaszewski > > > > > > > > <bartosz.golaszewski@linaro.org> > > > > > > > > --- > > > > > > > > drivers/soc/qcom/ice.c | 152 > > > > > +++++++++++++++++++++++++++++++++++++++++++++++-- > > > > > > > > include/soc/qcom/ice.h | 1 + > > > > > > > > 2 files changed, 149 insertions(+), 4 deletions(-) > > > > > > > > > > > > > > > > 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); > > > > > > > > @@ -150,6 +282,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); > > > > > > > > @@ -157,6 +293,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; > > > > > > > > } > > > > > > > > @@ -206,6 +343,12 @@ int qcom_ice_evict_key(struct qcom_ice > > > > > > > > *ice, > > > > > int slot) > > > > > > > > } > > > > > > > > EXPORT_SYMBOL_GPL(qcom_ice_evict_key); > > > > > > > > +bool qcom_ice_hwkm_supported(struct qcom_ice *ice) { return > > > > > > > > +ice->use_hwkm; } > > > > > EXPORT_SYMBOL_GPL(qcom_ice_hwkm_supported); > > > > > > > > + > > > > > > > > static struct qcom_ice *qcom_ice_create(struct device *dev, > > > > > > > > void __iomem *base) > > > > > > > > { > > > > > > > > @@ -240,6 +383,7 @@ 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 = qcom_scm_has_wrapped_key_support(); > > > > > > > > > > > > > > This still makes the decision on whether to use HW-wrapped keys > > > > > > > on behalf of a user. I suppose this is incorrect. The user must > > > > > > > be able to use raw keys even if HW-wrapped keys are available on > > > > > > > the platform. One of the examples for such use-cases is if a > > > > > > > user prefers to be able to recover stored information in case of > > > > > > > a device failure (such recovery will be impossible if SoC is > > > > > > > damaged and HW- > > > > > wrapped keys are used). > > > > > > > > > > > > Isn't that already the case ? the > > > BLK_CRYPTO_KEY_TYPE_HW_WRAPPED > > > > > size > > > > > > is here to select HW-wrapped key, otherwise the ol' raw key is passed. > > > > > > Just look the next patch. > > > > > > > > > > > > Or did I miss something ? > > > > > > > > > > That's a good question. If use_hwkm is set, ICE gets programmed to > > > > > use hwkm (see qcom_ice_hwkm_init() call above). I'm not sure if it > > > > > is expected to work properly if after such a call we pass raw key. > > > > > > > > > > > > > Once ICE has moved to a HWKM mode, the firmware key programming > > > currently does not support raw keys. > > > > This support is being added for the next Qualcomm chipset in Trustzone to > > > support both at he same time, but that will take another year or two to hit > > > the market. > > > > Until that time, due to TZ (firmware) limitations , the driver can only > > > support one or the other. > > > > > > > > We also cannot keep moving ICE modes, due to the HWKM enablement > > > being a one-time configurable value at boot. > > > > > > So the init of HWKM should be delayed until the point where the user tells if > > > HWKM or raw keys should be used. > > > > Ack. > > I'll work with Bartosz to look into moving to HWKM mode only during the first key program request > > > > That would mean the driver would have to initially advertise support for both > HW-wrapped keys and raw keys, and then it would revoke the support for one of > them later (due to the other one being used). However, runtime revocation of > crypto capabilities is not supported by the blk-crypto framework > (Documentation/block/inline-encryption.rst), and there is no clear path to > adding such support. Upper layers may have already checked the crypto > capabilities and decided to use them. It's too late to find out that the > support was revoked in the middle of an I/O request. Upper layer code > (blk-crypto, fscrypt, etc.) is not prepared for this. And even if it was, the > best it could do is cleanly fail the I/O, which is too late as e.g. it may > happen during background writeback and cause user data to be thrown away. Can we check crypto capabilities when the user sets the key? Compare this to the actual HSM used to secure communication or storage. It has certain capabilities, which can be enumerated, etc. But then at the time the user sets the key it is perfectly normal to return an error because HSM is out of resources. It might even have spare key slots, but it might be not enough to be able to program the required key (as a really crazy example, consider the HSM having at this time a single spare DES key slot, while the user wants to program 3DES key). > > So, the choice of support for HW-wrapped vs. raw will need to be made ahead of > time, rather than being implicitly set by the first use. That is most easily > done using a module parameter like qcom_ice.hw_wrapped_keys=1. Yes, it's a bit > inconvenient, but there's no realistic way around this currently. This doesn't work for Android usecase. The user isn't able to setup modparams.
On Fri, Sep 13, 2024 at 07:28:33AM +0300, Dmitry Baryshkov wrote: > On Fri, 13 Sept 2024 at 02:17, Eric Biggers <ebiggers@kernel.org> wrote: > > > > On Thu, Sep 12, 2024 at 10:17:03PM +0000, Gaurav Kashyap (QUIC) wrote: > > > > > > On Monday, September 9, 2024 11:29 PM PDT, Dmitry Baryshkov wrote: > > > > On Tue, 10 Sept 2024 at 03:51, Gaurav Kashyap (QUIC) > > > > <quic_gaurkash@quicinc.com> wrote: > > > > > > > > > > Hello Dmitry and Neil > > > > > > > > > > On Monday, September 9, 2024 2:44 AM PDT, Dmitry Baryshkov wrote: > > > > > > On Mon, Sep 09, 2024 at 10:58:30AM GMT, Neil Armstrong wrote: > > > > > > > On 07/09/2024 00:07, Dmitry Baryshkov wrote: > > > > > > > > On Fri, Sep 06, 2024 at 08:07:12PM GMT, Bartosz Golaszewski wrote: > > > > > > > > > From: Gaurav Kashyap <quic_gaurkash@quicinc.com> > > > > > > > > > > > > > > > > > > Qualcomm's ICE (Inline Crypto Engine) contains a proprietary > > > > > > > > > key management hardware called Hardware Key Manager (HWKM). > > > > > > > > > Add > > > > > > HWKM > > > > > > > > > support to the ICE driver if it is available on the platform. > > > > > > > > > HWKM primarily provides hardware wrapped key support where > > > > the > > > > > > > > > ICE > > > > > > > > > (storage) keys are not available in software and instead > > > > > > > > > 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, raw keys have to be used without using the HWKM. We > > > > > > > > > query the TZ at run-time to find out whether wrapped keys > > > > > > > > > support is > > > > > > available. > > > > > > > > > > > > > > > > > > Tested-by: Neil Armstrong <neil.armstrong@linaro.org> > > > > > > > > > Signed-off-by: Gaurav Kashyap <quic_gaurkash@quicinc.com> > > > > > > > > > Signed-off-by: Bartosz Golaszewski > > > > > > > > > <bartosz.golaszewski@linaro.org> > > > > > > > > > --- > > > > > > > > > drivers/soc/qcom/ice.c | 152 > > > > > > +++++++++++++++++++++++++++++++++++++++++++++++-- > > > > > > > > > include/soc/qcom/ice.h | 1 + > > > > > > > > > 2 files changed, 149 insertions(+), 4 deletions(-) > > > > > > > > > > > > > > > > > > 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); > > > > > > > > > @@ -150,6 +282,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); > > > > > > > > > @@ -157,6 +293,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; > > > > > > > > > } > > > > > > > > > @@ -206,6 +343,12 @@ int qcom_ice_evict_key(struct qcom_ice > > > > > > > > > *ice, > > > > > > int slot) > > > > > > > > > } > > > > > > > > > EXPORT_SYMBOL_GPL(qcom_ice_evict_key); > > > > > > > > > +bool qcom_ice_hwkm_supported(struct qcom_ice *ice) { return > > > > > > > > > +ice->use_hwkm; } > > > > > > EXPORT_SYMBOL_GPL(qcom_ice_hwkm_supported); > > > > > > > > > + > > > > > > > > > static struct qcom_ice *qcom_ice_create(struct device *dev, > > > > > > > > > void __iomem *base) > > > > > > > > > { > > > > > > > > > @@ -240,6 +383,7 @@ 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 = qcom_scm_has_wrapped_key_support(); > > > > > > > > > > > > > > > > This still makes the decision on whether to use HW-wrapped keys > > > > > > > > on behalf of a user. I suppose this is incorrect. The user must > > > > > > > > be able to use raw keys even if HW-wrapped keys are available on > > > > > > > > the platform. One of the examples for such use-cases is if a > > > > > > > > user prefers to be able to recover stored information in case of > > > > > > > > a device failure (such recovery will be impossible if SoC is > > > > > > > > damaged and HW- > > > > > > wrapped keys are used). > > > > > > > > > > > > > > Isn't that already the case ? the > > > > BLK_CRYPTO_KEY_TYPE_HW_WRAPPED > > > > > > size > > > > > > > is here to select HW-wrapped key, otherwise the ol' raw key is passed. > > > > > > > Just look the next patch. > > > > > > > > > > > > > > Or did I miss something ? > > > > > > > > > > > > That's a good question. If use_hwkm is set, ICE gets programmed to > > > > > > use hwkm (see qcom_ice_hwkm_init() call above). I'm not sure if it > > > > > > is expected to work properly if after such a call we pass raw key. > > > > > > > > > > > > > > > > Once ICE has moved to a HWKM mode, the firmware key programming > > > > currently does not support raw keys. > > > > > This support is being added for the next Qualcomm chipset in Trustzone to > > > > support both at he same time, but that will take another year or two to hit > > > > the market. > > > > > Until that time, due to TZ (firmware) limitations , the driver can only > > > > support one or the other. > > > > > > > > > > We also cannot keep moving ICE modes, due to the HWKM enablement > > > > being a one-time configurable value at boot. > > > > > > > > So the init of HWKM should be delayed until the point where the user tells if > > > > HWKM or raw keys should be used. > > > > > > Ack. > > > I'll work with Bartosz to look into moving to HWKM mode only during the first key program request > > > > > > > That would mean the driver would have to initially advertise support for both > > HW-wrapped keys and raw keys, and then it would revoke the support for one of > > them later (due to the other one being used). However, runtime revocation of > > crypto capabilities is not supported by the blk-crypto framework > > (Documentation/block/inline-encryption.rst), and there is no clear path to > > adding such support. Upper layers may have already checked the crypto > > capabilities and decided to use them. It's too late to find out that the > > support was revoked in the middle of an I/O request. Upper layer code > > (blk-crypto, fscrypt, etc.) is not prepared for this. And even if it was, the > > best it could do is cleanly fail the I/O, which is too late as e.g. it may > > happen during background writeback and cause user data to be thrown away. > > Can we check crypto capabilities when the user sets the key? I think you mean when a key is programmed into a keyslot? That happens during I/O, which is too late as I've explained above. > Compare this to the actual HSM used to secure communication or > storage. It has certain capabilities, which can be enumerated, etc. > But then at the time the user sets the key it is perfectly normal to > return an error because HSM is out of resources. It might even have > spare key slots, but it might be not enough to be able to program the > required key (as a really crazy example, consider the HSM having at > this time a single spare DES key slot, while the user wants to program > 3DES key). That isn't how the kernel handles inline encryption keyslots. They are only programmed as needed for I/O. If they are all in-use by pending I/O requests, then the kernel waits for an I/O request to finish and reprograms the keyslot it was using. There is never an error reported due to lack of keyslots. If I/O requests could randomly fail at any time when using inline encryption, then no one would use inline encryption because it would not be reliable. > > So, the choice of support for HW-wrapped vs. raw will need to be made ahead of > > time, rather than being implicitly set by the first use. That is most easily > > done using a module parameter like qcom_ice.hw_wrapped_keys=1. Yes, it's a bit > > inconvenient, but there's no realistic way around this currently. > > This doesn't work for Android usecase. The user isn't able to setup modparams. It does work for Android. The encryption setting that Android uses is configured in the build of Android for the device (by the OEM, or by whoever made the build in the case of a custom build). Refer to https://source.android.com/docs/security/features/encryption/file-based#enabling-file-based-encryption Anyone who can change that can also change the kernel command line. - Eric
On 13/09/2024 01:17, Eric Biggers wrote: > On Thu, Sep 12, 2024 at 10:17:03PM +0000, Gaurav Kashyap (QUIC) wrote: >> >> On Monday, September 9, 2024 11:29 PM PDT, Dmitry Baryshkov wrote: >>> On Tue, 10 Sept 2024 at 03:51, Gaurav Kashyap (QUIC) >>> <quic_gaurkash@quicinc.com> wrote: >>>> >>>> Hello Dmitry and Neil >>>> >>>> On Monday, September 9, 2024 2:44 AM PDT, Dmitry Baryshkov wrote: >>>>> On Mon, Sep 09, 2024 at 10:58:30AM GMT, Neil Armstrong wrote: >>>>>> On 07/09/2024 00:07, Dmitry Baryshkov wrote: >>>>>>> On Fri, Sep 06, 2024 at 08:07:12PM GMT, Bartosz Golaszewski wrote: >>>>>>>> From: Gaurav Kashyap <quic_gaurkash@quicinc.com> >>>>>>>> >>>>>>>> Qualcomm's ICE (Inline Crypto Engine) contains a proprietary >>>>>>>> key management hardware called Hardware Key Manager (HWKM). >>>>>>>> Add >>>>> HWKM >>>>>>>> support to the ICE driver if it is available on the platform. >>>>>>>> HWKM primarily provides hardware wrapped key support where >>> the >>>>>>>> ICE >>>>>>>> (storage) keys are not available in software and instead >>>>>>>> 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, raw keys have to be used without using the HWKM. We >>>>>>>> query the TZ at run-time to find out whether wrapped keys >>>>>>>> support is >>>>> available. >>>>>>>> >>>>>>>> Tested-by: Neil Armstrong <neil.armstrong@linaro.org> >>>>>>>> Signed-off-by: Gaurav Kashyap <quic_gaurkash@quicinc.com> >>>>>>>> Signed-off-by: Bartosz Golaszewski >>>>>>>> <bartosz.golaszewski@linaro.org> >>>>>>>> --- >>>>>>>> drivers/soc/qcom/ice.c | 152 >>>>> +++++++++++++++++++++++++++++++++++++++++++++++-- >>>>>>>> include/soc/qcom/ice.h | 1 + >>>>>>>> 2 files changed, 149 insertions(+), 4 deletions(-) >>>>>>>> >>>>>>>> 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); >>>>>>>> @@ -150,6 +282,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); >>>>>>>> @@ -157,6 +293,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; >>>>>>>> } >>>>>>>> @@ -206,6 +343,12 @@ int qcom_ice_evict_key(struct qcom_ice >>>>>>>> *ice, >>>>> int slot) >>>>>>>> } >>>>>>>> EXPORT_SYMBOL_GPL(qcom_ice_evict_key); >>>>>>>> +bool qcom_ice_hwkm_supported(struct qcom_ice *ice) { return >>>>>>>> +ice->use_hwkm; } >>>>> EXPORT_SYMBOL_GPL(qcom_ice_hwkm_supported); >>>>>>>> + >>>>>>>> static struct qcom_ice *qcom_ice_create(struct device *dev, >>>>>>>> void __iomem *base) >>>>>>>> { >>>>>>>> @@ -240,6 +383,7 @@ 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 = qcom_scm_has_wrapped_key_support(); >>>>>>> >>>>>>> This still makes the decision on whether to use HW-wrapped keys >>>>>>> on behalf of a user. I suppose this is incorrect. The user must >>>>>>> be able to use raw keys even if HW-wrapped keys are available on >>>>>>> the platform. One of the examples for such use-cases is if a >>>>>>> user prefers to be able to recover stored information in case of >>>>>>> a device failure (such recovery will be impossible if SoC is >>>>>>> damaged and HW- >>>>> wrapped keys are used). >>>>>> >>>>>> Isn't that already the case ? the >>> BLK_CRYPTO_KEY_TYPE_HW_WRAPPED >>>>> size >>>>>> is here to select HW-wrapped key, otherwise the ol' raw key is passed. >>>>>> Just look the next patch. >>>>>> >>>>>> Or did I miss something ? >>>>> >>>>> That's a good question. If use_hwkm is set, ICE gets programmed to >>>>> use hwkm (see qcom_ice_hwkm_init() call above). I'm not sure if it >>>>> is expected to work properly if after such a call we pass raw key. >>>>> >>>> >>>> Once ICE has moved to a HWKM mode, the firmware key programming >>> currently does not support raw keys. >>>> This support is being added for the next Qualcomm chipset in Trustzone to >>> support both at he same time, but that will take another year or two to hit >>> the market. >>>> Until that time, due to TZ (firmware) limitations , the driver can only >>> support one or the other. >>>> >>>> We also cannot keep moving ICE modes, due to the HWKM enablement >>> being a one-time configurable value at boot. >>> >>> So the init of HWKM should be delayed until the point where the user tells if >>> HWKM or raw keys should be used. >> >> Ack. >> I'll work with Bartosz to look into moving to HWKM mode only during the first key program request >> > > That would mean the driver would have to initially advertise support for both > HW-wrapped keys and raw keys, and then it would revoke the support for one of > them later (due to the other one being used). However, runtime revocation of > crypto capabilities is not supported by the blk-crypto framework > (Documentation/block/inline-encryption.rst), and there is no clear path to > adding such support. Upper layers may have already checked the crypto > capabilities and decided to use them. It's too late to find out that the > support was revoked in the middle of an I/O request. Upper layer code > (blk-crypto, fscrypt, etc.) is not prepared for this. And even if it was, the > best it could do is cleanly fail the I/O, which is too late as e.g. it may > happen during background writeback and cause user data to be thrown away. > > So, the choice of support for HW-wrapped vs. raw will need to be made ahead of > time, rather than being implicitly set by the first use. That is most easily > done using a module parameter like qcom_ice.hw_wrapped_keys=1. Yes, it's a bit > inconvenient, but there's no realistic way around this currently. Considering the arguments, I'll vote in favor of a module parameter, since using hw_wrapped_keys is a system design choice, it's fine to enable it via a module parameter. It will complicate CI, but in the actual case we just can't disable RAW keys support just because the firmware can potentially use wrapper keys. Neil > > - Eric
On Fri, Sep 13, 2024 at 04:57:16AM GMT, Eric Biggers wrote: > On Fri, Sep 13, 2024 at 07:28:33AM +0300, Dmitry Baryshkov wrote: > > On Fri, 13 Sept 2024 at 02:17, Eric Biggers <ebiggers@kernel.org> wrote: > > > > > > On Thu, Sep 12, 2024 at 10:17:03PM +0000, Gaurav Kashyap (QUIC) wrote: > > > > > > > > On Monday, September 9, 2024 11:29 PM PDT, Dmitry Baryshkov wrote: > > > > > On Tue, 10 Sept 2024 at 03:51, Gaurav Kashyap (QUIC) > > > > > <quic_gaurkash@quicinc.com> wrote: > > > > > > > > > > > > Hello Dmitry and Neil > > > > > > > > > > > > On Monday, September 9, 2024 2:44 AM PDT, Dmitry Baryshkov wrote: > > > > > > > On Mon, Sep 09, 2024 at 10:58:30AM GMT, Neil Armstrong wrote: > > > > > > > > On 07/09/2024 00:07, Dmitry Baryshkov wrote: > > > > > > > > > On Fri, Sep 06, 2024 at 08:07:12PM GMT, Bartosz Golaszewski wrote: > > > > > > > > > > From: Gaurav Kashyap <quic_gaurkash@quicinc.com> > > > > > > > > > > > > > > > > > > > > Qualcomm's ICE (Inline Crypto Engine) contains a proprietary > > > > > > > > > > key management hardware called Hardware Key Manager (HWKM). > > > > > > > > > > Add > > > > > > > HWKM > > > > > > > > > > support to the ICE driver if it is available on the platform. > > > > > > > > > > HWKM primarily provides hardware wrapped key support where > > > > > the > > > > > > > > > > ICE > > > > > > > > > > (storage) keys are not available in software and instead > > > > > > > > > > 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, raw keys have to be used without using the HWKM. We > > > > > > > > > > query the TZ at run-time to find out whether wrapped keys > > > > > > > > > > support is > > > > > > > available. > > > > > > > > > > > > > > > > > > > > Tested-by: Neil Armstrong <neil.armstrong@linaro.org> > > > > > > > > > > Signed-off-by: Gaurav Kashyap <quic_gaurkash@quicinc.com> > > > > > > > > > > Signed-off-by: Bartosz Golaszewski > > > > > > > > > > <bartosz.golaszewski@linaro.org> > > > > > > > > > > --- > > > > > > > > > > drivers/soc/qcom/ice.c | 152 > > > > > > > +++++++++++++++++++++++++++++++++++++++++++++++-- > > > > > > > > > > include/soc/qcom/ice.h | 1 + > > > > > > > > > > 2 files changed, 149 insertions(+), 4 deletions(-) > > > > > > > > > > > > > > > > > > > > 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); > > > > > > > > > > @@ -150,6 +282,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); > > > > > > > > > > @@ -157,6 +293,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; > > > > > > > > > > } > > > > > > > > > > @@ -206,6 +343,12 @@ int qcom_ice_evict_key(struct qcom_ice > > > > > > > > > > *ice, > > > > > > > int slot) > > > > > > > > > > } > > > > > > > > > > EXPORT_SYMBOL_GPL(qcom_ice_evict_key); > > > > > > > > > > +bool qcom_ice_hwkm_supported(struct qcom_ice *ice) { return > > > > > > > > > > +ice->use_hwkm; } > > > > > > > EXPORT_SYMBOL_GPL(qcom_ice_hwkm_supported); > > > > > > > > > > + > > > > > > > > > > static struct qcom_ice *qcom_ice_create(struct device *dev, > > > > > > > > > > void __iomem *base) > > > > > > > > > > { > > > > > > > > > > @@ -240,6 +383,7 @@ 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 = qcom_scm_has_wrapped_key_support(); > > > > > > > > > > > > > > > > > > This still makes the decision on whether to use HW-wrapped keys > > > > > > > > > on behalf of a user. I suppose this is incorrect. The user must > > > > > > > > > be able to use raw keys even if HW-wrapped keys are available on > > > > > > > > > the platform. One of the examples for such use-cases is if a > > > > > > > > > user prefers to be able to recover stored information in case of > > > > > > > > > a device failure (such recovery will be impossible if SoC is > > > > > > > > > damaged and HW- > > > > > > > wrapped keys are used). > > > > > > > > > > > > > > > > Isn't that already the case ? the > > > > > BLK_CRYPTO_KEY_TYPE_HW_WRAPPED > > > > > > > size > > > > > > > > is here to select HW-wrapped key, otherwise the ol' raw key is passed. > > > > > > > > Just look the next patch. > > > > > > > > > > > > > > > > Or did I miss something ? > > > > > > > > > > > > > > That's a good question. If use_hwkm is set, ICE gets programmed to > > > > > > > use hwkm (see qcom_ice_hwkm_init() call above). I'm not sure if it > > > > > > > is expected to work properly if after such a call we pass raw key. > > > > > > > > > > > > > > > > > > > Once ICE has moved to a HWKM mode, the firmware key programming > > > > > currently does not support raw keys. > > > > > > This support is being added for the next Qualcomm chipset in Trustzone to > > > > > support both at he same time, but that will take another year or two to hit > > > > > the market. > > > > > > Until that time, due to TZ (firmware) limitations , the driver can only > > > > > support one or the other. > > > > > > > > > > > > We also cannot keep moving ICE modes, due to the HWKM enablement > > > > > being a one-time configurable value at boot. > > > > > > > > > > So the init of HWKM should be delayed until the point where the user tells if > > > > > HWKM or raw keys should be used. > > > > > > > > Ack. > > > > I'll work with Bartosz to look into moving to HWKM mode only during the first key program request > > > > > > > > > > That would mean the driver would have to initially advertise support for both > > > HW-wrapped keys and raw keys, and then it would revoke the support for one of > > > them later (due to the other one being used). However, runtime revocation of > > > crypto capabilities is not supported by the blk-crypto framework > > > (Documentation/block/inline-encryption.rst), and there is no clear path to > > > adding such support. Upper layers may have already checked the crypto > > > capabilities and decided to use them. It's too late to find out that the > > > support was revoked in the middle of an I/O request. Upper layer code > > > (blk-crypto, fscrypt, etc.) is not prepared for this. And even if it was, the > > > best it could do is cleanly fail the I/O, which is too late as e.g. it may > > > happen during background writeback and cause user data to be thrown away. > > > > Can we check crypto capabilities when the user sets the key? > > I think you mean when a key is programmed into a keyslot? That happens during > I/O, which is too late as I've explained above. > > > Compare this to the actual HSM used to secure communication or > > storage. It has certain capabilities, which can be enumerated, etc. > > But then at the time the user sets the key it is perfectly normal to > > return an error because HSM is out of resources. It might even have > > spare key slots, but it might be not enough to be able to program the > > required key (as a really crazy example, consider the HSM having at > > this time a single spare DES key slot, while the user wants to program > > 3DES key). > > That isn't how the kernel handles inline encryption keyslots. They are only > programmed as needed for I/O. If they are all in-use by pending I/O requests, > then the kernel waits for an I/O request to finish and reprograms the keyslot it > was using. There is never an error reported due to lack of keyslots. Does that mean that the I/O can be outstanding for the very long period of time? Or that if the ICE hardware has just a single keyslot, but there are two concurrent I/O processes using two different keys, the framework will be constantly swapping the keys programmed to the HW? I think it might be prefereable for the drivers and the framework to support "preprogramming" of the keys, when the key is programmed to the hardware when it is set by the user. Another option might be to let the drivers validate the keys being set by userspace. This way in our case the driver might report that it supports both raw and wrapped keys, but start rejecting the keys once it gets notified that the user has programmed other kind of keys. This way key setup can fail, but the actual I/O can not. WDYT? > If I/O requests could randomly fail at any time when using inline encryption, > then no one would use inline encryption because it would not be reliable. Yes, I agree here. > > > > So, the choice of support for HW-wrapped vs. raw will need to be made ahead of > > > time, rather than being implicitly set by the first use. That is most easily > > > done using a module parameter like qcom_ice.hw_wrapped_keys=1. Yes, it's a bit > > > inconvenient, but there's no realistic way around this currently. > > > > This doesn't work for Android usecase. The user isn't able to setup modparams. > > It does work for Android. The encryption setting that Android uses is > configured in the build of Android for the device (by the OEM, or by whoever > made the build in the case of a custom build). Refer to > https://source.android.com/docs/security/features/encryption/file-based#enabling-file-based-encryption > > Anyone who can change that can also change the kernel command line. Ok. I think if the 'validation' or 'notify' proposal is declined, I'll have to agree to the modparam.
Hi Dmitry, On Fri, Sep 13, 2024 at 03:21:07PM +0300, Dmitry Baryshkov wrote: > > > > > > > Once ICE has moved to a HWKM mode, the firmware key programming > > > > > > currently does not support raw keys. > > > > > > > This support is being added for the next Qualcomm chipset in Trustzone to > > > > > > support both at he same time, but that will take another year or two to hit > > > > > > the market. > > > > > > > Until that time, due to TZ (firmware) limitations , the driver can only > > > > > > support one or the other. > > > > > > > > > > > > > > We also cannot keep moving ICE modes, due to the HWKM enablement > > > > > > being a one-time configurable value at boot. > > > > > > > > > > > > So the init of HWKM should be delayed until the point where the user tells if > > > > > > HWKM or raw keys should be used. > > > > > > > > > > Ack. > > > > > I'll work with Bartosz to look into moving to HWKM mode only during the first key program request > > > > > > > > > > > > > That would mean the driver would have to initially advertise support for both > > > > HW-wrapped keys and raw keys, and then it would revoke the support for one of > > > > them later (due to the other one being used). However, runtime revocation of > > > > crypto capabilities is not supported by the blk-crypto framework > > > > (Documentation/block/inline-encryption.rst), and there is no clear path to > > > > adding such support. Upper layers may have already checked the crypto > > > > capabilities and decided to use them. It's too late to find out that the > > > > support was revoked in the middle of an I/O request. Upper layer code > > > > (blk-crypto, fscrypt, etc.) is not prepared for this. And even if it was, the > > > > best it could do is cleanly fail the I/O, which is too late as e.g. it may > > > > happen during background writeback and cause user data to be thrown away. > > > > > > Can we check crypto capabilities when the user sets the key? > > > > I think you mean when a key is programmed into a keyslot? That happens during > > I/O, which is too late as I've explained above. > > > > > Compare this to the actual HSM used to secure communication or > > > storage. It has certain capabilities, which can be enumerated, etc. > > > But then at the time the user sets the key it is perfectly normal to > > > return an error because HSM is out of resources. It might even have > > > spare key slots, but it might be not enough to be able to program the > > > required key (as a really crazy example, consider the HSM having at > > > this time a single spare DES key slot, while the user wants to program > > > 3DES key). > > > > That isn't how the kernel handles inline encryption keyslots. They are only > > programmed as needed for I/O. If they are all in-use by pending I/O requests, > > then the kernel waits for an I/O request to finish and reprograms the keyslot it > > was using. There is never an error reported due to lack of keyslots. > > Does that mean that the I/O can be outstanding for the very long period > of time? Or that if the ICE hardware has just a single keyslot, but > there are two concurrent I/O processes using two different keys, the > framework will be constantly swapping the keys programmed to the HW? Yes for both. Of course, system designers are supposed to put in enough keyslots for this to not be much of a problem. So, the "wait for a keyslot" logic in the block layer is necessary in general so that applications don't unnecessarily get I/O errors. But in a properly tuned system this logic should be rarely executed. And in cases where the keyslots really are a bottleneck, users can of course just use software encryption instead. Note that the number of keyslots is reported in sysfs. > I think it might be prefereable for the drivers and the framework to > support "preprogramming" of the keys, when the key is programmed to the > hardware when it is set by the user. This doesn't sound particularly useful. If there are always enough keyslots, then keyslots never get evicted and there is no advantage to this. If there are *not* always enough keyslots, then it's sometimes necessary to evict keyslots, so it would not be desirable to have them permanently reserved. It could make sense to have some sort of hints mechanism, where frequently-used keys can be marked as high-priority to keep programmed in a keyslot. I don't see much of a need for this though, given that the eviction policy is already LRU, so it already prefers to keep frequently-used keys in a keyslot. > Another option might be to let the drivers validate the keys being set > by userspace. This way in our case the driver might report that it > supports both raw and wrapped keys, but start rejecting the keys once > it gets notified that the user has programmed other kind of keys. This > way key setup can fail, but the actual I/O can not. WDYT? Well, that has the same effect as the crypto capabilities check which is already done. The problem is that your proposal effectively revokes a capability, and that is racy. - Eric
On Sat, Sep 21, 2024 at 12:49:39PM GMT, Eric Biggers wrote: > Hi Dmitry, > > On Fri, Sep 13, 2024 at 03:21:07PM +0300, Dmitry Baryshkov wrote: > > > > > > > > Once ICE has moved to a HWKM mode, the firmware key programming > > > > > > > currently does not support raw keys. > > > > > > > > This support is being added for the next Qualcomm chipset in Trustzone to > > > > > > > support both at he same time, but that will take another year or two to hit > > > > > > > the market. > > > > > > > > Until that time, due to TZ (firmware) limitations , the driver can only > > > > > > > support one or the other. > > > > > > > > > > > > > > > > We also cannot keep moving ICE modes, due to the HWKM enablement > > > > > > > being a one-time configurable value at boot. > > > > > > > > > > > > > > So the init of HWKM should be delayed until the point where the user tells if > > > > > > > HWKM or raw keys should be used. > > > > > > > > > > > > Ack. > > > > > > I'll work with Bartosz to look into moving to HWKM mode only during the first key program request > > > > > > > > > > > > > > > > That would mean the driver would have to initially advertise support for both > > > > > HW-wrapped keys and raw keys, and then it would revoke the support for one of > > > > > them later (due to the other one being used). However, runtime revocation of > > > > > crypto capabilities is not supported by the blk-crypto framework > > > > > (Documentation/block/inline-encryption.rst), and there is no clear path to > > > > > adding such support. Upper layers may have already checked the crypto > > > > > capabilities and decided to use them. It's too late to find out that the > > > > > support was revoked in the middle of an I/O request. Upper layer code > > > > > (blk-crypto, fscrypt, etc.) is not prepared for this. And even if it was, the > > > > > best it could do is cleanly fail the I/O, which is too late as e.g. it may > > > > > happen during background writeback and cause user data to be thrown away. > > > > > > > > Can we check crypto capabilities when the user sets the key? > > > > > > I think you mean when a key is programmed into a keyslot? That happens during > > > I/O, which is too late as I've explained above. > > > > > > > Compare this to the actual HSM used to secure communication or > > > > storage. It has certain capabilities, which can be enumerated, etc. > > > > But then at the time the user sets the key it is perfectly normal to > > > > return an error because HSM is out of resources. It might even have > > > > spare key slots, but it might be not enough to be able to program the > > > > required key (as a really crazy example, consider the HSM having at > > > > this time a single spare DES key slot, while the user wants to program > > > > 3DES key). > > > > > > That isn't how the kernel handles inline encryption keyslots. They are only > > > programmed as needed for I/O. If they are all in-use by pending I/O requests, > > > then the kernel waits for an I/O request to finish and reprograms the keyslot it > > > was using. There is never an error reported due to lack of keyslots. > > > > Does that mean that the I/O can be outstanding for the very long period > > of time? Or that if the ICE hardware has just a single keyslot, but > > there are two concurrent I/O processes using two different keys, the > > framework will be constantly swapping the keys programmed to the HW? > > Yes for both. Of course, system designers are supposed to put in enough > keyslots for this to not be much of a problem. > > So, the "wait for a keyslot" logic in the block layer is necessary in general so > that applications don't unnecessarily get I/O errors. But in a properly tuned > system this logic should be rarely executed. > > And in cases where the keyslots really are a bottleneck, users can of course > just use software encryption instead. > > Note that the number of keyslots is reported in sysfs. > > > I think it might be prefereable for the drivers and the framework to > > support "preprogramming" of the keys, when the key is programmed to the > > hardware when it is set by the user. > > This doesn't sound particularly useful. If there are always enough keyslots, > then keyslots never get evicted and there is no advantage to this. If there are > *not* always enough keyslots, then it's sometimes necessary to evict keyslots, > so it would not be desirable to have them permanently reserved. I'm still trying to propose solutions for the hwkm-or-raw keys problem, trying to find a way to return an error early enough. So it's not about the hints for frequently-used keys, but for returning an error if the user tries to program key type which became unusupported after a previous call. > It could make sense to have some sort of hints mechanism, where frequently-used > keys can be marked as high-priority to keep programmed in a keyslot. I don't > see much of a need for this though, given that the eviction policy is already > LRU, so it already prefers to keep frequently-used keys in a keyslot. > > > Another option might be to let the drivers validate the keys being set > > by userspace. This way in our case the driver might report that it > > supports both raw and wrapped keys, but start rejecting the keys once > > it gets notified that the user has programmed other kind of keys. This > > way key setup can fail, but the actual I/O can not. WDYT? > > Well, that has the same effect as the crypto capabilities check which is already > done. The problem is that your proposal effectively revokes a capability, and > that is racy. > > - Eric
diff --git a/drivers/soc/qcom/ice.c b/drivers/soc/qcom/ice.c index 4393262a1bf2..667d993694ac 100644 --- a/drivers/soc/qcom/ice.c +++ b/drivers/soc/qcom/ice.c @@ -27,6 +27,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) @@ -35,6 +69,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)) @@ -47,6 +84,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) @@ -64,8 +104,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); @@ -114,27 +167,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. + * + * 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 (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); @@ -150,6 +282,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); @@ -157,6 +293,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; } @@ -206,6 +343,12 @@ int qcom_ice_evict_key(struct qcom_ice *ice, int slot) } EXPORT_SYMBOL_GPL(qcom_ice_evict_key); +bool qcom_ice_hwkm_supported(struct qcom_ice *ice) +{ + return ice->use_hwkm; +} +EXPORT_SYMBOL_GPL(qcom_ice_hwkm_supported); + static struct qcom_ice *qcom_ice_create(struct device *dev, void __iomem *base) { @@ -240,6 +383,7 @@ 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 = qcom_scm_has_wrapped_key_support(); 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__ */