Re: [RFC PATCH v2 3/4] scsi: ufs: add program_key() variant op

Eric Biggers
2020-03-04T20:52:37Z

Hi Barani,

On Wed, Mar 04, 2020 at 12:18:20PM -0800, bmuthuku@codeaurora.org wrote:
> Eric, I strongly recommend not to support the old mechanism of calling into
> TEE to set keys as this has been deprecated and will not work with newer
> hardware.

Actually, I've also verified that this driver works nearly as-is on Snapdragon
865 and Snapdragon 765. (Those SoCs currently lack upstream support, but I
tested with downstream kernels.) IIUC, this is the latest Qualcomm hardware in
the real world. Lots of phones will be shipping with those SoCS this year.

So your position is that crypto support for current Qualcomm hardware shouldn't
be added, and instead we should wait years for new hardware that hasn't even
been announced/released yet and has no upstream kernel support at all yet?
Snapdragon 845 (via DragonBoard 845c) already has upstream support.

We can easily support multiple hardware versions in the driver, so I don't see
why this is an issue.

In particular, if Qualcomm releases new hardware that just complies with the UFS
standard and doesn't require these weird vendor-specific SCM calls, we can just
conditionally skip the quirks when standard hardware is detected.

> There are few issues with this patch, it adds all the code within
> UFS and we would have to reimplement all the common ICE code for eMMC as
> well.

The downstream solution of having a separate ICE "device" and "driver" is really
horrendous and results in lots of layering violations and unnecessary code,
since as far as I can tell ICE is actually part of the UFS and eMMC controllers
and not a separate device.

So I think starting with the ICE logic in ufs-qcom is the right approach.

If it turns out that sdhci-msm needs similar ICE management code, we can put it
in a library. Though, note that after I removed all the unneeded stuff, we're
only talking about ~200 lines of code, so it may not even be worthwhile.

However, an issue with eMMC is that the eMMC crypto standard hasn't actually
been published yet. And I haven't heard of any vendors implementing it besides
Qualcomm, nor do I know of any upstream SoC we can use to develop it.

So if you want eMMC crypto support, you're going to have to help with it.

> For clearing a key, the patch uses program_key to set zeroes to the
> keyslot, without going into details (since newer hardware is not yet public)
> this will not work.

That's incorrect; this patch uses the SCM call QCOM_SCM_ES_INVALIDATE_ICE_KEY to
evict keys. That's exactly what the downstream driver does.

Of course, when we add support for newer hardware we can use whatever keyslot
management method works on that hardware. Hopefully, it's the standard UFS
method and not another weird vendor-specific method.

> We have a plan to upstream ICE support with the new hardware along with the
> framework to support wrapped keys and add sdhci/cqhci-crypto support.

Great, what's the timeline on this? Are there patches you can point to
anywhere? How would one get ahold of this hardware, and does it / will it have
upstream kernel support like sda845 does?

- Eric