[PATCH] nvdimm: security: allow secure erase to execute without key

[PATCH] nvdimm: security: allow secure erase to execute without key

[Dave Jiang]

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@intel.com>
---
 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) {
 		nd_cmd->status = ND_INTEL_STATUS_INVALID_STATE;
 		dev_dbg(dev, "secure erase: wrong security state\n");
 	} else if (memcmp(nd_cmd->passphrase, sec->passphrase,

_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

Re: [PATCH] nvdimm: security: allow secure erase to execute without key

[Dan Williams]

On Fri, Mar 22, 2019 at 2:33 PM Dave Jiang <dave.jiang@intel.com> 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@intel.com>
> ---
>  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?
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

Re: [PATCH] nvdimm: security: allow secure erase to execute without key

[Dave Jiang]

On 3/22/19 2:43 PM, Dan Williams wrote:
> On Fri, Mar 22, 2019 at 2:33 PM Dave Jiang <dave.jiang@intel.com> 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@intel.com>
>> ---
>>  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
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

Re: [PATCH] nvdimm: security: allow secure erase to execute without key

[Dan Williams]

On Fri, Mar 22, 2019 at 2:58 PM Dave Jiang <dave.jiang@intel.com> wrote:
>
>
>
> On 3/22/19 2:43 PM, Dan Williams wrote:
> > On Fri, Mar 22, 2019 at 2:33 PM Dave Jiang <dave.jiang@intel.com> 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@intel.com>
> >> ---
> >>  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.

I'd rather this be explicitly about accepting zero when the security
state is not enabled. Rather than accepting anything, because we've
seen zero be chosen as a default passphrase in some platforms.
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm