| Message ID | 20210104184542.4616-1-ebiggers@kernel.org |
|---|---|
| Headers | show |
| Series | eMMC inline encryption support | expand |
On Mon, 4 Jan 2021 at 19:48, Eric Biggers <ebiggers@kernel.org> wrote: > > From: Eric Biggers <ebiggers@google.com> > > In preparation for adding CQHCI crypto engine (inline encryption) > support, add the code required to make mmc_core and mmc_block aware of > inline encryption. Specifically: > > - Add a capability flag MMC_CAP2_CRYPTO to struct mmc_host. Drivers > will set this if the host and driver support inline encryption. > > - Embed a blk_keyslot_manager in struct mmc_host. Drivers will > initialize this if the host and driver support inline encryption. > mmc_block registers this keyslot manager with the request_queue of any > MMC card attached to the host. mmc_core destroys this keyslot manager > when freeing the mmc_host. > > - Make mmc_block copy the crypto keyslot and crypto data unit number > from struct request to struct mmc_request, so that drivers will have > access to them. > > - If the MMC host is reset, reprogram all the keyslots to ensure that > the software state stays in sync with the hardware state. > > Co-developed-by: Satya Tangirala <satyat@google.com> > Signed-off-by: Satya Tangirala <satyat@google.com> > Acked-by: Adrian Hunter <adrian.hunter@intel.com> > Reviewed-by: Satya Tangirala <satyat@google.com> > Reviewed-and-tested-by: Peng Zhou <peng.zhou@mediatek.com> > Signed-off-by: Eric Biggers <ebiggers@google.com> 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 > --- > drivers/mmc/core/Kconfig | 8 ++++++ > drivers/mmc/core/Makefile | 1 + > drivers/mmc/core/block.c | 3 +++ > drivers/mmc/core/core.c | 3 +++ > drivers/mmc/core/crypto.c | 54 +++++++++++++++++++++++++++++++++++++++ > drivers/mmc/core/crypto.h | 46 +++++++++++++++++++++++++++++++++ > drivers/mmc/core/host.c | 2 ++ > drivers/mmc/core/queue.c | 3 +++ > include/linux/mmc/core.h | 6 +++++ > include/linux/mmc/host.h | 7 +++++ > 10 files changed, 133 insertions(+) > create mode 100644 drivers/mmc/core/crypto.c > create mode 100644 drivers/mmc/core/crypto.h > > diff --git a/drivers/mmc/core/Kconfig b/drivers/mmc/core/Kconfig > index c12fe13e4b147..ae8b69aee6190 100644 > --- a/drivers/mmc/core/Kconfig > +++ b/drivers/mmc/core/Kconfig > @@ -81,3 +81,11 @@ config MMC_TEST > This driver is only of interest to those developing or > testing a host driver. Most people should say N here. > > +config MMC_CRYPTO > + bool "MMC Crypto Engine Support" > + depends on BLK_INLINE_ENCRYPTION What about making this "default y" as well. > + help > + Enable Crypto Engine Support in MMC. > + Enabling this makes it possible for the kernel to use the crypto > + capabilities of the MMC device (if present) to perform crypto > + operations on data being transferred to/from the device. > diff --git a/drivers/mmc/core/Makefile b/drivers/mmc/core/Makefile > index 95ffe008ebdf8..6a907736cd7a5 100644 > --- a/drivers/mmc/core/Makefile > +++ b/drivers/mmc/core/Makefile > @@ -18,3 +18,4 @@ obj-$(CONFIG_MMC_BLOCK) += mmc_block.o > mmc_block-objs := block.o queue.o > obj-$(CONFIG_MMC_TEST) += mmc_test.o > obj-$(CONFIG_SDIO_UART) += sdio_uart.o > +mmc_core-$(CONFIG_MMC_CRYPTO) += crypto.o > diff --git a/drivers/mmc/core/block.c b/drivers/mmc/core/block.c > index 42e27a2982180..b877f62df3660 100644 > --- a/drivers/mmc/core/block.c > +++ b/drivers/mmc/core/block.c > @@ -51,6 +51,7 @@ > #include "block.h" > #include "core.h" > #include "card.h" > +#include "crypto.h" > #include "host.h" > #include "bus.h" > #include "mmc_ops.h" > @@ -1247,6 +1248,8 @@ static void mmc_blk_data_prep(struct mmc_queue *mq, struct mmc_queue_req *mqrq, > > memset(brq, 0, sizeof(struct mmc_blk_request)); > > + mmc_crypto_prepare_req(mqrq); > + > brq->mrq.data = &brq->data; > brq->mrq.tag = req->tag; > > diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c > index 19f1ee57fb345..bd4b557e68899 100644 > --- a/drivers/mmc/core/core.c > +++ b/drivers/mmc/core/core.c > @@ -37,6 +37,7 @@ > > #include "core.h" > #include "card.h" > +#include "crypto.h" > #include "bus.h" > #include "host.h" > #include "sdio_bus.h" > @@ -992,6 +993,8 @@ void mmc_set_initial_state(struct mmc_host *host) > host->ops->hs400_enhanced_strobe(host, &host->ios); > > mmc_set_ios(host); > + > + mmc_crypto_set_initial_state(host); > } > > /** > diff --git a/drivers/mmc/core/crypto.c b/drivers/mmc/core/crypto.c > new file mode 100644 > index 0000000000000..4f47eb4740db0 > --- /dev/null > +++ b/drivers/mmc/core/crypto.c > @@ -0,0 +1,54 @@ > +// SPDX-License-Identifier: GPL-2.0-only > +/* > + * MMC crypto engine (inline encryption) support > + * > + * Copyright 2020 Google LLC > + */ > + > +#include <linux/blk-crypto.h> > +#include <linux/mmc/host.h> > + > +#include "core.h" > +#include "crypto.h" > +#include "queue.h" > + > +void mmc_crypto_set_initial_state(struct mmc_host *host) > +{ > + /* Reset might clear all keys, so reprogram all the keys. */ > + if (host->caps2 & MMC_CAP2_CRYPTO) > + blk_ksm_reprogram_all_keys(&host->ksm); > +} > + > +void mmc_crypto_free_host(struct mmc_host *host) > +{ > + if (host->caps2 & MMC_CAP2_CRYPTO) > + blk_ksm_destroy(&host->ksm); > +} > + > +void mmc_crypto_setup_queue(struct request_queue *q, struct mmc_host *host) > +{ > + if (host->caps2 & MMC_CAP2_CRYPTO) > + blk_ksm_register(&host->ksm, q); > +} > +EXPORT_SYMBOL_GPL(mmc_crypto_setup_queue); > + > +void mmc_crypto_prepare_req(struct mmc_queue_req *mqrq) > +{ > + struct request *req = mmc_queue_req_to_req(mqrq); > + struct mmc_request *mrq = &mqrq->brq.mrq; > + > + if (!req->crypt_keyslot) > + return; > + > + mrq->crypto_enabled = true; > + mrq->crypto_key_slot = blk_ksm_get_slot_idx(req->crypt_keyslot); > + > + /* > + * For now we assume that all MMC drivers set max_dun_bytes_supported=4, > + * which is the limit for CQHCI crypto. So all DUNs should be 32-bit. > + */ > + WARN_ON_ONCE(req->crypt_ctx->bc_dun[0] > U32_MAX); > + > + mrq->data_unit_num = req->crypt_ctx->bc_dun[0]; > +} > +EXPORT_SYMBOL_GPL(mmc_crypto_prepare_req); > diff --git a/drivers/mmc/core/crypto.h b/drivers/mmc/core/crypto.h > new file mode 100644 > index 0000000000000..4780639b832f4 > --- /dev/null > +++ b/drivers/mmc/core/crypto.h > @@ -0,0 +1,46 @@ > +/* SPDX-License-Identifier: GPL-2.0-only */ > +/* > + * MMC crypto engine (inline encryption) support > + * > + * Copyright 2020 Google LLC > + */ > + > +#ifndef _MMC_CORE_CRYPTO_H > +#define _MMC_CORE_CRYPTO_H > + > +struct mmc_host; > +struct mmc_queue_req; > +struct request_queue; > + > +#ifdef CONFIG_MMC_CRYPTO > + > +void mmc_crypto_set_initial_state(struct mmc_host *host); > + > +void mmc_crypto_free_host(struct mmc_host *host); > + > +void mmc_crypto_setup_queue(struct request_queue *q, struct mmc_host *host); > + > +void mmc_crypto_prepare_req(struct mmc_queue_req *mqrq); > + > +#else /* CONFIG_MMC_CRYPTO */ > + > +static inline void mmc_crypto_set_initial_state(struct mmc_host *host) > +{ > +} > + > +static inline void mmc_crypto_free_host(struct mmc_host *host) > +{ > +} > + > +static inline void mmc_crypto_setup_queue(struct request_queue *q, > + struct mmc_host *host) > +{ > +} > + > +static inline void mmc_crypto_prepare_req(struct mmc_queue_req *mqrq) > +{ > +} > + > +#endif /* !CONFIG_MMC_CRYPTO */ > + > +#endif /* _MMC_CORE_CRYPTO_H */ > diff --git a/drivers/mmc/core/host.c b/drivers/mmc/core/host.c > index 96b2ca1f1b06d..d962b9ca0e37a 100644 > --- a/drivers/mmc/core/host.c > +++ b/drivers/mmc/core/host.c > @@ -25,6 +25,7 @@ > #include <linux/mmc/slot-gpio.h> > > #include "core.h" > +#include "crypto.h" > #include "host.h" > #include "slot-gpio.h" > #include "pwrseq.h" > @@ -532,6 +533,7 @@ EXPORT_SYMBOL(mmc_remove_host); > */ > void mmc_free_host(struct mmc_host *host) > { > + mmc_crypto_free_host(host); > mmc_pwrseq_free(host); > put_device(&host->class_dev); > } > diff --git a/drivers/mmc/core/queue.c b/drivers/mmc/core/queue.c > index de7cb0369c308..d96db852bb91a 100644 > --- a/drivers/mmc/core/queue.c > +++ b/drivers/mmc/core/queue.c > @@ -19,6 +19,7 @@ > #include "block.h" > #include "core.h" > #include "card.h" > +#include "crypto.h" > #include "host.h" > > #define MMC_DMA_MAP_MERGE_SEGMENTS 512 > @@ -405,6 +406,8 @@ static void mmc_setup_queue(struct mmc_queue *mq, struct mmc_card *card) > mutex_init(&mq->complete_lock); > > init_waitqueue_head(&mq->wait); > + > + mmc_crypto_setup_queue(mq->queue, host); > } > > static inline bool mmc_merge_capable(struct mmc_host *host) > diff --git a/include/linux/mmc/core.h b/include/linux/mmc/core.h > index 29aa507116261..ab19245e99451 100644 > --- a/include/linux/mmc/core.h > +++ b/include/linux/mmc/core.h > @@ -162,6 +162,12 @@ struct mmc_request { > bool cap_cmd_during_tfr; > > int tag; > + > +#ifdef CONFIG_MMC_CRYPTO > + bool crypto_enabled; > + int crypto_key_slot; > + u32 data_unit_num; > +#endif > }; > > struct mmc_card; > diff --git a/include/linux/mmc/host.h b/include/linux/mmc/host.h > index 01bba36545c54..6f86948f92caf 100644 > --- a/include/linux/mmc/host.h > +++ b/include/linux/mmc/host.h > @@ -15,6 +15,7 @@ > #include <linux/mmc/card.h> > #include <linux/mmc/pm.h> > #include <linux/dma-direction.h> > +#include <linux/keyslot-manager.h> > > struct mmc_ios { > unsigned int clock; /* clock rate */ > @@ -384,6 +385,7 @@ struct mmc_host { > #define MMC_CAP2_CQE_DCMD (1 << 24) /* CQE can issue a direct command */ > #define MMC_CAP2_AVOID_3_3V (1 << 25) /* Host must negotiate down from 3.3V */ > #define MMC_CAP2_MERGE_CAPABLE (1 << 26) /* Host can merge a segment over the segment size */ > +#define MMC_CAP2_CRYPTO (1 << 27) /* Host supports inline encryption */ > > int fixed_drv_type; /* fixed driver type for non-removable media */ > > @@ -478,6 +480,11 @@ struct mmc_host { > bool cqe_enabled; > bool cqe_on; > > + /* Inline encryption support */ > +#ifdef CONFIG_MMC_CRYPTO > + struct blk_keyslot_manager ksm; > +#endif > + > /* Host Software Queue support */ > bool hsq_enabled; > > -- > 2.30.0 >
On Fri, Jan 15, 2021 at 10:22:03AM +0100, Ulf Hansson wrote: > On Mon, 4 Jan 2021 at 19:48, Eric Biggers <ebiggers@kernel.org> wrote: > > > > From: Eric Biggers <ebiggers@google.com> > > > > In preparation for adding CQHCI crypto engine (inline encryption) > > support, add the code required to make mmc_core and mmc_block aware of > > inline encryption. Specifically: > > > > - Add a capability flag MMC_CAP2_CRYPTO to struct mmc_host. Drivers > > will set this if the host and driver support inline encryption. > > > > - Embed a blk_keyslot_manager in struct mmc_host. Drivers will > > initialize this if the host and driver support inline encryption. > > mmc_block registers this keyslot manager with the request_queue of any > > MMC card attached to the host. mmc_core destroys this keyslot manager > > when freeing the mmc_host. > > > > - Make mmc_block copy the crypto keyslot and crypto data unit number > > from struct request to struct mmc_request, so that drivers will have > > access to them. > > > > - If the MMC host is reset, reprogram all the keyslots to ensure that > > the software state stays in sync with the hardware state. > > > > Co-developed-by: Satya Tangirala <satyat@google.com> > > Signed-off-by: Satya Tangirala <satyat@google.com> > > Acked-by: Adrian Hunter <adrian.hunter@intel.com> > > Reviewed-by: Satya Tangirala <satyat@google.com> > > Reviewed-and-tested-by: Peng Zhou <peng.zhou@mediatek.com> > > Signed-off-by: Eric Biggers <ebiggers@google.com> > > 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? - Eric
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
On Tue, Jan 19, 2021 at 12:52:00PM -0800, Eric Biggers wrote: > > 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. Actually devres is more flexible than I thought; it's possible to register custom actions to be executed. I'll send out a patchset that adds devm_blk_ksm_init() and converts the UFS driver to use it. - Eric
On Tue, Jan 19, 2021 at 12:51:58PM -0800, Eric Biggers wrote: > 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? I've sent out a new version of this patchset that uses the new function devm_blk_ksm_init() I've proposed, so that the blk_keyslot_manager no longer needs to be explicitly destroyed. Please let me know if you still have any other concerns. I think you may have been assuming that the blk_keyslot_manager doesn't get initialized until the CQE is enabled, and thus would have a separate lifetime from the other host structures? That's not what happens; it just gets initialized on the driver probe path. See e.g.: sdhci_msm_probe() => sdhci_msm_cqe_add_host() => cqhci_init() => cqhci_crypto_init() => devm_blk_ksm_init() And we don't leave MMC_CAP2_CRYPTO set but the blk_keyslot_manager uninitialized, as that combination doesn't make sense. Thanks, - Eric
On Thu, 21 Jan 2021 at 10:18, Eric Biggers <ebiggers@kernel.org> wrote: > > On Tue, Jan 19, 2021 at 12:51:58PM -0800, Eric Biggers wrote: > > 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? > > I've sent out a new version of this patchset that uses the new function > devm_blk_ksm_init() I've proposed, so that the blk_keyslot_manager no longer > needs to be explicitly destroyed. > > Please let me know if you still have any other concerns. > > I think you may have been assuming that the blk_keyslot_manager doesn't get > initialized until the CQE is enabled, and thus would have a separate lifetime > from the other host structures? That's not what happens; it just gets > initialized on the driver probe path. See e.g.: > > sdhci_msm_probe() > => sdhci_msm_cqe_add_host() > => cqhci_init() > => cqhci_crypto_init() > => devm_blk_ksm_init() > > And we don't leave MMC_CAP2_CRYPTO set but the blk_keyslot_manager > uninitialized, as that combination doesn't make sense. I think we still can (but we shouldn't), but see my reply on the latest version. Besides that minor thing, the latest version looks good to me. Thanks for doing the conversion to devm_blk_ksm_init() - it certainly simplifies the lifecycle issue for this case. Kind regards Uffe
Hello, This patchset adds support for eMMC inline encryption, as specified by the upcoming version of the eMMC specification and as already implemented and used on many devices. Building on that, it then adds Qualcomm ICE support and wires it up for the Snapdragon 630 SoC. Inline encryption hardware improves the performance of storage encryption and reduces power usage. See Documentation/block/inline-encryption.rst for more information about inline encryption and the blk-crypto framework (upstreamed in v5.8) which supports it. Most mobile devices already use UFS or eMMC inline encryption hardware; UFS support was already upstreamed in v5.9. Patches 1-4 add support for the standard eMMC inline encryption. However, as with UFS, host controller-specific patches are needed on top of the standard support. Therefore, patches 5-9 add Qualcomm ICE (Inline Crypto Engine) support and wire it up on the Snapdragon 630 SoC. To test this I took advantage of the recently upstreamed support for the Snapdragon 630 SoC, plus work-in-progress patches from the SoMainline project (https://github.com/SoMainline/linux/tree/konrad/v5.10-rc3). In particular, I was able to run the fscrypt xfstests for ext4 and f2fs in a Debian chroot. Among other things, these tests verified that the correct ciphertext is written to disk (the same as software encryption). It will also be possible to add support for Mediatek eMMC inline encryption hardware in mtk-sd, and it should be easier than the Qualcomm hardware since the Mediatek hardware follows the standard more closely. I.e., patches 1-4 should be almost enough for the Mediatek hardware. However, I don't have the hardware to do this yet. This patchset is based on v5.11-rc2, and it can also be retrieved from tag "mmc-crypto-v4" of https://git.kernel.org/pub/scm/linux/kernel/git/ebiggers/linux.git Changed since v3: - Added Acked-by and Reviewed-and-tested-by tags. - Rebased onto v5.11-rc2. Changed since v2: - Improved comment for sdhci_msm_ice_wait_bist_status() - Removed an unhelpful comment in union cqhci_crypto_cfg_entry. - Fixed the commit message of "mmc: cqhci: initialize upper 64 bits of 128-bit task descriptors". - Added Reviewed-by's and Acked-by's. Changed since v1: - Only select QCOM_SCM if ARCH_QCOM. (Fixes a build break.) - Split most of the cqhci_prep_task_desc() change into its own patch. - Made sdhci_msm_ice_wait_bist_status() use readl_poll_timeout(). - Added a couple more comments. - Added some Acked-by's. Eric Biggers (9): mmc: add basic support for inline encryption mmc: cqhci: rename cqhci.c to cqhci-core.c mmc: cqhci: initialize upper 64 bits of 128-bit task descriptors mmc: cqhci: add support for inline encryption mmc: cqhci: add cqhci_host_ops::program_key firmware: qcom_scm: update comment for ICE-related functions dt-bindings: mmc: sdhci-msm: add ICE registers and clock arm64: dts: qcom: sdm630: add ICE registers and clocks mmc: sdhci-msm: add Inline Crypto Engine support .../devicetree/bindings/mmc/sdhci-msm.txt | 3 + arch/arm64/boot/dts/qcom/sdm630.dtsi | 10 +- drivers/firmware/qcom_scm.c | 16 +- drivers/mmc/core/Kconfig | 8 + drivers/mmc/core/Makefile | 1 + drivers/mmc/core/block.c | 3 + drivers/mmc/core/core.c | 3 + drivers/mmc/core/crypto.c | 54 ++++ drivers/mmc/core/crypto.h | 46 +++ drivers/mmc/core/host.c | 2 + drivers/mmc/core/queue.c | 3 + drivers/mmc/host/Kconfig | 1 + drivers/mmc/host/Makefile | 2 + drivers/mmc/host/{cqhci.c => cqhci-core.c} | 69 ++++- drivers/mmc/host/cqhci-crypto.c | 245 ++++++++++++++++ drivers/mmc/host/cqhci-crypto.h | 47 +++ drivers/mmc/host/cqhci.h | 84 +++++- drivers/mmc/host/sdhci-msm.c | 276 +++++++++++++++++- include/linux/mmc/core.h | 6 + include/linux/mmc/host.h | 7 + 20 files changed, 861 insertions(+), 25 deletions(-) create mode 100644 drivers/mmc/core/crypto.c create mode 100644 drivers/mmc/core/crypto.h rename drivers/mmc/host/{cqhci.c => cqhci-core.c} (94%) create mode 100644 drivers/mmc/host/cqhci-crypto.c create mode 100644 drivers/mmc/host/cqhci-crypto.h base-commit: e71ba9452f0b5b2e8dc8aa5445198cd9214a6a62