Re: [PATCH v4 1/9] mmc: add basic support for inline encryption

[Eric Biggers <ebiggers@kernel.org>]

On Mon, Jan 18, 2021 at 03:21:01PM +0100, Ulf Hansson wrote:
> > > Eric, again, my apologies for the delay. Overall, I think this looks good.
> > >
> > > My only hesitation to merge this as is, is that I want to make sure
> > > you have thought of the life cycle issues for the struct
> > > blk_keyslot_manager ksm. It's being used both from the mmc core/block
> > > device driver and the mmc host driver. I am looking at this right now
> > > and will get back to you very soon, if I find some issues with it.
> > >
> > > If you have some time, feel free to elaborate around how this is
> > > intended to work.
> > >
> > > Kind regards
> > > Uffe
> >
> > The blk_keyslot_manager is initialized early on when the other host structures
> > (struct mmc_host, struct cqhci_host, struct sdhci_host, struct sdhci_msm_host)
> > are initialized, prior to mmc_add_host().
> >
> > It is destroyed when the struct mmc_host is freed by mmc_free_host().
> >
> > So it should just work; it's the same lifecycle as the existing host structures.
> > Is there something you think I'm overlooking?
> 
> I think so, but let me elaborate a bit.
> 
> As I understand it, to initialize the data structures, blk_ksm_init()
> is getting called and via cqhci_init().
> 
> To hook up the block request queue, blk_ksm_register() is called via
> mmc_setup_queue(), which means this happens when the mmc block device
> driver is probed.

Well, the call to blk_ksm_register() happens in mmc_crypto_setup_queue(), when
allocating the request_queue for a particular mmc_card.  As far as I can tell,
the mmc_host has already been initialized and added then, so we don't have to
worry about cases where the mmc_host has only been partially initialized.
And in particular, MMC_CAP2_CRYPTO will have its final value.

> 
> To free up the data structures, blk_ksm_destroy() is called from
> mmc_free_host().
> 
> To me, this can be made more consistent. For example, it looks like
> blk_ksm_destroy() could be called, even if blk_ksm_init() hasn't been
> called (depending on the probe error path of the mmc host).

blk_ksm_destroy() is a no-op on an all-zeroed struct, so it's fine to call it
unnecessarily.  We could call it unconditionally, if that would be clearer.

> There are a couple of options to better deal with this.
> 1) Extend the blk_ksm interface with a devm_blk_ksm_init() function
> (thus let it deal with lifecycle problems for us) and simply drop the
> call to blk_ksm_destroy().

This would require adding APIs to devm to support zeroing buffers on free and to
use kvmalloc() instead of kmalloc().  It looks like these new APIs wouldn't be
useful for many drivers (since almost everyone else just wants regular kmalloc
with no special behavior on free), so they don't seem worth adding yet.

> 2) Extend the cqhci interface with a cleanup function (perhaps
> "cqhci_deinit") and let it call blk_ksm_destroy().

The blk_keyslot_manager is part of struct mmc_host, so it makes more sense for
mmc_core to be responsible for freeing it.

We could move it to cqhci_host, but that would require adding multiple new
function pointers to mmc_cqe_ops for use by mmc_crypto_set_initial_state(),
mmc_crypto_free_host(), and mmc_crypto_setup_queue(), as these all currently
need access to the blk_keyslot_manager.

I think that making mmc_core directly aware of the blk_keyslot_manager is the
right call, as it avoids excessive callbacks, and it avoids tying the inline
encryption support too closely to CQHCI.  (Keep in mind that in the future, MMC
hosts could support inline encryption using other interfaces besides CQHCI.)

> 3) Convert to let blk_ksm_init() to be called from mmc_add_host() and
> blk_ksm_destroy() from mmc_remove_host().

That won't work because the driver has to fill in the crypto capabilities in the
blk_keyslot_manager after calling blk_ksm_init().  mmc_add_host() is too late to
do that.  mmc_add_host() happens after the driver has already initialized the
host structures and is finally registering them with the driver model.

> 
> Moreover, even if there seems to be no real need to call
> blk_ksm_unregister() for the mmc block device driver, perhaps we
> should still do it to be consistent with blk_ksm_register()?

blk_ksm_unregister() isn't exported to modules.  Its only purpose is for the
block layer to disable inline encryption support on a disk if blk-integrity
support is registered on the same disk.  So it shouldn't (and can't) be called
by drivers.

We probably should just remove blk_ksm_unregister() and make
blk_integrity_register() set the ->ksm pointer to NULL directly.  Also maybe
blk_ksm_register() should be renamed to something like
"queue_set_keyslot_manager()" to avoid implying that "unregister" is needed.

However those would be block layer changes, not related to this patchset.

> 
> Then a final concern. It looks like the mmc core relies on checking
> "host->caps2 & MMC_CAP2_CRYPTO", when it calls blk_ksm_register() and
> blk_ksm_reprogram_all_keys(), for example. Normally, host->caps2 bits
> are considered as static configurations and set during the host driver
> probe path, which may not be a good match for this case. Instead, it
> seems like we should set a new separate flag, to indicate for the mmc
> core that blk_ksm_init has been enabled. Otherwise it looks like we
> could end up calling blk_ksm_reprogram_all_keys(), even if
> blk_ksm_init() hasn't been called.

MMC_CAP2_CRYPTO *is* a static configuration that is set during the host driver
probe path.  So I don't understand your concern here.

It's true that during the host driver probe path, MMC_CAP2_CRYPTO initially
means "the hardware might support crypto", and then cqhci_crypto_init() clears
it if it decides that the hardware doesn't support crypto after all, after which
the bit really does mean "the hardware supports crypto".

That seems fine because this all happens while the host structures are being
initialized, before they are registered with the driver model and MMC cards are
detected.  So AFAICS there can't be any concurrent calls to
mmc_crypto_set_initial_state() or mmc_crypto_setup_queue().  Do you think
otherwise?

- Eric