Re: [PATCH v7 6/9] scsi: ufs: Add inline encryption support to UFS

[Eric Biggers <ebiggers@kernel.org>]

On Tue, Feb 25, 2020 at 03:21:25PM +0800, Stanley Chu wrote:
> Hi Christoph,
> 
> On Mon, 2020-02-24 at 15:37 -0800, Christoph Hellwig wrote:
> > On Sun, Feb 23, 2020 at 09:47:36PM +0800, Stanley Chu wrote:
> > > Yes, MediaTek is keeping work closely with inline encryption patch sets.
> > > Currently the v6 version can work well (without
> > > UFSHCD_QUIRK_BROKEN_CRYPTO quirk) at least in our MT6779 SoC platform
> > > which basic SoC support and some other peripheral drivers are under
> > > upstreaming as below link,
> > > 
> > > https://patchwork.kernel.org/project/linux-mediatek/list/?state=%
> > > 2A&q=6779&series=&submitter=&delegate=&archive=both
> > > 
> > > The integration with inline encryption patch set needs to patch
> > > ufs-mediatek and patches are ready in downstream. We plan to upstream
> > > them soon after inline encryption patch sets get merged.
> > 
> > What amount of support do you need in ufs-mediatek?  It seems like
> > pretty much every ufs low-level driver needs some kind of specific
> > support now, right?  I wonder if we should instead opt into the support
> > instead of all the quirking here.
> 
> The patch in ufs-mediatek is aimed to issue vendor-specific SMC calls
> for host initialization and configuration. This is because MediaTek UFS
> host has some "secure-protected" registers/features which need to be
> accessed/switched in secure world. 
> 
> Such protection is not mentioned by UFSHCI specifications thus inline
> encryption patch set assumes that every registers in UFSHCI can be
> accessed normally in kernel. This makes sense and surely the patchset
> can work fine in any "standard" UFS host. However if host has special
> design then it can work normally only if some vendor-specific treatment
> is applied.
> 
> I think one of the reason to apply UFSHCD_QUIRK_BROKEN_CRYPTO quirk in
> ufs-qcom host is similar to above case.

So, I had originally assumed that most kernel developers would prefer to make
the UFS crypto support opt-out rather than opt-in, since that matches the normal
Linux way of doing things.  I.e. normally the kernel's default assumption is
that devices implement the relevant standard, and only when a device is known to
deviate from the standard does the driver apply quirks.

But indeed, as we've investigated more vendors' UFS hardware, it seems that
everyone has some quirk that needs to be handled in the platform driver:

  - ufs-qcom (tested on DragonBoard 845c with upstream kernel) needs
    vendor-specific crypto initialization logic and SMC calls to set keys

  - ufs-mediatek needs the quirks that Stanley mentioned above

  - ufs-hisi (tested on Hikey960 with upstream kernel) needs to write a
    vendor-specific register to use high keyslots, but even then I still
    couldn't get the crypto support working correctly.

I'm not sure about the UFS controllers from Synopsys, Cadence, or Samsung, all
of which apparently have implemented some form of the crypto support too.  But I
wouldn't get my hopes up that everyone followed the UFS standard precisely.

So if there are no objections, IMO we should make the crypto support opt-in.

That makes it even more important to upstream the crypto support for specific
hardware like ufs-qcom and ufs-mediatek, since otherwise the ufshcd-crypto code
would be unusable even theoretically.  I'm volunteering to handle ufs-qcom with
https://lkml.kernel.org/linux-block/20200110061634.46742-1-ebiggers@kernel.org/.
Stanley, could you send out ufs-mediatek support as an RFC so people can see
better what it involves?

- Eric