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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Sat, Sep 21, 2024 at 12:49:39PM GMT, Eric Biggers wrote:
> 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.

I'm still trying to propose solutions for the hwkm-or-raw keys problem,
trying to find a way to return an error early enough. So it's not about
the hints for frequently-used keys, but for returning an error if the
user tries to program key type which became unusupported after a
previous call.

> 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

-- 
With best wishes
Dmitry




[Index of Archives]     [Linux RAID]     [Linux SCSI]     [Linux ATA RAID]     [IDE]     [Linux Wireless]     [Linux Kernel]     [ATH6KL]     [Linux Bluetooth]     [Linux Netdev]     [Kernel Newbies]     [Security]     [Git]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Device Mapper]

  Powered by Linux