Message-ID: <mwc3zbfst4pnbmxcdmdkhdqgsbrv3vdz5faqc4viifjwk6olfd@gc4pga5huqlv>
Date: Fri, 21 Jun 2024 23:52:01 +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, Jun 21, 2024 at 08:14:41PM GMT, Eric Biggers wrote:
> On Fri, Jun 21, 2024 at 10:24:07PM +0300, Dmitry Baryshkov wrote:
> > >
> > > (fscrypt used to use the keyring service a bit more: it looked up a key whenever
> > > a file was opened, and it supported evicting per-file keys by revoking the
> > > corresponding keyring key.  But this turned out to be totally broken.  E.g., it
> > > didn't provide the correct semantics for filesystem encryption where the key
> > > should either be present or absent filesystem-wide.)
> > >
> > > We do need the ability to create HW-wrapped keys in long-term wrapped form,
> > > either via "generate" or "import", return those long-term wrapped keys to
> > > userspace so that they can be stored on-disk, and convert them into
> > > ephemerally-wrapped form so they can be used.  It probably would be possible to
> > > support all of this through the keyrings service, but it would need a couple new
> > > key types:
> > >
> > > - One key type that can be instantiated with a raw key (or NULL to request
> > >   generation of a key) and that automagically creates a long-term wrapped key
> > >   and supports userspace reading it back.  This would be vaguely similar to
> > >   "trusted", but without any support for using the key directly.
> > >
> > > - One key type that can be instantiated using a long-term wrapped key which gets
> > >   automagically converted to an ephemerally-wrapped key.  This would be what is
> > >   passed to other kernel subsystems.  Functions specific to this key type would
> > >   need to be provided for users to use.
> > 
> > I think having one key type should be enough. The userspace loads /
> > generates&reads / wraps and reads back the 'exported' version wrapped
> > using the platform-specific key. In kernel the key is unsealed and
> > represented as binary key to be loaded to the hardware + a cookie for
> > the ephemeral key and device that have been used to wrap it. When
> > userspace asks the device to program the key, the cookie is verified
> > to match the device / ephemeral key and then the binary is programmed
> > to the hardware. Maybe it's enough to use the struct device as a
> > cookie.
> 
> The long-term wrapped key has to be wiped from memory as soon as it's no longer
> needed.  So it's hard to see how overloading a key type in this way can work, as
> the kernel can't know if userspace intends to read back the long-term wrapped
> key or not.

Why? It should be user's decision. Pretty much in the same way as it's
done for all other keys.

> > > I think it would be possible, but it feels like a bit of a shoehorned API.  The
> > > ioctls are a more straightforward solution.
> > 
> > Are we going to have another set of IOCTLs for loading the encrypted
> > keys? keys sealed by TPM?
> 
> Those features aren't compatible with hardware-wrapped inline encryption keys,
> so they're not really relevant here.  BLKCRYPTOIMPORTKEY could support importing
> a keyring service key as an alternative to a raw key, of course.  But this would
> just work similarly to fscrypt and dm-crypt where they just extract the payload,
> and the keyring service key plays no further role.

Yes, extracting the payload is fine. As you wrote, dm-crypt and fscrypt
already do it in this way. But what I really don't like here is the idea
of having two different kinds of API having pretty close functionality.
In my opinion, all the keys should be handled via the existing keyrings
API and then imported via the BLKCRYPTOIMPORTKEY IOCTL. This way all
kinds of keys are handled in a similar way from user's point of view.

> > > > > 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.
> > >
> > > If the driver has to make a decision about which type of keys to support (due to
> > > the hardware and firmware supporting both but not at the same time), I think
> > > this will need to be done via a module parameter, e.g.
> > > qcom_ice.hw_wrapped_keys=1 to support HW-wrapped keys instead of standard keys.
> > 
> > No, the user can not set modparams on  e.g. Android device. In my
> > opinion it should be first-come-first-serve. If the user wants
> > hw-wrapped keys (and the platform is fine with that), then further
> > attempts to use raw keys should fail. If the user loads a raw key,
> > further attempts to set hw-wrapped key should fail (maybe until the
> > last raw key has been evicted from the hw, if such thing is actually
> > supported).
> 
> That's not going to work.  Upper layers need to know what the crypto
> capabilities are before they decide to use them.  We can't randomly revoke
> capabilities based on who happened to get there first, as a user might have
> already checked the capabilities.  Yes, the module parameter is a litle
> annoying, but it seems to be necessary here.

Hmm. This is typical to have resource-limited capabilities. So yes, the
user checks the capabilities to identify whether the key type is
supported at all. But then _using_ the key might fail. For example
because all the hardware resources that are used by this key type are
already taken.

> It is not a problem for Android
> because the type of encryption an Android device uses is set by the build
> anyway, which makes it no easier to change than module parameters.

If AOSP misbehaves, it doesn't mean that we should follow the pattern.

-- 
With best wishes
Dmitry

Powered by blists - more mailing lists