[PATCH 1/5] libnvdimm: fix updating of kernel key during nvdimm key update

[PATCH 1/5] libnvdimm: fix updating of kernel key during nvdimm key update

[Dave Jiang]

There are several issues WRT kernel key update when we are doing nvdimm
security key update.

1. The kernel key created needs to have proper permission for update
2. We need to check key_update() return value and make sure it didn't fail
3. We need to not hold the key->sem when calling key_update() since it will
   call down_write() when doing modification to the key.

Signed-off-by: Dave Jiang <dave.jiang@intel.com>
---
 drivers/nvdimm/security.c |   16 ++++++++++++----
 1 file changed, 12 insertions(+), 4 deletions(-)

diff --git a/drivers/nvdimm/security.c b/drivers/nvdimm/security.c
index 776c440a02ef..ef83bdf47c31 100644
--- a/drivers/nvdimm/security.c
+++ b/drivers/nvdimm/security.c
@@ -27,7 +27,7 @@ static struct key *make_kernel_key(struct key *key)
 
 	new_key = key_alloc(&key_type_logon, key->description,
 			GLOBAL_ROOT_UID, GLOBAL_ROOT_GID, current_cred(),
-			KEY_POS_SEARCH, KEY_ALLOC_NOT_IN_QUOTA, NULL);
+			KEY_POS_ALL, KEY_ALLOC_NOT_IN_QUOTA, NULL);
 	if (IS_ERR(new_key))
 		return NULL;
 
@@ -419,11 +419,19 @@ int nvdimm_security_change_key(struct nvdimm *nvdimm,
 		dev_warn(dev, "key update failed: %d\n", rc);
 
 	if (old_key) {
+		up_read(&old_key->sem);
 		/* copy new payload to old payload */
-		if (rc == 0)
-			key_update(make_key_ref(old_key, 1), new_data,
+		if (rc == 0) {
+			rc = key_update(make_key_ref(old_key, 1), new_data,
 					old_key->datalen);
-		up_read(&old_key->sem);
+			if (rc < 0) {
+				dev_warn(dev,
+					"kernel key update failed: %d\n", rc);
+				key_invalidate(old_key);
+				key_put(old_key);
+				nvdimm->key = NULL;
+			}
+		}
 	}
 	up_read(&key->sem);
 

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

[PATCH 2/5] libnvdimm: fix incorrect output when nvdimm disable failed

[Dave Jiang]

Fix inocrrect dev_warn() in nvdimm_security_disable().

Signed-off-by: Dave Jiang <dave.jiang@intel.com>
---
 drivers/nvdimm/security.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/nvdimm/security.c b/drivers/nvdimm/security.c
index ef83bdf47c31..cf934c6025d9 100644
--- a/drivers/nvdimm/security.c
+++ b/drivers/nvdimm/security.c
@@ -256,7 +256,7 @@ int nvdimm_security_disable(struct nvdimm *nvdimm, unsigned int keyid)
 			(const struct nvdimm_key_data *)payload->data);
 	up_read(&key->sem);
 	if (rc < 0) {
-		dev_warn(dev, "unlock failed\n");
+		dev_warn(dev, "nvdimm disable failed\n");
 		goto out;
 	}
 

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

Re: [PATCH 2/5] libnvdimm: fix incorrect output when nvdimm disable failed

[Dan Williams]

On Fri, Oct 12, 2018 at 11:23 AM Dave Jiang <dave.jiang@intel.com> wrote:
>
> Fix inocrrect dev_warn() in nvdimm_security_disable().
>
> Signed-off-by: Dave Jiang <dave.jiang@intel.com>
> ---
>  drivers/nvdimm/security.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/nvdimm/security.c b/drivers/nvdimm/security.c
> index ef83bdf47c31..cf934c6025d9 100644
> --- a/drivers/nvdimm/security.c
> +++ b/drivers/nvdimm/security.c
> @@ -256,7 +256,7 @@ int nvdimm_security_disable(struct nvdimm *nvdimm, unsigned int keyid)
>                         (const struct nvdimm_key_data *)payload->data);
>         up_read(&key->sem);
>         if (rc < 0) {
> -               dev_warn(dev, "unlock failed\n");
> +               dev_warn(dev, "nvdimm disable failed\n");

Let's make this "security disable failed\n", the 'nvdimm' is redundant
with the "nvdimm: nmemX:" that will be added by the dev_warn().
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

[PATCH 3/5] libnvdimm: remove driver attached check for secure erase

[Dave Jiang]

We should be able to do secure erase when dimm is idle but not disabled.
Remove check for driver attachment.

Signed-off-by: Dave Jiang <dave.jiang@intel.com>
---
 drivers/nvdimm/security.c |    6 ------
 1 file changed, 6 deletions(-)

diff --git a/drivers/nvdimm/security.c b/drivers/nvdimm/security.c
index cf934c6025d9..f9ca1575012e 100644
--- a/drivers/nvdimm/security.c
+++ b/drivers/nvdimm/security.c
@@ -148,12 +148,6 @@ int nvdimm_security_erase(struct nvdimm *nvdimm, unsigned int keyid)
 		goto out;
 	}
 
-	if (dev_get_drvdata(dev)) {
-		dev_warn(dev, "Unable to secure erase while DIMM enabled.\n");
-		rc = -EBUSY;
-		goto out;
-	}
-
 	if (nvdimm->state == NVDIMM_SECURITY_UNSUPPORTED) {
 		dev_warn(dev, "Attempt to secure erase in wrong state.\n");
 		rc = -EOPNOTSUPP;

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

Re: [PATCH 3/5] libnvdimm: remove driver attached check for secure erase

[Dan Williams]

On Fri, Oct 12, 2018 at 11:24 AM Dave Jiang <dave.jiang@intel.com> wrote:
>
> We should be able to do secure erase when dimm is idle but not disabled.
> Remove check for driver attachment.
>
> Signed-off-by: Dave Jiang <dave.jiang@intel.com>

Yup, looks good.

Since there's no dependency on the other patches in this series I'll
just go ahead and apply this.
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

[PATCH 4/5] libnvdimm: remove code to pull user key when there's no kernel key

[Dave Jiang]

Remove extraneous code that used to expect nvdimm_get_and_verify_key() to
return NULL when there's no kernel key. We want to enforce the behavior
that when there is no kernel key we should fail security ops.

Signed-off-by: Dave Jiang <dave.jiang@intel.com>
---
 drivers/nvdimm/security.c |   35 ++++-------------------------------
 1 file changed, 4 insertions(+), 31 deletions(-)

diff --git a/drivers/nvdimm/security.c b/drivers/nvdimm/security.c
index f9ca1575012e..7b5d7c77514d 100644
--- a/drivers/nvdimm/security.c
+++ b/drivers/nvdimm/security.c
@@ -135,7 +135,6 @@ int nvdimm_security_erase(struct nvdimm *nvdimm, unsigned int keyid)
 	struct key *key;
 	struct user_key_payload *payload;
 	struct device *dev = &nvdimm->dev;
-	bool is_userkey = false;
 
 	if (!nvdimm->security_ops)
 		return -EOPNOTSUPP;
@@ -161,18 +160,6 @@ int nvdimm_security_erase(struct nvdimm *nvdimm, unsigned int keyid)
 		rc = PTR_ERR(key);
 		goto out;
 	}
-	if (!key) {
-		dev_dbg(dev, "No cached key found\n");
-		/* get old user key */
-		key = nvdimm_lookup_user_key(dev, keyid);
-		if (!key) {
-			dev_dbg(dev, "Unable to retrieve user key: %#x\n",
-					keyid);
-			rc = -ENOKEY;
-			goto out;
-		}
-		is_userkey = true;
-	}
 
 	down_read(&key->sem);
 	payload = key->payload.data[0];
@@ -181,10 +168,8 @@ int nvdimm_security_erase(struct nvdimm *nvdimm, unsigned int keyid)
 	up_read(&key->sem);
 
 	/* remove key since secure erase kills the passphrase */
-	if (!is_userkey) {
-		key_invalidate(key);
-		nvdimm->key = NULL;
-	}
+	key_invalidate(key);
+	nvdimm->key = NULL;
 	key_put(key);
 
  out:
@@ -218,7 +203,6 @@ int nvdimm_security_disable(struct nvdimm *nvdimm, unsigned int keyid)
 	struct key *key;
 	struct user_key_payload *payload;
 	struct device *dev = &nvdimm->dev;
-	bool is_userkey = false;
 
 	if (!nvdimm->security_ops)
 		return -EOPNOTSUPP;
@@ -233,15 +217,6 @@ int nvdimm_security_disable(struct nvdimm *nvdimm, unsigned int keyid)
 		mutex_unlock(&nvdimm->key_mutex);
 		return PTR_ERR(key);
 	}
-	if (!key) {
-		/* get old user key */
-		key = nvdimm_lookup_user_key(dev, keyid);
-		if (!key) {
-			mutex_unlock(&nvdimm->key_mutex);
-			return -ENOKEY;
-		}
-		is_userkey = true;
-	}
 
 	down_read(&key->sem);
 	payload = key->payload.data[0];
@@ -255,10 +230,8 @@ int nvdimm_security_disable(struct nvdimm *nvdimm, unsigned int keyid)
 	}
 
 	/* If we succeed then remove the key */
-	if (!is_userkey) {
-		key_invalidate(key);
-		nvdimm->key = NULL;
-	}
+	key_invalidate(key);
+	nvdimm->key = NULL;
 	key_put(key);
 
  out:

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

Re: [PATCH 4/5] libnvdimm: remove code to pull user key when there's no kernel key

[Dan Williams]

On Fri, Oct 12, 2018 at 11:24 AM Dave Jiang <dave.jiang@intel.com> wrote:
>
> Remove extraneous code that used to expect nvdimm_get_and_verify_key() to
> return NULL when there's no kernel key. We want to enforce the behavior
> that when there is no kernel key we should fail security ops.
>
> Signed-off-by: Dave Jiang <dave.jiang@intel.com>
> ---
>  drivers/nvdimm/security.c |   35 ++++-------------------------------
>  1 file changed, 4 insertions(+), 31 deletions(-)
>
> diff --git a/drivers/nvdimm/security.c b/drivers/nvdimm/security.c
> index f9ca1575012e..7b5d7c77514d 100644
> --- a/drivers/nvdimm/security.c
> +++ b/drivers/nvdimm/security.c
> @@ -135,7 +135,6 @@ int nvdimm_security_erase(struct nvdimm *nvdimm, unsigned int keyid)
>         struct key *key;
>         struct user_key_payload *payload;
>         struct device *dev = &nvdimm->dev;
> -       bool is_userkey = false;
>
>         if (!nvdimm->security_ops)
>                 return -EOPNOTSUPP;
> @@ -161,18 +160,6 @@ int nvdimm_security_erase(struct nvdimm *nvdimm, unsigned int keyid)
>                 rc = PTR_ERR(key);
>                 goto out;
>         }
> -       if (!key) {
> -               dev_dbg(dev, "No cached key found\n");
> -               /* get old user key */
> -               key = nvdimm_lookup_user_key(dev, keyid);
> -               if (!key) {
> -                       dev_dbg(dev, "Unable to retrieve user key: %#x\n",
> -                                       keyid);
> -                       rc = -ENOKEY;
> -                       goto out;
> -               }
> -               is_userkey = true;
> -       }
>
>         down_read(&key->sem);
>         payload = key->payload.data[0];
> @@ -181,10 +168,8 @@ int nvdimm_security_erase(struct nvdimm *nvdimm, unsigned int keyid)
>         up_read(&key->sem);
>
>         /* remove key since secure erase kills the passphrase */
> -       if (!is_userkey) {
> -               key_invalidate(key);
> -               nvdimm->key = NULL;
> -       }
> +       key_invalidate(key);
> +       nvdimm->key = NULL;
>         key_put(key);
>
>   out:
> @@ -218,7 +203,6 @@ int nvdimm_security_disable(struct nvdimm *nvdimm, unsigned int keyid)
>         struct key *key;
>         struct user_key_payload *payload;
>         struct device *dev = &nvdimm->dev;
> -       bool is_userkey = false;
>
>         if (!nvdimm->security_ops)
>                 return -EOPNOTSUPP;
> @@ -233,15 +217,6 @@ int nvdimm_security_disable(struct nvdimm *nvdimm, unsigned int keyid)
>                 mutex_unlock(&nvdimm->key_mutex);
>                 return PTR_ERR(key);
>         }
> -       if (!key) {
> -               /* get old user key */
> -               key = nvdimm_lookup_user_key(dev, keyid);
> -               if (!key) {
> -                       mutex_unlock(&nvdimm->key_mutex);
> -                       return -ENOKEY;
> -               }
> -               is_userkey = true;
> -       }
>
>         down_read(&key->sem);
>         payload = key->payload.data[0];
> @@ -255,10 +230,8 @@ int nvdimm_security_disable(struct nvdimm *nvdimm, unsigned int keyid)
>         }
>
>         /* If we succeed then remove the key */
> -       if (!is_userkey) {
> -               key_invalidate(key);
> -               nvdimm->key = NULL;
> -       }
> +       key_invalidate(key);
> +       nvdimm->key = NULL;
>         key_put(key);

Looks good, perhaps just use the key_destroy() helper here?
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

[PATCH 5/5] libnvdimm: address state where dimm is unlocked in preOS

[Dave Jiang]

When the nvdimm security state is unlocked during unlock, we skip the
operation. In this state, we are not able to fetch a key for verification
and at the same time the dimm is unlocked. This prevents us from doing
any security operations. We will send the freeze security DSM to make the
state consistent.

Signed-off-by: Dave Jiang <dave.jiang@intel.com>
---
 drivers/nvdimm/security.c |   15 +++++++++++++--
 1 file changed, 13 insertions(+), 2 deletions(-)

diff --git a/drivers/nvdimm/security.c b/drivers/nvdimm/security.c
index 7b5d7c77514d..6c5423228b31 100644
--- a/drivers/nvdimm/security.c
+++ b/drivers/nvdimm/security.c
@@ -250,8 +250,19 @@ int nvdimm_security_unlock_dimm(struct nvdimm *nvdimm)
 	if (!nvdimm->security_ops)
 		return 0;
 
-	if (nvdimm->state == NVDIMM_SECURITY_UNLOCKED ||
-			nvdimm->state == NVDIMM_SECURITY_UNSUPPORTED ||
+	/*
+	 * If the pre-OS has unlocked the DIMM, we will not be able to
+	 * verify the key against the hardware. Therefore we will not
+	 * retrieve the key and will freeze the security config. This will
+	 * prevent any other security operations.
+	 */
+	if (nvdimm->state == NVDIMM_SECURITY_UNLOCKED) {
+		rc = nvdimm_security_freeze_lock(nvdimm);
+		if (rc < 0)
+			return rc;
+	}
+
+	if (nvdimm->state == NVDIMM_SECURITY_UNSUPPORTED ||
 			nvdimm->state == NVDIMM_SECURITY_DISABLED)
 		return 0;
 

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

Re: [PATCH 5/5] libnvdimm: address state where dimm is unlocked in preOS

[Dan Williams]

On Fri, Oct 12, 2018 at 11:24 AM Dave Jiang <dave.jiang@intel.com> wrote:
>
> When the nvdimm security state is unlocked during unlock, we skip the
> operation. In this state, we are not able to fetch a key for verification
> and at the same time the dimm is unlocked. This prevents us from doing
> any security operations. We will send the freeze security DSM to make the
> state consistent.
>
> Signed-off-by: Dave Jiang <dave.jiang@intel.com>
> ---
>  drivers/nvdimm/security.c |   15 +++++++++++++--
>  1 file changed, 13 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/nvdimm/security.c b/drivers/nvdimm/security.c
> index 7b5d7c77514d..6c5423228b31 100644
> --- a/drivers/nvdimm/security.c
> +++ b/drivers/nvdimm/security.c
> @@ -250,8 +250,19 @@ int nvdimm_security_unlock_dimm(struct nvdimm *nvdimm)
>         if (!nvdimm->security_ops)
>                 return 0;
>
> -       if (nvdimm->state == NVDIMM_SECURITY_UNLOCKED ||
> -                       nvdimm->state == NVDIMM_SECURITY_UNSUPPORTED ||
> +       /*
> +        * If the pre-OS has unlocked the DIMM, we will not be able to
> +        * verify the key against the hardware. Therefore we will not
> +        * retrieve the key and will freeze the security config. This will
> +        * prevent any other security operations.
> +        */

I think we should try to retrieve the key if the DIMM is unlocked and
verify it with a 'change-key-to-self' check. If either of those steps
fail then freeze the dimm.
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

Re: [PATCH 1/5] libnvdimm: fix updating of kernel key during nvdimm key update

[Dan Williams]

On Fri, Oct 12, 2018 at 11:23 AM Dave Jiang <dave.jiang@intel.com> wrote:
>
> There are several issues WRT kernel key update when we are doing nvdimm
> security key update.
>
> 1. The kernel key created needs to have proper permission for update
> 2. We need to check key_update() return value and make sure it didn't fail
> 3. We need to not hold the key->sem when calling key_update() since it will
>    call down_write() when doing modification to the key.
>
> Signed-off-by: Dave Jiang <dave.jiang@intel.com>
> ---
>  drivers/nvdimm/security.c |   16 ++++++++++++----
>  1 file changed, 12 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/nvdimm/security.c b/drivers/nvdimm/security.c
> index 776c440a02ef..ef83bdf47c31 100644
> --- a/drivers/nvdimm/security.c
> +++ b/drivers/nvdimm/security.c
> @@ -27,7 +27,7 @@ static struct key *make_kernel_key(struct key *key)
>
>         new_key = key_alloc(&key_type_logon, key->description,
>                         GLOBAL_ROOT_UID, GLOBAL_ROOT_GID, current_cred(),
> -                       KEY_POS_SEARCH, KEY_ALLOC_NOT_IN_QUOTA, NULL);
> +                       KEY_POS_ALL, KEY_ALLOC_NOT_IN_QUOTA, NULL);

Should that be (KEY_POS_ALL & ~KEY_POS_SETATTR)? I don't see a reason
why we would ever change key attributes.

>         if (IS_ERR(new_key))
>                 return NULL;
>
> @@ -419,11 +419,19 @@ int nvdimm_security_change_key(struct nvdimm *nvdimm,
>                 dev_warn(dev, "key update failed: %d\n", rc);
>
>         if (old_key) {
> +               up_read(&old_key->sem);
>                 /* copy new payload to old payload */

Let's delete that comment and replace it with one that talks about the
locking rules. Namely that we are done with the old_key payload after
->change_key() returns and that key_update() will take the write lock
on the payload to update it.

> -               if (rc == 0)
> -                       key_update(make_key_ref(old_key, 1), new_data,
> +               if (rc == 0) {
> +                       rc = key_update(make_key_ref(old_key, 1), new_data,
>                                         old_key->datalen);
> -               up_read(&old_key->sem);
> +                       if (rc < 0) {
> +                               dev_warn(dev,
> +                                       "kernel key update failed: %d\n", rc);
> +                               key_invalidate(old_key);
> +                               key_put(old_key);

I added a local 'key_destroy()' helper for that invalidate+put pattern.
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm