On 3/22/19 2:43 PM, Dan Williams wrote:
> On Fri, Mar 22, 2019 at 2:33 PM Dave Jiang <[email protected]> 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 <[email protected]>
>> ---
>> 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.
_______________________________________________
Linux-nvdimm mailing list
[email protected]
https://lists.01.org/mailman/listinfo/linux-nvdimm
