Re: [PATCH v6 09/17] soc: qcom: ice: add HWKM support to the ICE driver

[Neil Armstrong <neil.armstrong@linaro.org>]

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