| Message ID | 166792842000.3767969.18296572896453551207.stgit@djiang5-desk3.ch.intel.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Introduce security commands for CXL pmem device | expand |
On Tue, 08 Nov 2022 10:27:00 -0700 Dave Jiang <dave.jiang@intel.com> wrote: > nfit_test overrode the security_show() sysfs attribute function in nvdimm > dimm_devs in order to allow testing of security unlock. With the > introduction of CXL security commands, the trick to override > security_show() becomes significantly more complicated. By introdcing a > security flag CONFIG_NVDIMM_SECURITY_TEST, libnvdimm can just toggle the > check via a compile option. In addition the original override can can be > removed from tools/testing/nvdimm/. > > Signed-off-by: Dave Jiang <dave.jiang@intel.com> Trivial comment inline you may want to address. Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com> > --- > drivers/nvdimm/Kconfig | 13 +++++++++++++ > drivers/nvdimm/dimm_devs.c | 9 ++++++++- > drivers/nvdimm/security.c | 4 ++++ > tools/testing/nvdimm/Kbuild | 1 - > tools/testing/nvdimm/dimm_devs.c | 30 ------------------------------ > 5 files changed, 25 insertions(+), 32 deletions(-) > delete mode 100644 tools/testing/nvdimm/dimm_devs.c > > diff --git a/drivers/nvdimm/Kconfig b/drivers/nvdimm/Kconfig > index 5a29046e3319..0a13c53c926f 100644 > --- a/drivers/nvdimm/Kconfig > +++ b/drivers/nvdimm/Kconfig > @@ -114,4 +114,17 @@ config NVDIMM_TEST_BUILD > core devm_memremap_pages() implementation and other > infrastructure. > > +config NVDIMM_SECURITY_TEST > + bool "Nvdimm security test code toggle" > + depends on NVDIMM_KEYS > + help > + Debug flag for security testing when using nfit_test or cxl_test > + modules in tools/testing/. > + > + Select Y if using nfit_test or cxl_test for security testing. > + Accidentally selecting this option when not using cxl_test I'd drop the "Accidentally" Selecting this option when ... > + introduces 1 mailbox request to the CXL device to get security > + status for DIMM unlock operation or sysfs attribute "security" > + read. > + > endif > diff --git a/drivers/nvdimm/dimm_devs.c b/drivers/nvdimm/dimm_devs.c > index c7c980577491..1fc081dcf631 100644 > --- a/drivers/nvdimm/dimm_devs.c > +++ b/drivers/nvdimm/dimm_devs.c > @@ -349,11 +349,18 @@ static ssize_t available_slots_show(struct device *dev, > } > static DEVICE_ATTR_RO(available_slots); > > -__weak ssize_t security_show(struct device *dev, > +ssize_t security_show(struct device *dev, > struct device_attribute *attr, char *buf) > { > struct nvdimm *nvdimm = to_nvdimm(dev); > > + /* > + * For the test version we need to poll the "hardware" in order > + * to get the updated status for unlock testing. > + */ > + if (IS_ENABLED(CONFIG_NVDIMM_SECURITY_TEST)) > + nvdimm->sec.flags = nvdimm_security_flags(nvdimm, NVDIMM_USER); > + > if (test_bit(NVDIMM_SECURITY_OVERWRITE, &nvdimm->sec.flags)) > return sprintf(buf, "overwrite\n"); > if (test_bit(NVDIMM_SECURITY_DISABLED, &nvdimm->sec.flags)) > diff --git a/drivers/nvdimm/security.c b/drivers/nvdimm/security.c > index 92af4c3ca0d3..12a3926f4289 100644 > --- a/drivers/nvdimm/security.c > +++ b/drivers/nvdimm/security.c > @@ -177,6 +177,10 @@ static int __nvdimm_security_unlock(struct nvdimm *nvdimm) > || !nvdimm->sec.flags) > return -EIO; > > + /* While nfit_test does not need this, cxl_test does */ > + if (IS_ENABLED(CONFIG_NVDIMM_SECURITY_TEST)) > + nvdimm->sec.flags = nvdimm_security_flags(nvdimm, NVDIMM_USER); > + > /* No need to go further if security is disabled */ > if (test_bit(NVDIMM_SECURITY_DISABLED, &nvdimm->sec.flags)) > return 0; > diff --git a/tools/testing/nvdimm/Kbuild b/tools/testing/nvdimm/Kbuild > index 5eb5c23b062f..8153251ea389 100644 > --- a/tools/testing/nvdimm/Kbuild > +++ b/tools/testing/nvdimm/Kbuild > @@ -79,7 +79,6 @@ libnvdimm-$(CONFIG_BTT) += $(NVDIMM_SRC)/btt_devs.o > libnvdimm-$(CONFIG_NVDIMM_PFN) += $(NVDIMM_SRC)/pfn_devs.o > libnvdimm-$(CONFIG_NVDIMM_DAX) += $(NVDIMM_SRC)/dax_devs.o > libnvdimm-$(CONFIG_NVDIMM_KEYS) += $(NVDIMM_SRC)/security.o > -libnvdimm-y += dimm_devs.o > libnvdimm-y += libnvdimm_test.o > libnvdimm-y += config_check.o > > diff --git a/tools/testing/nvdimm/dimm_devs.c b/tools/testing/nvdimm/dimm_devs.c > deleted file mode 100644 > index 57bd27dedf1f..000000000000 > --- a/tools/testing/nvdimm/dimm_devs.c > +++ /dev/null > @@ -1,30 +0,0 @@ > -// SPDX-License-Identifier: GPL-2.0 > -/* Copyright Intel Corp. 2018 */ > -#include <linux/init.h> > -#include <linux/module.h> > -#include <linux/moduleparam.h> > -#include <linux/nd.h> > -#include "pmem.h" > -#include "pfn.h" > -#include "nd.h" > -#include "nd-core.h" > - > -ssize_t security_show(struct device *dev, > - struct device_attribute *attr, char *buf) > -{ > - struct nvdimm *nvdimm = to_nvdimm(dev); > - > - /* > - * For the test version we need to poll the "hardware" in order > - * to get the updated status for unlock testing. > - */ > - nvdimm->sec.flags = nvdimm_security_flags(nvdimm, NVDIMM_USER); > - > - if (test_bit(NVDIMM_SECURITY_DISABLED, &nvdimm->sec.flags)) > - return sprintf(buf, "disabled\n"); > - if (test_bit(NVDIMM_SECURITY_UNLOCKED, &nvdimm->sec.flags)) > - return sprintf(buf, "unlocked\n"); > - if (test_bit(NVDIMM_SECURITY_LOCKED, &nvdimm->sec.flags)) > - return sprintf(buf, "locked\n"); > - return -ENOTTY; > -} > >
diff --git a/drivers/nvdimm/Kconfig b/drivers/nvdimm/Kconfig index 5a29046e3319..0a13c53c926f 100644 --- a/drivers/nvdimm/Kconfig +++ b/drivers/nvdimm/Kconfig @@ -114,4 +114,17 @@ config NVDIMM_TEST_BUILD core devm_memremap_pages() implementation and other infrastructure. +config NVDIMM_SECURITY_TEST + bool "Nvdimm security test code toggle" + depends on NVDIMM_KEYS + help + Debug flag for security testing when using nfit_test or cxl_test + modules in tools/testing/. + + Select Y if using nfit_test or cxl_test for security testing. + Accidentally selecting this option when not using cxl_test + introduces 1 mailbox request to the CXL device to get security + status for DIMM unlock operation or sysfs attribute "security" + read. + endif diff --git a/drivers/nvdimm/dimm_devs.c b/drivers/nvdimm/dimm_devs.c index c7c980577491..1fc081dcf631 100644 --- a/drivers/nvdimm/dimm_devs.c +++ b/drivers/nvdimm/dimm_devs.c @@ -349,11 +349,18 @@ static ssize_t available_slots_show(struct device *dev, } static DEVICE_ATTR_RO(available_slots); -__weak ssize_t security_show(struct device *dev, +ssize_t security_show(struct device *dev, struct device_attribute *attr, char *buf) { struct nvdimm *nvdimm = to_nvdimm(dev); + /* + * For the test version we need to poll the "hardware" in order + * to get the updated status for unlock testing. + */ + if (IS_ENABLED(CONFIG_NVDIMM_SECURITY_TEST)) + nvdimm->sec.flags = nvdimm_security_flags(nvdimm, NVDIMM_USER); + if (test_bit(NVDIMM_SECURITY_OVERWRITE, &nvdimm->sec.flags)) return sprintf(buf, "overwrite\n"); if (test_bit(NVDIMM_SECURITY_DISABLED, &nvdimm->sec.flags)) diff --git a/drivers/nvdimm/security.c b/drivers/nvdimm/security.c index 92af4c3ca0d3..12a3926f4289 100644 --- a/drivers/nvdimm/security.c +++ b/drivers/nvdimm/security.c @@ -177,6 +177,10 @@ static int __nvdimm_security_unlock(struct nvdimm *nvdimm) || !nvdimm->sec.flags) return -EIO; + /* While nfit_test does not need this, cxl_test does */ + if (IS_ENABLED(CONFIG_NVDIMM_SECURITY_TEST)) + nvdimm->sec.flags = nvdimm_security_flags(nvdimm, NVDIMM_USER); + /* No need to go further if security is disabled */ if (test_bit(NVDIMM_SECURITY_DISABLED, &nvdimm->sec.flags)) return 0; diff --git a/tools/testing/nvdimm/Kbuild b/tools/testing/nvdimm/Kbuild index 5eb5c23b062f..8153251ea389 100644 --- a/tools/testing/nvdimm/Kbuild +++ b/tools/testing/nvdimm/Kbuild @@ -79,7 +79,6 @@ libnvdimm-$(CONFIG_BTT) += $(NVDIMM_SRC)/btt_devs.o libnvdimm-$(CONFIG_NVDIMM_PFN) += $(NVDIMM_SRC)/pfn_devs.o libnvdimm-$(CONFIG_NVDIMM_DAX) += $(NVDIMM_SRC)/dax_devs.o libnvdimm-$(CONFIG_NVDIMM_KEYS) += $(NVDIMM_SRC)/security.o -libnvdimm-y += dimm_devs.o libnvdimm-y += libnvdimm_test.o libnvdimm-y += config_check.o diff --git a/tools/testing/nvdimm/dimm_devs.c b/tools/testing/nvdimm/dimm_devs.c deleted file mode 100644 index 57bd27dedf1f..000000000000 --- a/tools/testing/nvdimm/dimm_devs.c +++ /dev/null @@ -1,30 +0,0 @@ -// SPDX-License-Identifier: GPL-2.0 -/* Copyright Intel Corp. 2018 */ -#include <linux/init.h> -#include <linux/module.h> -#include <linux/moduleparam.h> -#include <linux/nd.h> -#include "pmem.h" -#include "pfn.h" -#include "nd.h" -#include "nd-core.h" - -ssize_t security_show(struct device *dev, - struct device_attribute *attr, char *buf) -{ - struct nvdimm *nvdimm = to_nvdimm(dev); - - /* - * For the test version we need to poll the "hardware" in order - * to get the updated status for unlock testing. - */ - nvdimm->sec.flags = nvdimm_security_flags(nvdimm, NVDIMM_USER); - - if (test_bit(NVDIMM_SECURITY_DISABLED, &nvdimm->sec.flags)) - return sprintf(buf, "disabled\n"); - if (test_bit(NVDIMM_SECURITY_UNLOCKED, &nvdimm->sec.flags)) - return sprintf(buf, "unlocked\n"); - if (test_bit(NVDIMM_SECURITY_LOCKED, &nvdimm->sec.flags)) - return sprintf(buf, "locked\n"); - return -ENOTTY; -}
nfit_test overrode the security_show() sysfs attribute function in nvdimm dimm_devs in order to allow testing of security unlock. With the introduction of CXL security commands, the trick to override security_show() becomes significantly more complicated. By introdcing a security flag CONFIG_NVDIMM_SECURITY_TEST, libnvdimm can just toggle the check via a compile option. In addition the original override can can be removed from tools/testing/nvdimm/. Signed-off-by: Dave Jiang <dave.jiang@intel.com> --- drivers/nvdimm/Kconfig | 13 +++++++++++++ drivers/nvdimm/dimm_devs.c | 9 ++++++++- drivers/nvdimm/security.c | 4 ++++ tools/testing/nvdimm/Kbuild | 1 - tools/testing/nvdimm/dimm_devs.c | 30 ------------------------------ 5 files changed, 25 insertions(+), 32 deletions(-) delete mode 100644 tools/testing/nvdimm/dimm_devs.c