Message-ID: <CAA8EJpqytynwQrCAqqBsmx2XYgV5tsNeV4hpYzT6snqu+r8Wdg@mail.gmail.com>
Date: Fri, 21 Jun 2024 20:49:56 +0300
From: Dmitry Baryshkov <dmitry.baryshkov@...aro.org>
To: Eric Biggers <ebiggers@...nel.org>
Cc: "Gaurav Kashyap (QUIC)" <quic_gaurkash@...cinc.com>, 
	"linux-arm-msm@...r.kernel.org" <linux-arm-msm@...r.kernel.org>, 
	"linux-scsi@...r.kernel.org" <linux-scsi@...r.kernel.org>, "andersson@...nel.org" <andersson@...nel.org>, 
	"neil.armstrong@...aro.org" <neil.armstrong@...aro.org>, 
	"srinivas.kandagatla" <srinivas.kandagatla@...aro.org>, 
	"krzysztof.kozlowski+dt@...aro.org" <krzysztof.kozlowski+dt@...aro.org>, 
	"conor+dt@...nel.org" <conor+dt@...nel.org>, "robh+dt@...nel.org" <robh+dt@...nel.org>, 
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>, 
	"linux-mmc@...r.kernel.org" <linux-mmc@...r.kernel.org>, kernel <kernel@...cinc.com>, 
	"linux-crypto@...r.kernel.org" <linux-crypto@...r.kernel.org>, 
	"devicetree@...r.kernel.org" <devicetree@...r.kernel.org>, 
	"Om Prakash Singh (QUIC)" <quic_omprsing@...cinc.com>, 
	"Bao D. Nguyen (QUIC)" <quic_nguyenb@...cinc.com>, 
	"bartosz.golaszewski" <bartosz.golaszewski@...aro.org>, 
	"konrad.dybcio@...aro.org" <konrad.dybcio@...aro.org>, 
	"ulf.hansson@...aro.org" <ulf.hansson@...aro.org>, "jejb@...ux.ibm.com" <jejb@...ux.ibm.com>, 
	"martin.petersen@...cle.com" <martin.petersen@...cle.com>, "mani@...nel.org" <mani@...nel.org>, 
	"davem@...emloft.net" <davem@...emloft.net>, 
	"herbert@...dor.apana.org.au" <herbert@...dor.apana.org.au>, Prasad Sodagudi <psodagud@...cinc.com>, 
	Sonal Gupta <sonalg@...cinc.com>
Subject: Re: [PATCH v5 04/15] soc: qcom: ice: add hwkm support in ice

On Fri, 21 Jun 2024 at 19:31, Eric Biggers <ebiggers@...nel.org> wrote:
>
> On Fri, Jun 21, 2024 at 07:06:25PM +0300, Dmitry Baryshkov wrote:
> > On Fri, 21 Jun 2024 at 18:39, Eric Biggers <ebiggers@...nel.org> wrote:
> > >
> > > On Fri, Jun 21, 2024 at 06:16:37PM +0300, Dmitry Baryshkov wrote:
> > > > On Fri, 21 Jun 2024 at 07:47, Eric Biggers <ebiggers@...nel.org> 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@kernel.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
> > > >
> > > > Thank you. I have quickly skimmed through the patches, but I didn't
> > > > review them thoroughly. Maybe the patchset already implements the
> > > > interfaces that I'm thinking about. In such a case please excuse me. I
> > > > will give it a more thorough look later today.
> > > >
> > > > > As for why "trusted keys" aren't used, they just aren't helpful here.  "Trusted
> > > > > keys" are based around a model where the kernel can request that keys be sealed
> > > > > and unsealed using a trust source, and the kernel gets access to the raw
> > > > > unsealed keys.  Hardware-wrapped inline encryption keys use a different model
> > > > > where the kernel never gets access to the raw keys.  They also have the concept
> > > > > of ephemeral wrapping which does not exist in "trusted keys".  And they need to
> > > > > be properly integrated with the inline encryption framework in the block layer.
> > > >
> > > > Then what exactly does qcom_scm_derive_sw_secret() do? Does it rewrap
> > > > the key under some other key?
> > >
> > > It derives a secret for functionality such as filenames encryption that can't
> > > use inline encryption.
> > >
> > > > I had the feeling that there are two separate pieces of functionality
> > > > being stuffed into a single patchset and into a single solution.
> > > >
> > > > First one is handling the keys. I keep on thinking that there should
> > > > be a separate software interface to unseal the key and rewrap it under
> > > > an ephemeral key.
> > >
> > > There is.  That's what the BLKCRYPTOPREPAREKEY ioctl is for.
> > >
> > > > Some hardware might permit importing raw keys.
> > >
> > > That's what BLKCRYPTOIMPORTKEY is for.
> > >
> > > > Other hardware might insist on generating the keys on-chip so that raw keys
> > > > can never be used.
> > >
> > > And that's what BLKCRYPTOGENERATEKEY is for.
> >
> > Again, this might be answered somewhere, but why can't we use keyctl
> > for handling the keys and then use a single IOCTL to point the block
> > device to the key in the keyring?
>
> All the same functionality would need to be supported, and I think that
> shoehorning it into the keyrings service instead of just adding new ioctls would
> be more difficult.  The keyrings service was not designed for this use case.
> We've already had a lot of problems trying to take advantage of the keyrings
> service in fscrypt previously.  The keyrings service is something that sounds
> useful but really isn't all that useful.

I would be really interested in reading or listening to any kind of
summary or parts of the issues.
I'm slightly pushy towards keyctl / keyrings, because it already
provides support for different kinds of key wrapping and key
management. Encrypted keys, trusted keys - those are all kinds of key
management, which either will be missing or will have to be
reimplemented for block layers.

I know that keyrings are clumsy and not that logical, but then their
API needs to be improved. Just ignoring the existing mechanisms sounds
like a bad idea.

>
> By "a single IOCTL to point the block device to the key in the keyring", you
> seem to be referring to configuring full block device encryption with a single
> key.  That's not something that's supported by the upstream kernel yet, and it's
> not related to this patchset; currently only fscrypt supports inline encryption.

I see that dm has at least some provisioning and hooks for
CONFIG_BLK_INLINE_ENCRYPTION. Thus I thought that it's possible to use
inline encryption through DM.

> Support for it will be added at some point, which will likely indeed take the
> form of an ioctl to set a key on a block device.  But that would be the case
> even without HW-wrapped keys.  And *requiring* the key to be given in a keyring
> (instead of just in a byte array passed to the ioctl) isn't very helpful, as it
> just makes the API harder to use.  We've learned this from the fscrypt API
> already where we actually had to move away from the keyrings service in order to
> fix all the issues caused by it (see FS_IOC_ADD_ENCRYPTION_KEY).
>
> > >
> > > > Second part is the actual block interface. Gaurav wrote about
> > > > targeting fscrypt, but there should be no actual difference between
> > > > crypto targets. FDE or having a single partition encrypted should
> > > > probably work in the same way. Convert the key into blk_crypto_key
> > > > (including the cookie for the ephemeral key), program the key into the
> > > > slot, use the slot to en/decrypt hardware blocks.
> > > >
> > > > My main point is that the decision on the key type should be coming
> > > > from the user.
> > >
> > > That's exactly how it works.  There is a block interface for specifying an
> > > inline encryption key along with each bio.  The submitter of the bio can specify
> > > either a standard key or a HW-wrapped key.
> >
> > Not in this patchset. The ICE driver decides whether it can support
> > HW-wrapped keys or not and then fails to support other type of keys.
> >
>
> Sure, that's just a matter of hardware capabilities though, right?  The block
> layer provides a way for drivers to declare which inline encryption capabilities
> they support.  They can declare they support standard keys, HW-wrapped keys,
> both, or neither.  If Qualcomm SoCs can't support both types of keys at the same
> time, that's unfortunate, but I'm not sure what your poitnt is.  The user (e.g.
> fscrypt) still has control over whether they use the functionality that the
> hardware provides.

It's a matter of policy. Harware / firmware doesn't support using both
kinds of keys concurrently, if I understood Gaurav's explanations
correctly. But the user should be able to make a judgement and use
non-hw-wrapped keys if it fits their requirements. The driver should
not make this kind of judgement. Note, this is not an issue of your
original patchset, but it's a driver flaw in this patchset.

--
With best wishes
Dmitry

Powered by blists - more mailing lists