| Message ID | 166983619332.2734609.2800078343178136915.stgit@djiang5-desk3.ch.intel.com |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Introduce security commands for CXL pmem device | expand |
On Wed, 30 Nov 2022, Dave Jiang wrote: >Bypass cpu_cache_invalidate_memregion() and checks when doing testing >using CONFIG_NVDIMM_SECURITY_TEST flag. The bypass allows testing on >QEMU where cpu_cache_has_invalidate_memregion() fails. Usage of >cpu_cache_invalidate_memregion() is not needed for cxl_test security >testing. We'll also want something similar for the non-pmem specific security bits by extending these wrappers with CONFIG_CXL_SECURITY_TEST. I think the current naming is very generic but the functionality is too tied to pmem. So I would either rename these to 'cxl_pmem...' or make them more generic by placing them in cxlmem.h and taking the dev pointer directly as well as the iores. Thanks, Davidlohr
Davidlohr Bueso wrote: > On Wed, 30 Nov 2022, Dave Jiang wrote: > > >Bypass cpu_cache_invalidate_memregion() and checks when doing testing > >using CONFIG_NVDIMM_SECURITY_TEST flag. The bypass allows testing on > >QEMU where cpu_cache_has_invalidate_memregion() fails. Usage of > >cpu_cache_invalidate_memregion() is not needed for cxl_test security > >testing. > > We'll also want something similar for the non-pmem specific security > bits Wait, you expect someone is going to build a device *with* security commands but *without* pmem? In the volatile case the device can just secure erase itself without user intervention every time power is removed, no need for explicit user action to trigger that. So the data-at-rest security argument goes away with a pure volatile device, no? > think the current naming is very generic but the functionality is > too tied to pmem. So I would either rename these to 'cxl_pmem...' > or make them more generic by placing them in cxlmem.h and taking the > dev pointer directly as well as the iores. This does remind me that security is just one use case CXL has for cpu_cache_has_invalidate_memregion(). It also needs to be used for any HDM decoder changes where the HPA to DPA translation has changed. I think this means that any user created region needs to flush CPU caches before that region goes into service. So I like the idea of separate cxl_pmem_invalidate_memregion() and cxl_region_invalidate_memregion() with something like: diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c index 1e61d1bafc0c..430e8e5ba7d9 100644 --- a/drivers/cxl/core/region.c +++ b/drivers/cxl/core/region.c @@ -1403,6 +1403,8 @@ static int attach_target(struct cxl_region *cxlr, const char *decoder, int pos) goto out; down_read(&cxl_dpa_rwsem); rc = cxl_region_attach(cxlr, to_cxl_endpoint_decoder(dev), pos); + if (rc == 0) + set_bit(CXL_REGION_F_INCOHERENT, &cxlr->flags); up_read(&cxl_dpa_rwsem); up_write(&cxl_region_rwsem); out: @@ -1958,6 +1960,33 @@ static int devm_cxl_add_pmem_region(struct cxl_region *cxlr) return rc; } +static bool cxl_region_has_invalidate_memregion(struct cxl_region *cxlr) +{ + if (!cpu_cache_has_invalidate_memregion()) { + if (IS_ENABLED(CONFIG_CXL_REGION_TEST)) { + dev_warn_once( + &cxlr->dev, + "Bypassing cpu_cache_has_invalidate_memregion() check for testing!\n"); + return true; + } + return false; + } + + return true; +} + +static void cxl_region_invalidate_memregion(struct cxl_region *cxlr) +{ + if (IS_ENABLED(CONFIG_CXL_REGION_TEST)) { + dev_warn_once( + &cxlr->dev, + "Bypassing cpu_cache_invalidate_memergion() for testing!\n"); + return; + } + + cpu_cache_invalidate_memregion(IORES_DESC_CXL); +} + static int cxl_region_probe(struct device *dev) { struct cxl_region *cxlr = to_cxl_region(dev); @@ -1975,12 +2004,22 @@ static int cxl_region_probe(struct device *dev) rc = -ENXIO; } + if (test_bit(CXL_REGION_F_INCOHERENT, &cxlr->flags) && + !cxl_region_has_invalidate_memregion(cxlr)) { + dev_err(&cxlr->dev, "Failed to synchronize CPU cache state\n"); + rc = -ENXIO; + } else if (test_and_clear_bit(CXL_REGION_F_INCOHERENT, &cxlr->flags)) + cxl_region_invalidate_memregion(cxlr); + /* * From this point on any path that changes the region's state away from * CXL_CONFIG_COMMIT is also responsible for releasing the driver. */ up_read(&cxl_region_rwsem); + if (rc) + return rc; + switch (cxlr->mode) { case CXL_DECODER_PMEM: return devm_cxl_add_pmem_region(cxlr); @@ -2008,4 +2047,5 @@ void cxl_region_exit(void) } MODULE_IMPORT_NS(CXL); +MODULE_IMPORT_NS(DEVMEM); MODULE_ALIAS_CXL(CXL_DEVICE_REGION); diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h index 9a212ab3cae4..827b1ad6cae4 100644 --- a/drivers/cxl/cxl.h +++ b/drivers/cxl/cxl.h @@ -388,12 +388,19 @@ struct cxl_region_params { int nr_targets; }; +/* + * Flag whether this region needs to have its HPA span synchronized with + * CPU cache state at region activation time. + */ +#define CXL_REGION_F_INCOHERENT BIT(0) + /** * struct cxl_region - CXL region * @dev: This region's device * @id: This region's id. Id is globally unique across all regions * @mode: Endpoint decoder allocation / access mode * @type: Endpoint decoder target type + * @flags: Region state flags * @cxl_nvb: nvdimm bridge for coordinating @cxlr_pmem setup / shutdown * @cxlr_pmem: (for pmem regions) cached copy of the nvdimm bridge * @params: active + config params for the region @@ -403,6 +410,7 @@ struct cxl_region { int id; enum cxl_decoder_mode mode; enum cxl_decoder_type type; + unsigned long flags; struct cxl_nvdimm_bridge *cxl_nvb; struct cxl_pmem_region *cxlr_pmem; struct cxl_region_params params;
On Wed, 30 Nov 2022, Dan Williams wrote: >Davidlohr Bueso wrote: >> On Wed, 30 Nov 2022, Dave Jiang wrote: >> >> >Bypass cpu_cache_invalidate_memregion() and checks when doing testing >> >using CONFIG_NVDIMM_SECURITY_TEST flag. The bypass allows testing on >> >QEMU where cpu_cache_has_invalidate_memregion() fails. Usage of >> >cpu_cache_invalidate_memregion() is not needed for cxl_test security >> >testing. >> >> We'll also want something similar for the non-pmem specific security >> bits > >Wait, you expect someone is going to build a device *with* security >commands but *without* pmem? In the volatile case the device can just >secure erase itself without user intervention every time power is >removed, no need for explicit user action to trigger that. So the >data-at-rest security argument goes away with a pure volatile device, >no? Well the spec explicitly states that sanitation can be done to volatile capacity devices, which makes me think the use case for this would not require rebooting. Thanks, Davidlohr
Davidlohr Bueso wrote: > On Wed, 30 Nov 2022, Dan Williams wrote: > > >Davidlohr Bueso wrote: > >> On Wed, 30 Nov 2022, Dave Jiang wrote: > >> > >> >Bypass cpu_cache_invalidate_memregion() and checks when doing testing > >> >using CONFIG_NVDIMM_SECURITY_TEST flag. The bypass allows testing on > >> >QEMU where cpu_cache_has_invalidate_memregion() fails. Usage of > >> >cpu_cache_invalidate_memregion() is not needed for cxl_test security > >> >testing. > >> > >> We'll also want something similar for the non-pmem specific security > >> bits > > > >Wait, you expect someone is going to build a device *with* security > >commands but *without* pmem? In the volatile case the device can just > >secure erase itself without user intervention every time power is > >removed, no need for explicit user action to trigger that. So the > >data-at-rest security argument goes away with a pure volatile device, > >no? > > Well the spec explicitly states that sanitation can be done to volatile > capacity devices, which makes me think the use case for this would not > require rebooting. It does, but Linux supports memory sharing across multiple tenants today without secure erase. So I think the specification may have been getting ahead of itself about how useful that is as a concept, at least in the near term. In the long term, when memory may be shared outside of the system I think the kernel will have an interest in santizing that memory before it leaves the local system control, but for that there's the "Sanitize on Release" facility for DCD regions. You are right though, I was under the mistaken impression that CXL Secure Erase behaved like PMEM [1], and only erased the persistent media, not the volatile, but the spec says it erases everything user accessible. That's going to need to make it as violent as CXL Sanitize from the perspective of what needs to be taken offline before the operation, similar to PMEM Overwrite. However, the cache management for Sanitize, Secure Erase, and Unlock can all be unified under the same cxl_region_invalidate_memregion() proposal. Basically any time the DPA contents are changed behind the kernel's back either "in-place" by a security operation, or a "remap" event of assigning different DPA, any associated regions get marked CXL_REGION_F_INCOHERENT. Then the region driver can handle cache management centrally if the given device gets reactivated. That also has the nice property of deferring wbinvd violence until the last possible moment where the cache incoherence can actually spill over into a data corruption scenario. [1]: https://pmem.io/documents/IntelOptanePMem_DSM_Interface-V3.0.pdf
On Wed, 30 Nov 2022 12:23:13 -0700 Dave Jiang <dave.jiang@intel.com> wrote: > Bypass cpu_cache_invalidate_memregion() and checks when doing testing > using CONFIG_NVDIMM_SECURITY_TEST flag. The bypass allows testing on > QEMU where cpu_cache_has_invalidate_memregion() fails. Usage of > cpu_cache_invalidate_memregion() is not needed for cxl_test security > testing. > > Signed-off-by: Dave Jiang <dave.jiang@intel.com> Subject to naming discussion resolving, this looks good to me.. Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com> > --- > drivers/cxl/security.c | 35 ++++++++++++++++++++++++++++++----- > 1 file changed, 30 insertions(+), 5 deletions(-) > > diff --git a/drivers/cxl/security.c b/drivers/cxl/security.c > index cbd005ceb091..2b5088af8fc4 100644 > --- a/drivers/cxl/security.c > +++ b/drivers/cxl/security.c > @@ -111,6 +111,31 @@ static int cxl_pmem_security_freeze(struct nvdimm *nvdimm) > return cxl_mbox_send_cmd(cxlds, CXL_MBOX_OP_FREEZE_SECURITY, NULL, 0, NULL, 0); > } > > +static bool cxl_has_invalidate_memregion(struct cxl_nvdimm *cxl_nvd) > +{ > + if (!cpu_cache_has_invalidate_memregion()) { > + if (IS_ENABLED(CONFIG_NVDIMM_SECURITY_TEST)) { > + dev_warn_once(&cxl_nvd->dev, > + "Bypassing cpu_cache_has_invalidate_memregion() check for testing!\n"); > + return true; > + } > + return false; > + } > + > + return true; > +} > + > +static void cxl_invalidate_memregion(struct cxl_nvdimm *cxl_nvd) > +{ > + if (IS_ENABLED(CONFIG_NVDIMM_SECURITY_TEST)) { > + dev_warn_once(&cxl_nvd->dev, > + "Bypassing cpu_cache_invalidate_memergion() for testing!\n"); > + return; > + } > + > + cpu_cache_invalidate_memregion(IORES_DESC_PERSISTENT_MEMORY); > +} > + > static int cxl_pmem_security_unlock(struct nvdimm *nvdimm, > const struct nvdimm_key_data *key_data) > { > @@ -120,7 +145,7 @@ static int cxl_pmem_security_unlock(struct nvdimm *nvdimm, > u8 pass[NVDIMM_PASSPHRASE_LEN]; > int rc; > > - if (!cpu_cache_has_invalidate_memregion()) > + if (!cxl_has_invalidate_memregion(cxl_nvd)) > return -EINVAL; > > memcpy(pass, key_data->data, NVDIMM_PASSPHRASE_LEN); > @@ -130,7 +155,7 @@ static int cxl_pmem_security_unlock(struct nvdimm *nvdimm, > return rc; > > /* DIMM unlocked, invalidate all CPU caches before we read it */ > - cpu_cache_invalidate_memregion(IORES_DESC_PERSISTENT_MEMORY); > + cxl_invalidate_memregion(cxl_nvd); > return 0; > } > > @@ -144,21 +169,21 @@ static int cxl_pmem_security_passphrase_erase(struct nvdimm *nvdimm, > struct cxl_pass_erase erase; > int rc; > > - if (!cpu_cache_has_invalidate_memregion()) > + if (!cxl_has_invalidate_memregion(cxl_nvd)) > return -EINVAL; > > erase.type = ptype == NVDIMM_MASTER ? > CXL_PMEM_SEC_PASS_MASTER : CXL_PMEM_SEC_PASS_USER; > memcpy(erase.pass, key->data, NVDIMM_PASSPHRASE_LEN); > /* Flush all cache before we erase mem device */ > - cpu_cache_invalidate_memregion(IORES_DESC_PERSISTENT_MEMORY); > + cxl_invalidate_memregion(cxl_nvd); > rc = cxl_mbox_send_cmd(cxlds, CXL_MBOX_OP_PASSPHRASE_SECURE_ERASE, > &erase, sizeof(erase), NULL, 0); > if (rc < 0) > return rc; > > /* mem device erased, invalidate all CPU caches before data is read */ > - cpu_cache_invalidate_memregion(IORES_DESC_PERSISTENT_MEMORY); > + cxl_invalidate_memregion(cxl_nvd); > return 0; > } > > >
diff --git a/drivers/cxl/security.c b/drivers/cxl/security.c index cbd005ceb091..2b5088af8fc4 100644 --- a/drivers/cxl/security.c +++ b/drivers/cxl/security.c @@ -111,6 +111,31 @@ static int cxl_pmem_security_freeze(struct nvdimm *nvdimm) return cxl_mbox_send_cmd(cxlds, CXL_MBOX_OP_FREEZE_SECURITY, NULL, 0, NULL, 0); } +static bool cxl_has_invalidate_memregion(struct cxl_nvdimm *cxl_nvd) +{ + if (!cpu_cache_has_invalidate_memregion()) { + if (IS_ENABLED(CONFIG_NVDIMM_SECURITY_TEST)) { + dev_warn_once(&cxl_nvd->dev, + "Bypassing cpu_cache_has_invalidate_memregion() check for testing!\n"); + return true; + } + return false; + } + + return true; +} + +static void cxl_invalidate_memregion(struct cxl_nvdimm *cxl_nvd) +{ + if (IS_ENABLED(CONFIG_NVDIMM_SECURITY_TEST)) { + dev_warn_once(&cxl_nvd->dev, + "Bypassing cpu_cache_invalidate_memergion() for testing!\n"); + return; + } + + cpu_cache_invalidate_memregion(IORES_DESC_PERSISTENT_MEMORY); +} + static int cxl_pmem_security_unlock(struct nvdimm *nvdimm, const struct nvdimm_key_data *key_data) { @@ -120,7 +145,7 @@ static int cxl_pmem_security_unlock(struct nvdimm *nvdimm, u8 pass[NVDIMM_PASSPHRASE_LEN]; int rc; - if (!cpu_cache_has_invalidate_memregion()) + if (!cxl_has_invalidate_memregion(cxl_nvd)) return -EINVAL; memcpy(pass, key_data->data, NVDIMM_PASSPHRASE_LEN); @@ -130,7 +155,7 @@ static int cxl_pmem_security_unlock(struct nvdimm *nvdimm, return rc; /* DIMM unlocked, invalidate all CPU caches before we read it */ - cpu_cache_invalidate_memregion(IORES_DESC_PERSISTENT_MEMORY); + cxl_invalidate_memregion(cxl_nvd); return 0; } @@ -144,21 +169,21 @@ static int cxl_pmem_security_passphrase_erase(struct nvdimm *nvdimm, struct cxl_pass_erase erase; int rc; - if (!cpu_cache_has_invalidate_memregion()) + if (!cxl_has_invalidate_memregion(cxl_nvd)) return -EINVAL; erase.type = ptype == NVDIMM_MASTER ? CXL_PMEM_SEC_PASS_MASTER : CXL_PMEM_SEC_PASS_USER; memcpy(erase.pass, key->data, NVDIMM_PASSPHRASE_LEN); /* Flush all cache before we erase mem device */ - cpu_cache_invalidate_memregion(IORES_DESC_PERSISTENT_MEMORY); + cxl_invalidate_memregion(cxl_nvd); rc = cxl_mbox_send_cmd(cxlds, CXL_MBOX_OP_PASSPHRASE_SECURE_ERASE, &erase, sizeof(erase), NULL, 0); if (rc < 0) return rc; /* mem device erased, invalidate all CPU caches before data is read */ - cpu_cache_invalidate_memregion(IORES_DESC_PERSISTENT_MEMORY); + cxl_invalidate_memregion(cxl_nvd); return 0; }
Bypass cpu_cache_invalidate_memregion() and checks when doing testing using CONFIG_NVDIMM_SECURITY_TEST flag. The bypass allows testing on QEMU where cpu_cache_has_invalidate_memregion() fails. Usage of cpu_cache_invalidate_memregion() is not needed for cxl_test security testing. Signed-off-by: Dave Jiang <dave.jiang@intel.com> --- drivers/cxl/security.c | 35 ++++++++++++++++++++++++++++++----- 1 file changed, 30 insertions(+), 5 deletions(-)