[PATCH v2 0/4] misc patches for nvdimm security fixes

[PATCH v2 0/4] misc patches for nvdimm security fixes

[Dave Jiang]

v2:
- Fix key perm flags and remove SET_ATTR. (Dan)
- Fixup comment for key_update code block (Dan)
- Use key_destroy() instead of key_invalidate+key_put (Dan)
- Fix dev_warn() output for nvdimm_security_disable() (Dan)
- Drop patch 3/5 (applied by Dan)
- Add support for self verify the requested key before giving up and freeze
  security. (Dan)

---

Dave Jiang (4):
      libnvdimm: fix updating of kernel key during nvdimm key update
      libnvdimm: fix incorrect output when nvdimm disable failed
      libnvdimm: remove code to pull user key when there's no kernel key
      libnvdimm: address state where dimm is unlocked in preOS


 drivers/nvdimm/security.c |  129 +++++++++++++++++++++++++++++----------------
 1 file changed, 82 insertions(+), 47 deletions(-)

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

[PATCH v2 1/4] 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 |   23 ++++++++++++++++++-----
 1 file changed, 18 insertions(+), 5 deletions(-)

diff --git a/drivers/nvdimm/security.c b/drivers/nvdimm/security.c
index 2e764abe015a..8de34b03d402 100644
--- a/drivers/nvdimm/security.c
+++ b/drivers/nvdimm/security.c
@@ -27,7 +27,8 @@ 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_POS_SETATTR,
+			KEY_ALLOC_NOT_IN_QUOTA, NULL);
 	if (IS_ERR(new_key))
 		return NULL;
 
@@ -413,11 +414,23 @@ int nvdimm_security_change_key(struct nvdimm *nvdimm,
 		dev_warn(dev, "key update failed: %d\n", rc);
 
 	if (old_key) {
-		/* copy new payload to old payload */
-		if (rc == 0)
-			key_update(make_key_ref(old_key, 1), new_data,
-					old_key->datalen);
 		up_read(&old_key->sem);
+		/*
+		 * With the key update done via hardware, we no longer need
+		 * the old payload and need to replace it with the new
+		 * payload. key_update() will acquire write sem of the
+		 * old key and update with new data.
+		 */
+		if (rc == 0) {
+			rc = key_update(make_key_ref(old_key, 1), new_data,
+					old_key->datalen);
+			if (rc < 0) {
+				dev_warn(dev,
+					"kernel key update failed: %d\n", rc);
+				key_destroy(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 v2 2/4] 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 8de34b03d402..2de5ef107216 100644
--- a/drivers/nvdimm/security.c
+++ b/drivers/nvdimm/security.c
@@ -251,7 +251,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, "security disable failed\n");
 		goto out;
 	}
 

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

[PATCH v2 3/4] 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 |   49 +++++++++------------------------------------
 1 file changed, 10 insertions(+), 39 deletions(-)

diff --git a/drivers/nvdimm/security.c b/drivers/nvdimm/security.c
index 2de5ef107216..eb778667cd93 100644
--- a/drivers/nvdimm/security.c
+++ b/drivers/nvdimm/security.c
@@ -122,6 +122,12 @@ struct key *nvdimm_get_and_verify_key(struct nvdimm *nvdimm,
 	return key;
 }
 
+static void key_destroy(struct key *key)
+{
+	key_invalidate(key);
+	key_put(key);
+}
+
 int nvdimm_security_get_state(struct nvdimm *nvdimm)
 {
 	if (!nvdimm->security_ops)
@@ -136,7 +142,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;
@@ -162,18 +167,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];
@@ -182,11 +175,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_put(key);
+	key_destroy(key);
+	nvdimm->key = NULL;
 
  out:
 	mutex_unlock(&nvdimm->key_mutex);
@@ -219,7 +209,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;
@@ -234,15 +223,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];
@@ -256,11 +236,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_put(key);
+	key_destroy(key);
+	nvdimm->key = NULL;
 
  out:
 	mutex_unlock(&nvdimm->key_mutex);
@@ -330,12 +307,6 @@ void nvdimm_security_release(struct nvdimm *nvdimm)
 	mutex_unlock(&nvdimm->key_mutex);
 }
 
-static void key_destroy(struct key *key)
-{
-	key_invalidate(key);
-	key_put(key);
-}
-
 int nvdimm_security_change_key(struct nvdimm *nvdimm,
 		unsigned int old_keyid, unsigned int new_keyid)
 {

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

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

[Dave Jiang]

When the nvdimm security state is unlocked during unlock, we will do a
request_key() and verify the key against the hardware. If we fail, we
will freeze the security configuration.

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

diff --git a/drivers/nvdimm/security.c b/drivers/nvdimm/security.c
index eb778667cd93..3a905c58a935 100644
--- a/drivers/nvdimm/security.c
+++ b/drivers/nvdimm/security.c
@@ -245,6 +245,42 @@ int nvdimm_security_disable(struct nvdimm *nvdimm, unsigned int keyid)
 	return rc;
 }
 
+static int nvdimm_self_verify_key(struct nvdimm *nvdimm)
+{
+	struct key *key;
+	struct user_key_payload *payload;
+	void *data;
+	int rc;
+
+	lockdep_assert_held(&nvdimm->key_mutex);
+
+	key = nvdimm_request_key(nvdimm);
+	if (!key)
+		return -ENOKEY;
+
+	if (key->datalen != NVDIMM_PASSPHRASE_LEN) {
+		key_put(key);
+		return -EINVAL;
+	}
+
+	down_read(&key->sem);
+	payload = key->payload.data[0];
+	data = payload->data;
+
+	/*
+	 * We send the same key to the hardware as new and old key to
+	 * verify that the key is good.
+	 */
+	rc = nvdimm->security_ops->change_key(nvdimm, data, data);
+	if (rc < 0) {
+		key_put(key);
+		return rc;
+	}
+	up_read(&key->sem);
+	nvdimm->key = key;
+	return 0;
+}
+
 int nvdimm_security_unlock_dimm(struct nvdimm *nvdimm)
 {
 	struct key *key;
@@ -255,12 +291,27 @@ 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 (nvdimm->state == NVDIMM_SECURITY_UNSUPPORTED ||
 			nvdimm->state == NVDIMM_SECURITY_DISABLED)
 		return 0;
 
 	mutex_lock(&nvdimm->key_mutex);
+	/*
+	 * If the pre-OS has unlocked the DIMM, we will attempt to send
+	 * the key from request_key() to the hardware for verification.
+	 * If we are not able to verify the key against the hardware we
+	 * will freeze the security configuration. This will prevent any
+	 * other security operations.
+	 */
+	if (nvdimm->state == NVDIMM_SECURITY_UNLOCKED) {
+		rc = nvdimm_self_verify_key(nvdimm);
+		if (rc < 0) {
+			rc = nvdimm_security_freeze_lock(nvdimm);
+			mutex_unlock(&nvdimm->key_mutex);
+			return rc;
+		}
+	}
+
 	key = nvdimm->key;
 	if (!key) {
 		key = nvdimm_request_key(nvdimm);

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

Re: [PATCH v2 0/4] misc patches for nvdimm security fixes

[Dan Williams]

On Fri, Oct 12, 2018 at 1:39 PM Dave Jiang <dave.jiang@intel.com> wrote:
>
> v2:
> - Fix key perm flags and remove SET_ATTR. (Dan)
> - Fixup comment for key_update code block (Dan)
> - Use key_destroy() instead of key_invalidate+key_put (Dan)
> - Fix dev_warn() output for nvdimm_security_disable() (Dan)
> - Drop patch 3/5 (applied by Dan)
> - Add support for self verify the requested key before giving up and freeze
>   security. (Dan)
>
> ---
>
> Dave Jiang (4):
>       libnvdimm: fix updating of kernel key during nvdimm key update
>       libnvdimm: fix incorrect output when nvdimm disable failed
>       libnvdimm: remove code to pull user key when there's no kernel key
>       libnvdimm: address state where dimm is unlocked in preOS
>

These look good to me, applied.
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm