Adding support to allow secure erase to happen when security state is not enabled. Key data of 0's will be passed in.

Signed-off-by: Dave Jiang <dave.jiang(a)intel.com&gt; --- drivers/nvdimm/security.c | 17 ++++++++++++----- tools/testing/nvdimm/test/nfit.c | 3 +-- 2 files changed, 13 insertions(+), 7 deletions(-) diff --git a/drivers/nvdimm/security.c b/drivers/nvdimm/security.c index f8bb746a549f..b7bd26030964 100644 --- a/drivers/nvdimm/security.c +++ b/drivers/nvdimm/security.c @@ -286,8 +286,9 @@ int nvdimm_security_erase(struct nvdimm *nvdimm, unsigned int keyid, { struct device *dev = &nvdimm->dev; struct nvdimm_bus *nvdimm_bus = walk_to_nvdimm_bus(dev); - struct key *key; + struct key *key = NULL; int rc; + char *data, dummy_key[NVDIMM_PASSPHRASE_LEN];

/* The bus lock should be held at the top level of the call stack */ lockdep_assert_held(&nvdimm_bus->reconfig_mutex); @@ -319,11 +320,17 @@ int nvdimm_security_erase(struct nvdimm *nvdimm, unsigned int keyid, return -EOPNOTSUPP; } - key = nvdimm_lookup_user_key(nvdimm, keyid, NVDIMM_BASE_KEY); - if (!key) - return -ENOKEY; + if (keyid != 0) { + key = nvdimm_lookup_user_key(nvdimm, keyid, NVDIMM_BASE_KEY); + if (!key) + return -ENOKEY; + data = key_data(key); + } else { + memset(dummy_key, 0, NVDIMM_PASSPHRASE_LEN);

+ data = dummy_key;

+ } - rc = nvdimm->sec.ops->erase(nvdimm, key_data(key), pass_type); + rc = nvdimm->sec.ops->erase(nvdimm, (void *)data, pass_type); dev_dbg(dev, "key: %d erase%s: %s\n", key_serial(key), pass_type == NVDIMM_MASTER ? "(master)" : "(user)", rc == 0 ? "success" : "fail"); diff --git a/tools/testing/nvdimm/test/nfit.c b/tools/testing/nvdimm/test/nfit.c index b579f962451d..9351a81ea945 100644 --- a/tools/testing/nvdimm/test/nfit.c +++ b/tools/testing/nvdimm/test/nfit.c @@ -1059,8 +1059,7 @@ static int nd_intel_test_cmd_secure_erase(struct nfit_test *t, struct device *dev = &t->pdev.dev; struct nfit_test_sec *sec = &dimm_sec_info[dimm]; - if (!(sec->state & ND_INTEL_SEC_STATE_ENABLED) || - (sec->state & ND_INTEL_SEC_STATE_FROZEN)) { + if (sec->state & ND_INTEL_SEC_STATE_FROZEN) {

On 3/22/19 2:43 PM, Dan Williams wrote: > On Fri, Mar 22, 2019 at 2:33 PM Dave Jiang <dave.jiang(a)intel.com&gt; wrote: >> >> Adding support to allow secure erase to happen when security state is not >> enabled. Key data of 0's will be passed in. > > I think I want to change this wording and patch title to say > "libnvdimm/security: Support a zero-key for secure-erase". Because we > are still passing a key and the kernel interface requires the key-id > parameter, we're just arranging for a special key to be used. > >> >> Signed-off-by: Dave Jiang <dave.jiang(a)intel.com&gt; >> --- >> drivers/nvdimm/security.c | 17 ++++++++++++----- >> tools/testing/nvdimm/test/nfit.c | 3 +-- >> 2 files changed, 13 insertions(+), 7 deletions(-) >> >> diff --git a/drivers/nvdimm/security.c b/drivers/nvdimm/security.c >> index f8bb746a549f..b7bd26030964 100644 >> --- a/drivers/nvdimm/security.c >> +++ b/drivers/nvdimm/security.c >> @@ -286,8 +286,9 @@ int nvdimm_security_erase(struct nvdimm *nvdimm, unsigned int keyid, >> { >> struct device *dev = &nvdimm->dev; >> struct nvdimm_bus *nvdimm_bus = walk_to_nvdimm_bus(dev); >> - struct key *key; >> + struct key *key = NULL; >> int rc; >> + char *data, dummy_key[NVDIMM_PASSPHRASE_LEN]; > > Let's make this > > static const char zero_key[NVDIMM_PASSPHRASE_LEN]; > > ...and make it global. > >> >> /* The bus lock should be held at the top level of the call stack */ >> lockdep_assert_held(&nvdimm_bus->reconfig_mutex); >> @@ -319,11 +320,17 @@ int nvdimm_security_erase(struct nvdimm *nvdimm, unsigned int keyid, >> return -EOPNOTSUPP; >> } >> >> - key = nvdimm_lookup_user_key(nvdimm, keyid, NVDIMM_BASE_KEY); >> - if (!key) >> - return -ENOKEY; >> + if (keyid != 0) { >> + key = nvdimm_lookup_user_key(nvdimm, keyid, NVDIMM_BASE_KEY); >> + if (!key) >> + return -ENOKEY; >> + data = key_data(key); >> + } else { >> + memset(dummy_key, 0, NVDIMM_PASSPHRASE_LEN); > > ...with the above change no need to do a memset. > >> + data = dummy_key; > > There may be hardware that actually expects zeroes so I'd rather be > explicit, because if it was truly "dummy" there would be no need to > initialize it. > >> + } >> >> - rc = nvdimm->sec.ops->erase(nvdimm, key_data(key), pass_type); >> + rc = nvdimm->sec.ops->erase(nvdimm, (void *)data, pass_type); >> dev_dbg(dev, "key: %d erase%s: %s\n", key_serial(key), >> pass_type == NVDIMM_MASTER ? "(master)" : "(user)", >> rc == 0 ? "success" : "fail"); >> diff --git a/tools/testing/nvdimm/test/nfit.c b/tools/testing/nvdimm/test/nfit.c >> index b579f962451d..9351a81ea945 100644 >> --- a/tools/testing/nvdimm/test/nfit.c >> +++ b/tools/testing/nvdimm/test/nfit.c >> @@ -1059,8 +1059,7 @@ static int nd_intel_test_cmd_secure_erase(struct nfit_test *t, >> struct device *dev = &t->pdev.dev; >> struct nfit_test_sec *sec = &dimm_sec_info[dimm]; >> >> - if (!(sec->state & ND_INTEL_SEC_STATE_ENABLED) || >> - (sec->state & ND_INTEL_SEC_STATE_FROZEN)) { >> + if (sec->state & ND_INTEL_SEC_STATE_FROZEN) { > > What does this have to do with the new zero-key? > Because otherwise we will reject the op when security is not enabled. The whole point of this was to support secure erase when security is not enabled.