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

[Eric Biggers <ebiggers@kernel.org>]

On Fri, Jun 21, 2024 at 11:52:01PM +0300, Dmitry Baryshkov wrote:
> 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.

Sorry, I don't understand what your point is supposed to be here.  It's
certainly not okay to leave the long-term wrapped key in memory, since that
destroys the security properties of hardware-wrapped keys.  So we need to
provide an API that makes it possible for the long-term wrapped key to be
zeroized.  The API you're proposing, as I understand it, wouldn't allow for that
because the long-term wrapped key would remain in memory as long as the keyring
service key exists, even when only the ephemerally-wrapped key is needed.

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

But in that case all the proposed new BLKCRYPTO* ioctls are still needed.  Your
suggestion would just make them harder to use by requiring users to copy their
key into a keyrings service key instead of just providing it directly in the
ioctl.

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

That mustn't happen here, since finding out in the middle of an I/O request that
inline encryption isn't supported is too late.  That's what the crypto
capabilities in struct blk_crypto_profile are for -- to allow users to check
what is supported before trying to use it.

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

It's not "misbehaving" -- it's just an example of a system that configures the
encryption centrally, which is common.  (And the reason I brought up that the
module parameter works for Android is because you claimed it wouldn't.)

Again, needing a module parameter is unfortunate but I don't see any realistic
way around it for these Qualcomm SoCs.

- Eric