| Message ID | 20240906-wrapped-keys-v6-0-d59e61bc0cb4@linaro.org |
|---|---|
| Headers | show |
| Series | Hardware wrapped key support for QCom ICE and UFS core | expand |
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 > > > > > >
On 6.09.2024 8:07 PM, Bartosz Golaszewski wrote: > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> > > Add a helper that allows users to check if wrapped key support is > available on the platform by checking if the SCM call allowing to > derive the software secret from a wrapped key is enabled. > > Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> > --- I dearly hope that all firmwares that advertise this call, also advertise the other necessary ones Reviewed-by: Konrad Dybcio <konradybcio@kernel.org> Konrad
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
I took this work over from Gaurav Kashyap and integrated Eric's series into it for an easier discussion on the actual API to be used for wrapped keys as well as if and how to enable users to indicate whether wrapped keys should be used at all. I know Dmitry's opinion on that and expect this to be more of an RFC rather than a real patch series. That being said, what is here, works fine on sm8650. Hardware-wrapped keys are encrypted keys that can only be unwrapped (decrypted) and used by hardware - either by the inline encryption hardware itself, or by a dedicated hardware block that can directly provision keys to the inline encryption hardware. For more details, please see patches 1-3 in this series which extend the inline encryption docs with more information. This series adds support for wrapped keys to the block layer, fscrypt and then build upwards from there by implementing relevant callbacks in QCom SCM driver, then the ICE driver and finally in UFS core and QCom layer. Tested on sm8650-qrd. How to test: Use the wip-wrapped-keys branch from https://github.com/ebiggers/fscryptctl to build a custom fscryptctl that supports generating wrapped keys. Enable the following config options: CONFIG_BLK_INLINE_ENCRYPTION=y CONFIG_QCOM_INLINE_CRYPTO_ENGINE=m CONFIG_FS_ENCRYPTION_INLINE_CRYPT=y CONFIG_SCSI_UFS_CRYPTO=y $ mkfs.ext4 -F -O encrypt,stable_inodes /dev/disk/by-partlabel/userdata $ mount /dev/disk/by-partlabel/userdata -o inlinecrypt /mnt $ fscryptctl generate_hw_wrapped_key /dev/disk/by-partlabel/userdata > /mnt/key.longterm $ fscryptctl prepare_hw_wrapped_key /dev/disk/by-partlabel/userdata < /mnt/key.longterm > /tmp/key.ephemeral $ KEYID=$(fscryptctl add_key --hw-wrapped-key < /tmp/key.ephemeral /mnt) $ rm -rf /mnt/dir $ mkdir /mnt/dir $ fscryptctl set_policy --hw-wrapped-key --iv-ino-lblk-64 "$KEYID" /mnt/dir $ dmesg > /mnt/dir/test.txt $ sync Reboot the board $ mount /dev/disk/by-partlabel/userdata -o inlinecrypt /mnt $ ls /mnt/dir $ fscryptctl prepare_hw_wrapped_key /dev/disk/by-partlabel/userdata < /mnt/key.longterm > /tmp/key.ephemeral $ KEYID=$(fscryptctl add_key --hw-wrapped-key < /tmp/key.ephemeral /mnt) $ fscryptctl set_policy --hw-wrapped-key --iv-ino-lblk-64 "$KEYID" /mnt/dir $ cat /mnt/dir/test.txt # File should now be decrypted Changes since v5: - add the wrapped key support from Eric Biggers to the series - remove the new DT property from the series and instead query the at run-time rustZone to find out if wrapped keys are supported - make the wrapped key support into a UFS capability, not a quirk - improve kerneldocs - improve and rework coding style in most patches - improve and reformat commit messages - simplify the offset calculation for CRYPTOCFG - split out the DTS changes into a separate series Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> --- Bartosz Golaszewski (1): firmware: qcom: scm: add a call for checking wrapped key support Eric Biggers (4): blk-crypto: add basic hardware-wrapped key support blk-crypto: show supported key types in sysfs blk-crypto: add ioctls to create and prepare hardware-wrapped keys fscrypt: add support for hardware-wrapped keys Gaurav Kashyap (12): ice, ufs, mmc: use the blk_crypto_key struct when programming the key firmware: qcom: scm: add a call for deriving the software secret firmware: qcom: scm: add calls for creating, preparing and importing keys soc: qcom: ice: add HWKM support to the ICE driver soc: qcom: ice: add support for hardware wrapped keys soc: qcom: ice: add support for generating, importing and preparing keys ufs: core: add support for wrapped keys to UFS core ufs: core: add support for deriving the software secret ufs: core: add support for generating, importing and preparing keys ufs: host: add support for wrapped keys in QCom UFS ufs: host: add a callback for deriving software secrets and use it ufs: host: add support for generating, importing and preparing wrapped keys Documentation/ABI/stable/sysfs-block | 18 ++ Documentation/block/inline-encryption.rst | 245 +++++++++++++- Documentation/filesystems/fscrypt.rst | 154 ++++++++- Documentation/userspace-api/ioctl/ioctl-number.rst | 2 + block/blk-crypto-fallback.c | 5 +- block/blk-crypto-internal.h | 10 + block/blk-crypto-profile.c | 103 ++++++ block/blk-crypto-sysfs.c | 35 ++ block/blk-crypto.c | 194 ++++++++++- block/ioctl.c | 5 + drivers/firmware/qcom/qcom_scm.c | 233 ++++++++++++++ drivers/firmware/qcom/qcom_scm.h | 4 + drivers/md/dm-table.c | 1 + drivers/mmc/host/cqhci-crypto.c | 9 +- drivers/mmc/host/cqhci.h | 2 + drivers/mmc/host/sdhci-msm.c | 6 +- drivers/soc/qcom/ice.c | 355 ++++++++++++++++++++- drivers/ufs/core/ufshcd-crypto.c | 86 ++++- drivers/ufs/host/ufs-qcom.c | 61 +++- fs/crypto/fscrypt_private.h | 71 ++++- fs/crypto/hkdf.c | 4 +- fs/crypto/inline_crypt.c | 44 ++- fs/crypto/keyring.c | 124 +++++-- fs/crypto/keysetup.c | 54 +++- fs/crypto/keysetup_v1.c | 5 +- fs/crypto/policy.c | 11 +- include/linux/blk-crypto-profile.h | 73 +++++ include/linux/blk-crypto.h | 75 ++++- include/linux/firmware/qcom/qcom_scm.h | 8 + include/soc/qcom/ice.h | 18 +- include/uapi/linux/blk-crypto.h | 44 +++ include/uapi/linux/fs.h | 6 +- include/uapi/linux/fscrypt.h | 7 +- include/ufs/ufshcd.h | 21 ++ 34 files changed, 1958 insertions(+), 135 deletions(-) --- base-commit: ad40aff1edffeccc412cde93894196dca7bc739e change-id: 20240802-wrapped-keys-eea0032fbfed Best regards,