On 13/09/2024 01:17, Eric Biggers wrote:

On Thu, Sep 12, 2024 at 10:17:03PM +0000, Gaurav Kashyap (QUIC) wrote:

quoted

On Monday, September 9, 2024 11:29 PM PDT, Dmitry Baryshkov wrote:

quoted

On Tue, 10 Sept 2024 at 03:51, Gaurav Kashyap (QUIC)
[off-list ref] wrote:

quoted

Hello Dmitry and Neil

On Monday, September 9, 2024 2:44 AM PDT, Dmitry Baryshkov wrote:

quoted

On Mon, Sep 09, 2024 at 10:58:30AM GMT, Neil Armstrong wrote:

quoted

On 07/09/2024 00:07, Dmitry Baryshkov wrote:

quoted

On Fri, Sep 06, 2024 at 08:07:12PM GMT, Bartosz Golaszewski wrote:

quoted

From: Gaurav Kashyap <redacted>

Qualcomm's ICE (Inline Crypto Engine) contains a proprietary
key management hardware called Hardware Key Manager (HWKM).
Add

HWKM

quoted

quoted

quoted

support to the ICE driver if it is available on the platform.
HWKM primarily provides hardware wrapped key support where

the

quoted

quoted

quoted

quoted

quoted

ICE
(storage) keys are not available in software and instead
protected in

hardware.

quoted

quoted

quoted

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.

quoted

quoted

quoted

Tested-by: Neil Armstrong <neil.armstrong@linaro.org>
Signed-off-by: Gaurav Kashyap <redacted>
Signed-off-by: Bartosz Golaszewski
[off-list ref]
---
   drivers/soc/qcom/ice.c | 152

+++++++++++++++++++++++++++++++++++++++++++++++--

quoted

quoted

quoted

   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)

quoted

quoted

quoted

quoted

quoted

                   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)

quoted

quoted

quoted

   }
   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);

quoted

quoted

quoted

+
   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,

quoted

quoted

quoted

                   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).

quoted

Isn't that already the case ? the

BLK_CRYPTO_KEY_TYPE_HW_WRAPPED

quoted

quoted

size

quoted

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.

quoted

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.

quoted

Until that time, due to TZ (firmware) limitations , the driver can only

support one or the other.

quoted

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