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

[Eric Biggers <ebiggers@kernel.org>]

Hi Dmitry,

On Fri, Sep 13, 2024 at 03:21:07PM +0300, Dmitry Baryshkov wrote:
> > > > > > > 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.
> > > 
> > > Can we check crypto capabilities when the user sets the key?
> > 
> > I think you mean when a key is programmed into a keyslot?  That happens during
> > I/O, which is too late as I've explained above.
> > 
> > > Compare this to the actual HSM used to secure communication or
> > > storage. It has certain capabilities, which can be enumerated, etc.
> > > But then at the time the user sets the key it is perfectly normal to
> > > return an error because HSM is out of resources. It might even have
> > > spare key slots, but it might be not enough to be able to program the
> > > required key (as a really crazy example, consider the HSM having at
> > > this time a single spare DES key slot, while the user wants to program
> > > 3DES key).
> > 
> > That isn't how the kernel handles inline encryption keyslots.  They are only
> > programmed as needed for I/O.  If they are all in-use by pending I/O requests,
> > then the kernel waits for an I/O request to finish and reprograms the keyslot it
> > was using.  There is never an error reported due to lack of keyslots.
> 
> Does that mean that the I/O can be outstanding for the very long period
> of time? Or that if the ICE hardware has just a single keyslot, but
> there are two concurrent I/O processes using two different keys, the
> framework will be constantly swapping the keys programmed to the HW?

Yes for both.  Of course, system designers are supposed to put in enough
keyslots for this to not be much of a problem.

So, the "wait for a keyslot" logic in the block layer is necessary in general so
that applications don't unnecessarily get I/O errors.  But in a properly tuned
system this logic should be rarely executed.

And in cases where the keyslots really are a bottleneck, users can of course
just use software encryption instead.

Note that the number of keyslots is reported in sysfs.

> I think it might be prefereable for the drivers and the framework to
> support "preprogramming" of the keys, when the key is programmed to the
> hardware when it is set by the user.

This doesn't sound particularly useful.  If there are always enough keyslots,
then keyslots never get evicted and there is no advantage to this.  If there are
*not* always enough keyslots, then it's sometimes necessary to evict keyslots,
so it would not be desirable to have them permanently reserved.

It could make sense to have some sort of hints mechanism, where frequently-used
keys can be marked as high-priority to keep programmed in a keyslot.  I don't
see much of a need for this though, given that the eviction policy is already
LRU, so it already prefers to keep frequently-used keys in a keyslot.

> Another option might be to let the drivers validate the keys being set
> by userspace. This way in our case the driver might report that it
> supports both raw and wrapped keys, but start rejecting the keys once
> it gets notified that the user has programmed other kind of keys. This
> way key setup can fail, but the actual I/O can not. WDYT?

Well, that has the same effect as the crypto capabilities check which is already
done.  The problem is that your proposal effectively revokes a capability, and
that is racy.

- Eric