Re: [PATCH v5 04/15] soc: qcom: ice: add hwkm support in ice

["neil.armstrong@linaro.org" <neil.armstrong@linaro.org>]

On 25/06/2024 06:58, Gaurav Kashyap (QUIC) wrote:
> 
> Hey Eric
> 
> On 06/21/2024, 9:02 AM PDT, Eric Biggers wrote:
>> On Fri, Jun 21, 2024 at 03:35:40PM +0000, Gaurav Kashyap wrote:
>>> Hello Eric
>>>
>>> On 06/20/2024, 9:48 PM PDT, Eric Biggers wrote:
>>>> On Thu, Jun 20, 2024 at 02:57:40PM +0300, Dmitry Baryshkov wrote:
>>>>>>>>
>>>>>>>>> Is it possible to use both kind of keys when working on
>>>>>>>>> standard
>>>> mode?
>>>>>>>>> If not, it should be the user who selects what type of
>>>>>>>>> keys to be
>>>> used.
>>>>>>>>> Enforcing this via DT is not a way to go.
>>>>>>>>>
>>>>>>>>
>>>>>>>> Unfortunately, that support is not there yet. When you say
>>>>>>>> user, do you mean to have it as a filesystem mount option?
>>>>>>>
>>>>>>> During cryptsetup time. When running e.g. cryptsetup I, as a
>>>>>>> user, would like to be able to use either a hardware-wrapped
>>>>>>> key or a
>>>> standard key.
>>>>>>>
>>>>>>
>>>>>> What we are looking for with these patches is for
>>>>>> per-file/folder
>>>> encryption using fscrypt policies.
>>>>>> Cryptsetup to my understanding supports only full-disk , and
>>>>>> does not support FBE (File-Based)
>>>>>
>>>>> I must admit, I mostly used dm-crypt beforehand, so I had to look
>>>>> at fscrypt now. Some of my previous comments might not be fully
>>>>> applicable.
>>>>>
>>>>>> Hence the idea here is that we mount an unencrypted device (with
>>>>>> the inlinecrypt option that indicates inline encryption is
>>>>>> supported) And
>>>> specify policies (links to keys) for different folders.
>>>>>>
>>>>>>>> The way the UFS/EMMC crypto layer is designed currently is
>>>>>>>> that, this information is needed when the modules are loaded.
>>>>>>>>
>>>>>>>> https://lore.kernel.org/all/20231104211259.17448-2-ebiggers@
>>>>>>>> kern el.org /#Z31drivers:ufs:core:ufshcd-crypto.c
>>>>>>>
>>>>>>> I see that the driver lists capabilities here. E.g. that it
>>>>>>> supports HW-wrapped keys. But the line doesn't specify that
>>>>>>> standard
>>>> keys are not supported.
>>>>>>>
>>>>>>
>>>>>> Those are capabilities that are read from the storage controller.
>>>>>> However, wrapped keys Are not a standard in the ICE JEDEC
>>>>>> specification, and in most cases, is a value add coming from the SoC.
>>>>>>
>>>>>> QCOM SOC and firmware currently does not support both kinds of
>>>>>> keys in
>>>> the HWKM mode.
>>>>>> That is something we are internally working on, but not available yet.
>>>>>
>>>>> I'd say this is a significant obstacle, at least from my point of
>>>>> view. I understand that the default might be to use hw-wrapped
>>>>> keys, but it should be possible for the user to select non-HW keys
>>>>> if the ability to recover the data is considered to be important.
>>>>> Note, I'm really pointing to the user here, not to the system
>>>>> integrator. So using DT property or specifying kernel arguments to
>>>>> switch between these modes is not really an option.
>>>>>
>>>>> But I'd really love to hear some feedback from linux-security
>>>>> and/or linux-fscrypt here.
>>>>>
>>>>> In my humble opinion the user should be able to specify that the
>>>>> key is wrapped using the hardware KMK. Then if the hardware has
>>>>> already started using the other kind of keys, it should be able to
>>>>> respond with -EINVAL / whatever else. Then the user can evict
>>>>> previously programmed key and program a desired one.
>>>>>
>>>>>>> Also, I'd have expected that hw-wrapped keys are handled using
>>>>>>> trusted keys mechanism (see security/keys/trusted-keys/).
>>>>>>> Could you please point out why that's not the case?
>>>>>>>
>>>>>>
>>>>>> I will evaluate this.
>>>>>> But my initial response is that we currently cannot communicate
>>>>>> to our TPM directly from HLOS, but goes through QTEE, and I
>>>>>> don't think our qtee currently interfaces with the open source
>>>>>> tee driver. The
>>>> interface is through QCOM SCM driver.
>>>>>
>>>>> Note, this is just an API interface, see how it is implemented for
>>>>> the CAAM hardware.
>>>>>
>>>>
>>>> The problem is that this patchset was sent out without the patches
>>>> that add the block and filesystem-level framework for
>>>> hardware-wrapped inline encryption keys, which it depends on.  So
>>>> it's lacking context.  The proposed framework can be found at
>>>> https://lore.kernel.org/linux-
>>>> block/20231104211259.17448-1-ebiggers@kernel.org/T/#u
>>>>
>>>
>>> I have only been adding the fscryp patch link as part of the cover letter - as
>> a dependency.
>>> https://lore.kernel.org/all/20240617005825.1443206-1-quic_gaurkash@qui
>>> cinc.com/ If you would like me to include it in the patch series
>>> itself, I can do that as well.
>>>
>>
>> I think including all prerequisite patches would be helpful for reviewers.
> 
> Noted. I'll do that for the next patch.
> 
>>
>> Thanks for continuing to work on this!
>>
>> I still need to get ahold of a sm8650 based device and test this out.  Is the
>> SM8650 HDK the only option, or is there a sm8650 based phone with
>> upstream support yet?

Yes you should be able to buy the SM8650 HDK from lantronix:
https://www.lantronix.com/products/snapdragon-8-gen-3-mobile-hardware-development-kit/

It should be supported in v6.11

Neil

> 
> There are some devices released with SM8650 (Snapdragon 8 Gen 3). Sorry, I have
> not kept track of which. I know the S24s were released with that. But there should be
> more in the market.
> 
>>
>> - Eric