[PATCH 00/11] Additional patches for nvdimm security support

[PATCH 00/11] Additional patches for nvdimm security support

[Dave Jiang]

The following series adds additional support for nvdimm security.
1. Converted logon keys to encrypted-keys.
2. Add overwrite DSM support
3. Add DSM 1.8 master passphrase support

The patch series is based off the branch from here:
https://git.kernel.org/pub/scm/linux/kernel/git/nvdimm/nvdimm.git/log/?h=for-5.0/nvdimm-security

Instead of squashing the previous changes, they are kept for history purposes
to document how we arrived to the current iteration.

Mimi,
Patch 1 requires your ack as it makes changes to encrypted-keys by adding
the nvdimm key format type. Dan wanted to restrict the key to 32bytes
during creation. I also wouldn't mind if you take a look at patch 2 and
make sure I'm providing correct usage of encrypted keys. Thank you!

---

Dave Jiang (11):
      keys-encrypted: add nvdimm key format type to encrypted keys
      libnvdimm/security: change clear text nvdimm keys to encrypted keys
      libnvdimm/security: add override module param for key self verification
      libnvdimm/security: introduce NDD_SECURITY_BUSY flag
      acpi/nfit, libnvdimm/security: Add security DSM overwrite support
      tools/testing/nvdimm: Add overwrite support for nfit_test
      libnvdimm/security: add overwrite status notification
      libnvdimm/security: add documentation for ovewrite
      acpi/nfit, libnvdimm/security: add Intel DSM 1.8 master passphrase support
      tools/testing/nvdimm: add Intel DSM 1.8 support for nfit_test
      acpi/nfit: prevent indiscriminate DSM payload dumping for security DSMs


 Documentation/nvdimm/security.txt        |   68 +++-
 drivers/acpi/nfit/Kconfig                |    7 
 drivers/acpi/nfit/core.c                 |   31 ++
 drivers/acpi/nfit/intel.c                |  245 +++++++++++++++
 drivers/acpi/nfit/intel.h                |   22 +
 drivers/acpi/nfit/nfit.h                 |    7 
 drivers/nvdimm/core.c                    |    3 
 drivers/nvdimm/dimm.c                    |    3 
 drivers/nvdimm/dimm_devs.c               |   50 +++
 drivers/nvdimm/nd-core.h                 |    9 -
 drivers/nvdimm/nd.h                      |   19 +
 drivers/nvdimm/region_devs.c             |    7 
 drivers/nvdimm/security.c                |  496 +++++++++++++++++-------------
 include/linux/libnvdimm.h                |   22 +
 security/keys/encrypted-keys/encrypted.c |   29 +-
 tools/testing/nvdimm/dimm_devs.c         |    2 
 tools/testing/nvdimm/test/nfit.c         |  141 +++++++++
 17 files changed, 895 insertions(+), 266 deletions(-)

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

[PATCH 01/11] keys-encrypted: add nvdimm key format type to encrypted keys

[Dave Jiang]

Adding nvdimm key format type to encrypted keys in order to limit the size
of the key to 32bytes.

Signed-off-by: Dave Jiang <dave.jiang@intel.com>
---
 security/keys/encrypted-keys/encrypted.c |   29 ++++++++++++++++++++---------
 1 file changed, 20 insertions(+), 9 deletions(-)

diff --git a/security/keys/encrypted-keys/encrypted.c b/security/keys/encrypted-keys/encrypted.c
index d92cbf9687c3..182b4f136bdf 100644
--- a/security/keys/encrypted-keys/encrypted.c
+++ b/security/keys/encrypted-keys/encrypted.c
@@ -45,6 +45,7 @@ static const char hmac_alg[] = "hmac(sha256)";
 static const char blkcipher_alg[] = "cbc(aes)";
 static const char key_format_default[] = "default";
 static const char key_format_ecryptfs[] = "ecryptfs";
+static const char key_format_nvdimm[] = "nvdimm";
 static unsigned int ivsize;
 static int blksize;
 
@@ -54,6 +55,7 @@ static int blksize;
 #define HASH_SIZE SHA256_DIGEST_SIZE
 #define MAX_DATA_SIZE 4096
 #define MIN_DATA_SIZE  20
+#define KEY_NVDIMM_PAYLOAD_LEN 32
 
 static struct crypto_shash *hash_tfm;
 
@@ -62,12 +64,13 @@ enum {
 };
 
 enum {
-	Opt_error = -1, Opt_default, Opt_ecryptfs
+	Opt_error = -1, Opt_default, Opt_ecryptfs, Opt_nvdimm
 };
 
 static const match_table_t key_format_tokens = {
 	{Opt_default, "default"},
 	{Opt_ecryptfs, "ecryptfs"},
+	{Opt_nvdimm, "nvdimm"},
 	{Opt_error, NULL}
 };
 
@@ -195,6 +198,7 @@ static int datablob_parse(char *datablob, const char **format,
 	key_format = match_token(p, key_format_tokens, args);
 	switch (key_format) {
 	case Opt_ecryptfs:
+	case Opt_nvdimm:
 	case Opt_default:
 		*format = p;
 		*master_desc = strsep(&datablob, " \t");
@@ -625,15 +629,22 @@ static struct encrypted_key_payload *encrypted_key_alloc(struct key *key,
 	format_len = (!format) ? strlen(key_format_default) : strlen(format);
 	decrypted_datalen = dlen;
 	payload_datalen = decrypted_datalen;
-	if (format && !strcmp(format, key_format_ecryptfs)) {
-		if (dlen != ECRYPTFS_MAX_KEY_BYTES) {
-			pr_err("encrypted_key: keylen for the ecryptfs format "
-			       "must be equal to %d bytes\n",
-			       ECRYPTFS_MAX_KEY_BYTES);
-			return ERR_PTR(-EINVAL);
+	if (format) {
+		if (!strcmp(format, key_format_ecryptfs)) {
+			if (dlen != ECRYPTFS_MAX_KEY_BYTES) {
+				pr_err("encrypted_key: keylen for the ecryptfs format must be equal to %d bytes\n",
+					ECRYPTFS_MAX_KEY_BYTES);
+				return ERR_PTR(-EINVAL);
+			}
+			decrypted_datalen = ECRYPTFS_MAX_KEY_BYTES;
+			payload_datalen = sizeof(struct ecryptfs_auth_tok);
+		} else if (!strcmp(format, key_format_nvdimm)) {
+			if (decrypted_datalen != KEY_NVDIMM_PAYLOAD_LEN) {
+				pr_err("encrypted_key: nvdimm key payload incorrect length: %d\n",
+						decrypted_datalen);
+				return ERR_PTR(-EINVAL);
+			}
 		}
-		decrypted_datalen = ECRYPTFS_MAX_KEY_BYTES;
-		payload_datalen = sizeof(struct ecryptfs_auth_tok);
 	}
 
 	encrypted_datalen = roundup(decrypted_datalen, blksize);

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

[PATCH 02/11] libnvdimm/security: change clear text nvdimm keys to encrypted keys

[Dave Jiang]

In order to make nvdimm more secure, encrypted keys will be used instead of
clear text keys. A master key will be created to seal encrypted nvdimm
keys. The master key can be a trusted key generated from TPM 2.0 or a less
secure user key.

In the process of this conversion, the kernel cached key will be removed
in order to simplify the verification process. The hardware will be used to
verify the decrypted user payload directly.

Signed-off-by: Dave Jiang <dave.jiang@intel.com>
---
 Documentation/nvdimm/security.txt |   29 ++-
 drivers/nvdimm/dimm.c             |    3 
 drivers/nvdimm/dimm_devs.c        |    2 
 drivers/nvdimm/nd-core.h          |    3 
 drivers/nvdimm/nd.h               |    5 -
 drivers/nvdimm/security.c         |  316 ++++++++++---------------------------
 6 files changed, 108 insertions(+), 250 deletions(-)

diff --git a/Documentation/nvdimm/security.txt b/Documentation/nvdimm/security.txt
index 50cbb6cb96a1..11240ce48755 100644
--- a/Documentation/nvdimm/security.txt
+++ b/Documentation/nvdimm/security.txt
@@ -39,34 +39,39 @@ The DIMM id would be provided along with the key payload (passphrase) to
 the kernel.
 
 The security keys are managed on the basis of a single key per DIMM. The
-key "passphrase" is expected to be 32bytes long or padded to 32bytes. This is
+key "passphrase" is expected to be 32bytes long. This is
 similar to the ATA security specification [2]. A key is initially acquired
 via the request_key() kernel API call and retrieved from userspace. It is up to
 the user to provide an upcall application to retrieve the key in whatever
 fashion meets their security requirements.
 
-A nvdimm user logon key has the description format of:
+A nvdimm encrypted key has the description format of:
 nvdimm:<bus-provider-specific-unique-id>
 
+See Documentation/security/keys/trusted-encrypted.rst for details on encrypted
+keys. The encrypted key for the DIMM is generated from a master key that is
+either a trusted key created via TPM2 or a less secure user key. The payload
+for the nvdimm encrypted key is randomly generated by the kernel and is not
+visible to the user. The user is responsible for retaining the encrypted key
+blob generated from the encrypted key to be loaded at the next boot session.
+
 4. Unlocking
 ------------
 When the DIMMs are being enumerated by the kernel, the kernel will attempt to
-retrieve the key from its keyring. If that fails, it will attempt to
 acquire the key from the userspace upcall function. This is the only time
 a locked DIMM can be unlocked. Once unlocked, the DIMM will remain unlocked
 until reboot.
 
 5. Update
 ---------
-When doing an update, it is expected that the new key with the 64bit payload of
-format described above is added via the keyutils API or utility. The update
-command written to the sysfs attribute will be with the format:
+When doing an update, it is expected that the new key is added via the
+keyutils API or utility. The update command written to the sysfs attribute
+will be with the format:
 update <old keyid> <new keyid>
 
-It is expected that a user logon key has been injected via keyutils to provide
-the payload for the update operation. The kernel will take the new user key,
-attempt the update operation with the nvdimm, and replace the existing key's
-payload with the new passphrase.
+It is expected that a encrypted key has been injected via keyutils to provide
+the payload for the update operation. The kernel will take the new user key
+and the old user key to attempt the update operation.
 
 If there is no old key id due to a security enabling, then a 0 should be
 passed in.  If a nvdimm has an existing passphrase, then an "old" key should
@@ -80,7 +85,7 @@ frozen by a user with root privelege.
 7. Disable
 ----------
 The security disable command format is:
-disable <old keyid>
+disable <current keyid>
 
 An "old" key with the passphrase payload that is tied to the nvdimm should be
 injected with a key description that does not have the "nvdimm:" prefix and
@@ -89,7 +94,7 @@ its keyid should be passed in via sysfs.
 8. Secure Erase
 ---------------
 The command format for doing a secure erase is:
-erase <old keyid>
+erase <current keyid>
 
 An "old" key with the passphrase payload that is tied to the nvdimm should be
 injected with a key description that does not have the "nvdimm:" prefix and
diff --git a/drivers/nvdimm/dimm.c b/drivers/nvdimm/dimm.c
index 225a8474c44a..c3886a22b5e4 100644
--- a/drivers/nvdimm/dimm.c
+++ b/drivers/nvdimm/dimm.c
@@ -112,9 +112,6 @@ static int nvdimm_probe(struct device *dev)
 static int nvdimm_remove(struct device *dev)
 {
 	struct nvdimm_drvdata *ndd = dev_get_drvdata(dev);
-	struct nvdimm *nvdimm = to_nvdimm(dev);
-
-	nvdimm_security_release(nvdimm);
 
 	if (!ndd)
 		return 0;
diff --git a/drivers/nvdimm/dimm_devs.c b/drivers/nvdimm/dimm_devs.c
index 5d34251c4f3b..269f401a5e68 100644
--- a/drivers/nvdimm/dimm_devs.c
+++ b/drivers/nvdimm/dimm_devs.c
@@ -485,7 +485,7 @@ struct nvdimm *__nvdimm_create(struct nvdimm_bus *nvdimm_bus, void *provider_dat
 	nvdimm->num_flush = num_flush;
 	nvdimm->flush_wpq = flush_wpq;
 	atomic_set(&nvdimm->busy, 0);
-	mutex_init(&nvdimm->key_mutex);
+	mutex_init(&nvdimm->sec_mutex);
 	dev = &nvdimm->dev;
 	dev_set_name(dev, "nmem%d", nvdimm->id);
 	dev->parent = &nvdimm_bus->dev;
diff --git a/drivers/nvdimm/nd-core.h b/drivers/nvdimm/nd-core.h
index 6e2877996a9b..cfb87f71db8d 100644
--- a/drivers/nvdimm/nd-core.h
+++ b/drivers/nvdimm/nd-core.h
@@ -45,8 +45,7 @@ struct nvdimm {
 	const char *dimm_id;
 	const struct nvdimm_security_ops *security_ops;
 	enum nvdimm_security_state state;
-	struct key *key;
-	struct mutex key_mutex;
+	struct mutex sec_mutex;
 };
 
 /**
diff --git a/drivers/nvdimm/nd.h b/drivers/nvdimm/nd.h
index 892f127bfd38..f8d8f0a2a40d 100644
--- a/drivers/nvdimm/nd.h
+++ b/drivers/nvdimm/nd.h
@@ -428,7 +428,6 @@ bool pmem_should_map_pages(struct device *dev);
 
 #ifdef CONFIG_NVDIMM_SECURITY
 int nvdimm_security_unlock_dimm(struct nvdimm *nvdimm);
-void nvdimm_security_release(struct nvdimm *nvdimm);
 int nvdimm_security_get_state(struct nvdimm *nvdimm);
 int nvdimm_security_change_key(struct nvdimm *nvdimm, unsigned int old_keyid,
 		unsigned int new_keyid);
@@ -441,10 +440,6 @@ static inline int nvdimm_security_unlock_dimm(struct nvdimm *nvdimm)
 	return 0;
 }
 
-static inline void nvdimm_security_release(struct nvdimm *nvdimm)
-{
-}
-
 static inline int nvdimm_security_get_state(struct nvdimm *nvdimm)
 {
 	return -EOPNOTSUPP;
diff --git a/drivers/nvdimm/security.c b/drivers/nvdimm/security.c
index 5aacc590b4c0..ee741199d623 100644
--- a/drivers/nvdimm/security.c
+++ b/drivers/nvdimm/security.c
@@ -10,43 +10,10 @@
 #include <linux/key.h>
 #include <linux/key-type.h>
 #include <keys/user-type.h>
+#include <keys/encrypted-type.h>
 #include "nd-core.h"
 #include "nd.h"
 
-/*
- * Replacing the user key with a kernel key. The function expects that
- * we hold the sem for the key passed in. The function will release that
- * sem when done process. We will also hold the sem for the valid new key
- * returned.
- */
-static struct key *make_kernel_key(struct key *key)
-{
-	struct key *new_key;
-	struct user_key_payload *payload;
-	int rc;
-
-	new_key = key_alloc(&key_type_logon, key->description,
-			GLOBAL_ROOT_UID, GLOBAL_ROOT_GID, current_cred(),
-			KEY_POS_ALL & ~KEY_POS_SETATTR,
-			KEY_ALLOC_NOT_IN_QUOTA, NULL);
-	if (IS_ERR(new_key))
-		return NULL;
-
-	payload = key->payload.data[0];
-	rc = key_instantiate_and_link(new_key, payload->data,
-			payload->datalen, NULL, NULL);
-	up_read(&key->sem);
-	if (rc < 0) {
-		key_put(new_key);
-		return NULL;
-	}
-
-	key_put(key);
-
-	down_read(&new_key->sem);
-	return new_key;
-}
-
 /*
  * Retrieve user injected key
  */
@@ -61,6 +28,10 @@ static struct key *nvdimm_lookup_user_key(struct device *dev,
 		return NULL;
 
 	key = key_ref_to_ptr(keyref);
+	if (key->type != &key_type_encrypted) {
+		key_put(key);
+		return NULL;
+	}
 	dev_dbg(dev, "%s: key found: %#x\n", __func__, key_serial(key));
 
 	return key;
@@ -76,7 +47,7 @@ static struct key *nvdimm_request_key(struct nvdimm *nvdimm)
 	struct device *dev = &nvdimm->dev;
 
 	sprintf(desc, "%s%s", NVDIMM_PREFIX, nvdimm->dimm_id);
-	key = request_key(&key_type_logon, desc, "");
+	key = request_key(&key_type_encrypted, desc, "");
 	if (IS_ERR(key)) {
 		if (PTR_ERR(key) == -ENOKEY)
 			dev_warn(dev, "request_key() found no key\n");
@@ -88,52 +59,6 @@ static struct key *nvdimm_request_key(struct nvdimm *nvdimm)
 	return key;
 }
 
-struct key *nvdimm_get_and_verify_key(struct nvdimm *nvdimm,
-		unsigned int user_key_id)
-{
-	int rc;
-	struct key *user_key, *key;
-	struct device *dev = &nvdimm->dev;
-	struct user_key_payload *upayload, *payload;
-
-	lockdep_assert_held(&nvdimm->key_mutex);
-	key = nvdimm->key;
-	if (!key) {
-		dev_dbg(dev, "No cached kernel key\n");
-		return ERR_PTR(-EAGAIN);;
-	}
-	dev_dbg(dev, "cached_key: %#x\n", key_serial(key));
-
-	user_key = nvdimm_lookup_user_key(dev, user_key_id);
-	if (!user_key) {
-		dev_dbg(dev, "Old user key lookup failed\n");
-		return ERR_PTR(-EINVAL);
-	}
-	dev_dbg(dev, "user_key: %#x\n", key_serial(user_key));
-
-	down_read(&key->sem);
-	down_read(&user_key->sem);
-	payload = key->payload.data[0];
-	upayload = user_key->payload.data[0];
-
-	rc = memcmp(payload->data, upayload->data, NVDIMM_PASSPHRASE_LEN);
-	up_read(&user_key->sem);
-	key_put(user_key);
-	up_read(&key->sem);
-
-	if (rc != 0) {
-		dev_warn(dev, "Supplied old user key fails check.\n");
-		return ERR_PTR(-EINVAL);
-	}
-	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)
@@ -146,14 +71,15 @@ int nvdimm_security_erase(struct nvdimm *nvdimm, unsigned int keyid)
 {
 	int rc = 0;
 	struct key *key;
-	struct user_key_payload *payload;
+	struct encrypted_key_payload *epayload;
 	struct device *dev = &nvdimm->dev;
+	void *data;
 
 	if (!nvdimm->security_ops)
 		return -EOPNOTSUPP;
 
 	nvdimm_bus_lock(dev);
-	mutex_lock(&nvdimm->key_mutex);
+	mutex_lock(&nvdimm->sec_mutex);
 	if (atomic_read(&nvdimm->busy)) {
 		dev_warn(dev, "Unable to secure erase while DIMM active.\n");
 		rc = -EBUSY;
@@ -166,26 +92,23 @@ int nvdimm_security_erase(struct nvdimm *nvdimm, unsigned int keyid)
 		goto out;
 	}
 
-	/* look for a key from cached key if exists */
-	key = nvdimm_get_and_verify_key(nvdimm, keyid);
-	if (IS_ERR(key)) {
+	key = nvdimm_lookup_user_key(dev, keyid);
+	if (!key) {
 		dev_dbg(dev, "Unable to get and verify key\n");
-		rc = PTR_ERR(key);
+		rc = -ENOKEY;
 		goto out;
 	}
 
 	down_read(&key->sem);
-	payload = key->payload.data[0];
+	epayload = dereference_key_locked(key);
+	data = epayload->decrypted_data;
 	rc = nvdimm->security_ops->erase(nvdimm,
-			(const struct nvdimm_key_data *)payload->data);
+			(const struct nvdimm_key_data *)data);
 	up_read(&key->sem);
-
-	/* remove key since secure erase kills the passphrase */
-	key_destroy(key);
-	nvdimm->key = NULL;
+	key_put(key);
 
  out:
-	mutex_unlock(&nvdimm->key_mutex);
+	mutex_unlock(&nvdimm->sec_mutex);
 	nvdimm_bus_unlock(dev);
 	nvdimm_security_get_state(nvdimm);
 	return rc;
@@ -201,11 +124,15 @@ int nvdimm_security_freeze_lock(struct nvdimm *nvdimm)
 	if (nvdimm->state == NVDIMM_SECURITY_UNSUPPORTED)
 		return -EOPNOTSUPP;
 
+	mutex_lock(&nvdimm->sec_mutex);
 	rc = nvdimm->security_ops->freeze_lock(nvdimm);
-	if (rc < 0)
+	if (rc < 0) {
+		mutex_unlock(&nvdimm->sec_mutex);
 		return rc;
+	}
 
 	nvdimm_security_get_state(nvdimm);
+	mutex_unlock(&nvdimm->sec_mutex);
 	return 0;
 }
 
@@ -213,8 +140,9 @@ int nvdimm_security_disable(struct nvdimm *nvdimm, unsigned int keyid)
 {
 	int rc;
 	struct key *key;
-	struct user_key_payload *payload;
+	struct encrypted_key_payload *epayload;
 	struct device *dev = &nvdimm->dev;
+	void *data;
 
 	if (!nvdimm->security_ops)
 		return -EOPNOTSUPP;
@@ -222,57 +150,52 @@ int nvdimm_security_disable(struct nvdimm *nvdimm, unsigned int keyid)
 	if (nvdimm->state == NVDIMM_SECURITY_UNSUPPORTED)
 		return -EOPNOTSUPP;
 
-	mutex_lock(&nvdimm->key_mutex);
+	mutex_lock(&nvdimm->sec_mutex);
 	/* look for a key from cached key */
-	key = nvdimm_get_and_verify_key(nvdimm, keyid);
-	if (IS_ERR(key)) {
-		mutex_unlock(&nvdimm->key_mutex);
-		return PTR_ERR(key);
+	key = nvdimm_lookup_user_key(dev, keyid);
+	if (!key) {
+		mutex_unlock(&nvdimm->sec_mutex);
+		return -ENOKEY;
 	}
 
 	down_read(&key->sem);
-	payload = key->payload.data[0];
+	epayload = dereference_key_locked(key);
+	data = epayload->decrypted_data;
 
 	rc = nvdimm->security_ops->disable(nvdimm,
-			(const struct nvdimm_key_data *)payload->data);
+			(const struct nvdimm_key_data *)data);
 	up_read(&key->sem);
-	if (rc < 0) {
+	if (rc < 0)
 		dev_warn(dev, "security disable failed\n");
-		goto out;
-	}
 
-	/* If we succeed then remove the key */
-	key_destroy(key);
-	nvdimm->key = NULL;
-
- out:
-	mutex_unlock(&nvdimm->key_mutex);
+	key_put(key);
 	nvdimm_security_get_state(nvdimm);
+	mutex_unlock(&nvdimm->sec_mutex);
 	return rc;
 }
 
-static int nvdimm_self_verify_key(struct nvdimm *nvdimm)
+static struct key *nvdimm_self_verify_key(struct nvdimm *nvdimm)
 {
 	struct key *key;
-	struct user_key_payload *payload;
+	struct encrypted_key_payload *epayload;
 	void *data;
 	int rc;
 
-	lockdep_assert_held(&nvdimm->key_mutex);
-
+	lockdep_assert_held(&nvdimm->sec_mutex);
 	key = nvdimm_request_key(nvdimm);
 	if (!key)
-		return -ENOKEY;
+		return NULL;
+
+	down_read(&key->sem);
+	epayload = dereference_key_locked(key);
+	data = epayload->decrypted_data;
 
-	if (key->datalen != NVDIMM_PASSPHRASE_LEN) {
+	if (epayload->decrypted_datalen != NVDIMM_PASSPHRASE_LEN) {
 		key_put(key);
-		return -EINVAL;
+		key = NULL;
+		goto out;
 	}
 
-	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.
@@ -280,19 +203,21 @@ static int nvdimm_self_verify_key(struct nvdimm *nvdimm)
 	rc = nvdimm->security_ops->change_key(nvdimm, data, data);
 	if (rc < 0) {
 		key_put(key);
-		return rc;
+		key = NULL;
 	}
+
+out:
 	up_read(&key->sem);
-	nvdimm->key = key;
-	return 0;
+	return key;
 }
 
 int nvdimm_security_unlock_dimm(struct nvdimm *nvdimm)
 {
-	struct key *key;
-	int rc = -ENXIO;
-	struct user_key_payload *payload;
+	struct key *key = NULL;
+	int rc;
+	struct encrypted_key_payload *epayload;
 	struct device *dev = &nvdimm->dev;
+	void *data;
 
 	if (!nvdimm->security_ops)
 		return 0;
@@ -301,7 +226,7 @@ int nvdimm_security_unlock_dimm(struct nvdimm *nvdimm)
 			nvdimm->state == NVDIMM_SECURITY_DISABLED)
 		return 0;
 
-	mutex_lock(&nvdimm->key_mutex);
+	mutex_lock(&nvdimm->sec_mutex);
 	/*
 	 * If the pre-OS has unlocked the DIMM, we will attempt to send
 	 * the key from request_key() to the hardware for verification.
@@ -310,60 +235,50 @@ int nvdimm_security_unlock_dimm(struct nvdimm *nvdimm)
 	 * other security operations.
 	 */
 	if (nvdimm->state == NVDIMM_SECURITY_UNLOCKED) {
-		rc = nvdimm_self_verify_key(nvdimm);
-		if (rc < 0) {
+		key = nvdimm_self_verify_key(nvdimm);
+		if (!key) {
 			rc = nvdimm_security_freeze_lock(nvdimm);
-			mutex_unlock(&nvdimm->key_mutex);
-			return rc;
+			mutex_unlock(&nvdimm->sec_mutex);
+			return -ENOKEY;
 		}
 	}
 
-	key = nvdimm->key;
-	if (!key) {
+	if (!key)
 		key = nvdimm_request_key(nvdimm);
-		if (key && key->datalen != NVDIMM_PASSPHRASE_LEN) {
-			key_put(key);
-			key = NULL;
-			rc = -EINVAL;
-		}
-	}
+
 	if (!key) {
-		mutex_unlock(&nvdimm->key_mutex);
-		return rc;
+		mutex_unlock(&nvdimm->sec_mutex);
+		return -ENOKEY;
 	}
 
 	dev_dbg(dev, "%s: key: %#x\n", __func__, key_serial(key));
 	down_read(&key->sem);
-	payload = key->payload.data[0];
+	epayload = dereference_key_locked(key);
+	data = epayload->decrypted_data;
+
+	if (epayload->decrypted_datalen != NVDIMM_PASSPHRASE_LEN) {
+		up_read(&key->sem);
+		key_put(key);
+		mutex_unlock(&nvdimm->sec_mutex);
+		return -EINVAL;
+	}
+
 	rc = nvdimm->security_ops->unlock(nvdimm,
-			(const struct nvdimm_key_data *)payload->data);
+			(const struct nvdimm_key_data *)data);
 	up_read(&key->sem);
 
 	if (rc == 0) {
-		if (!nvdimm->key)
-			nvdimm->key = key;
 		nvdimm->state = NVDIMM_SECURITY_UNLOCKED;
 		dev_dbg(dev, "DIMM %s unlocked\n", dev_name(dev));
-	} else {
-		key_invalidate(key);
-		key_put(key);
-		nvdimm->key = NULL;
+	} else
 		dev_warn(dev, "Failed to unlock dimm: %s\n", dev_name(dev));
-	}
 
-	mutex_unlock(&nvdimm->key_mutex);
+	key_put(key);
 	nvdimm_security_get_state(nvdimm);
+	mutex_unlock(&nvdimm->sec_mutex);
 	return rc;
 }
 
-void nvdimm_security_release(struct nvdimm *nvdimm)
-{
-	mutex_lock(&nvdimm->key_mutex);
-	key_put(nvdimm->key);
-	nvdimm->key = NULL;
-	mutex_unlock(&nvdimm->key_mutex);
-}
-
 int nvdimm_security_change_key(struct nvdimm *nvdimm,
 		unsigned int old_keyid, unsigned int new_keyid)
 {
@@ -371,7 +286,7 @@ int nvdimm_security_change_key(struct nvdimm *nvdimm,
 	struct key *key, *old_key;
 	void *old_data = NULL, *new_data;
 	struct device *dev = &nvdimm->dev;
-	struct user_key_payload *payload, *old_payload;
+	struct encrypted_key_payload *epayload, *old_epayload;
 
 	if (!nvdimm->security_ops)
 		return -EOPNOTSUPP;
@@ -379,15 +294,10 @@ int nvdimm_security_change_key(struct nvdimm *nvdimm,
 	if (nvdimm->state == NVDIMM_SECURITY_FROZEN)
 		return -EBUSY;
 
-	mutex_lock(&nvdimm->key_mutex);
+	mutex_lock(&nvdimm->sec_mutex);
 	/* look for a key from cached key if exists */
-	old_key = nvdimm_get_and_verify_key(nvdimm, old_keyid);
-	if (IS_ERR(old_key) && PTR_ERR(old_key) == -EAGAIN)
-		old_key = NULL;
-	else if (IS_ERR(old_key)) {
-		mutex_unlock(&nvdimm->key_mutex);
-		return PTR_ERR(old_key);
-	} else
+	old_key = nvdimm_lookup_user_key(dev, old_keyid);
+	if (old_key)
 		dev_dbg(dev, "%s: old key: %#x\n", __func__,
 				key_serial(old_key));
 
@@ -402,76 +312,28 @@ int nvdimm_security_change_key(struct nvdimm *nvdimm,
 	dev_dbg(dev, "%s: new key: %#x\n", __func__, key_serial(key));
 
 	down_read(&key->sem);
-	payload = key->payload.data[0];
-	if (payload->datalen != NVDIMM_PASSPHRASE_LEN) {
-		rc = -EINVAL;
-		up_read(&key->sem);
-		goto out;
-	}
-
-	/*
-	 * Since there is no existing key this user key will become the
-	 * kernel's key.
-	 */
-	if (!old_key) {
-		key = make_kernel_key(key);
-		if (!key) {
-			rc = -ENOMEM;
-			goto out;
-		}
-	}
-
-	/*
-	 * We don't need to release key->sem here because
-	 * make_kernel_key() will have upgraded the user key to kernel
-	 * and handled the semaphore handoff.
-	 */
-	payload = key->payload.data[0];
+	epayload = dereference_key_locked(key);
+	new_data = epayload->decrypted_data;
 
 	if (old_key) {
 		down_read(&old_key->sem);
-		old_payload = old_key->payload.data[0];
-		old_data = old_payload->data;
+		old_epayload = dereference_key_locked(old_key);
+		old_data = old_epayload->decrypted_data;
 	}
 
-	new_data = payload->data;
-
 	rc = nvdimm->security_ops->change_key(nvdimm, old_data,
 			new_data);
 	if (rc)
 		dev_warn(dev, "key update failed: %d\n", rc);
 
-	if (old_key) {
+	if (old_key)
 		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);
-
-	if (!old_key) {
-		if (rc == 0) {
-			dev_dbg(dev, "key cached: %#x\n", key_serial(key));
-			nvdimm->key = key;
-		} else
-			key_destroy(key);
-	}
 	nvdimm_security_get_state(nvdimm);
 
  out:
-	mutex_unlock(&nvdimm->key_mutex);
+	mutex_unlock(&nvdimm->sec_mutex);
+	key_put(old_key);
+	key_put(key);
 	return rc;
 }

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

[PATCH 03/11] libnvdimm/security: add override module param for key self verification

[Dave Jiang]

Provide the user an override via kernel module parameter for security key
self verification. no_key_self_verify parameter is being added to bypass
security key verify against the hardware during nvdimm unlock path.

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

diff --git a/drivers/nvdimm/security.c b/drivers/nvdimm/security.c
index ee741199d623..d2831e61f3d8 100644
--- a/drivers/nvdimm/security.c
+++ b/drivers/nvdimm/security.c
@@ -1,6 +1,7 @@
 // SPDX-License-Identifier: GPL-2.0
 /* Copyright(c) 2018 Intel Corporation. All rights reserved. */
 
+#include <linux/module.h>
 #include <linux/device.h>
 #include <linux/ndctl.h>
 #include <linux/slab.h>
@@ -14,6 +15,10 @@
 #include "nd-core.h"
 #include "nd.h"
 
+static bool no_key_self_verify;
+module_param(no_key_self_verify, bool, 0644);
+MODULE_PARM_DESC(no_key_self_verify, "Bypass security key self verify");
+
 /*
  * Retrieve user injected key
  */
@@ -235,6 +240,12 @@ int nvdimm_security_unlock_dimm(struct nvdimm *nvdimm)
 	 * other security operations.
 	 */
 	if (nvdimm->state == NVDIMM_SECURITY_UNLOCKED) {
+		/* bypass if user override */
+		if (no_key_self_verify) {
+			mutex_unlock(&nvdimm->sec_mutex);
+			return 0;
+		}
+
 		key = nvdimm_self_verify_key(nvdimm);
 		if (!key) {
 			rc = nvdimm_security_freeze_lock(nvdimm);

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

[PATCH 04/11] libnvdimm/security: introduce NDD_SECURITY_BUSY flag

[Dave Jiang]

Adding a flag for nvdimm->flags to support erase functions. While it's ok
to hold the nvdimm_bus lock for secure erase due to minimal time to execute
the command, overwrite requires a significantly longer time and makes this
impossible. The flag will block any drivers from being loaded and DIMMs
being probed.

Signed-off-by: Dave Jiang <dave.jiang@intel.com>
---
 drivers/nvdimm/nd.h          |    1 +
 drivers/nvdimm/region_devs.c |    7 +++++
 drivers/nvdimm/security.c    |   61 ++++++++++++++++++++++++++++++++++++++----
 include/linux/libnvdimm.h    |    2 +
 4 files changed, 65 insertions(+), 6 deletions(-)

diff --git a/drivers/nvdimm/nd.h b/drivers/nvdimm/nd.h
index f8d8f0a2a40d..6cb1cd4a39d0 100644
--- a/drivers/nvdimm/nd.h
+++ b/drivers/nvdimm/nd.h
@@ -250,6 +250,7 @@ long nvdimm_clear_poison(struct device *dev, phys_addr_t phys,
 void nvdimm_set_aliasing(struct device *dev);
 void nvdimm_set_locked(struct device *dev);
 void nvdimm_clear_locked(struct device *dev);
+int nvdimm_check_security_busy(struct nvdimm *nvdimm);
 struct nd_btt *to_nd_btt(struct device *dev);
 
 struct nd_gen_sb {
diff --git a/drivers/nvdimm/region_devs.c b/drivers/nvdimm/region_devs.c
index 174a418cb171..a097282b2c01 100644
--- a/drivers/nvdimm/region_devs.c
+++ b/drivers/nvdimm/region_devs.c
@@ -78,6 +78,13 @@ int nd_region_activate(struct nd_region *nd_region)
 	for (i = 0; i < nd_region->ndr_mappings; i++) {
 		struct nd_mapping *nd_mapping = &nd_region->mapping[i];
 		struct nvdimm *nvdimm = nd_mapping->nvdimm;
+		int rc;
+
+		rc = nvdimm_check_security_busy(nvdimm);
+		if (rc) {
+			nvdimm_bus_unlock(&nd_region->dev);
+			return rc;
+		}
 
 		/* at least one null hint slot per-dimm for the "no-hint" case */
 		flush_data_size += sizeof(void *);
diff --git a/drivers/nvdimm/security.c b/drivers/nvdimm/security.c
index d2831e61f3d8..2a83be47798e 100644
--- a/drivers/nvdimm/security.c
+++ b/drivers/nvdimm/security.c
@@ -19,6 +19,27 @@ static bool no_key_self_verify;
 module_param(no_key_self_verify, bool, 0644);
 MODULE_PARM_DESC(no_key_self_verify, "Bypass security key self verify");
 
+/*
+ * Check if we are doing security wipes
+ */
+int nvdimm_check_security_busy(struct nvdimm *nvdimm)
+{
+	if (test_bit(NDD_SECURITY_BUSY, &nvdimm->flags))
+		return -EBUSY;
+
+	return 0;
+}
+
+static inline void nvdimm_set_security_busy(struct nvdimm *nvdimm)
+{
+	set_bit(NDD_SECURITY_BUSY, &nvdimm->flags);
+}
+
+static inline void nvdimm_clear_security_busy(struct nvdimm *nvdimm)
+{
+	clear_bit(NDD_SECURITY_BUSY, &nvdimm->flags);
+}
+
 /*
  * Retrieve user injected key
  */
@@ -85,6 +106,13 @@ int nvdimm_security_erase(struct nvdimm *nvdimm, unsigned int keyid)
 
 	nvdimm_bus_lock(dev);
 	mutex_lock(&nvdimm->sec_mutex);
+	rc = nvdimm_check_security_busy(nvdimm);
+	if (rc < 0) {
+		dev_warn(dev, "Security operation in progress.\n");
+		goto out;
+	}
+
+	nvdimm_set_security_busy(nvdimm);
 	if (atomic_read(&nvdimm->busy)) {
 		dev_warn(dev, "Unable to secure erase while DIMM active.\n");
 		rc = -EBUSY;
@@ -113,6 +141,7 @@ int nvdimm_security_erase(struct nvdimm *nvdimm, unsigned int keyid)
 	key_put(key);
 
  out:
+	nvdimm_clear_security_busy(nvdimm);
 	mutex_unlock(&nvdimm->sec_mutex);
 	nvdimm_bus_unlock(dev);
 	nvdimm_security_get_state(nvdimm);
@@ -130,15 +159,19 @@ int nvdimm_security_freeze_lock(struct nvdimm *nvdimm)
 		return -EOPNOTSUPP;
 
 	mutex_lock(&nvdimm->sec_mutex);
+	rc = nvdimm_check_security_busy(nvdimm);
+	if (rc < 0)
+		goto out;
+
 	rc = nvdimm->security_ops->freeze_lock(nvdimm);
-	if (rc < 0) {
-		mutex_unlock(&nvdimm->sec_mutex);
-		return rc;
-	}
+	if (rc < 0)
+		goto out;
 
 	nvdimm_security_get_state(nvdimm);
+
+out:
 	mutex_unlock(&nvdimm->sec_mutex);
-	return 0;
+	return rc;
 }
 
 int nvdimm_security_disable(struct nvdimm *nvdimm, unsigned int keyid)
@@ -156,6 +189,12 @@ int nvdimm_security_disable(struct nvdimm *nvdimm, unsigned int keyid)
 		return -EOPNOTSUPP;
 
 	mutex_lock(&nvdimm->sec_mutex);
+	rc = nvdimm_check_security_busy(nvdimm);
+	if (rc < 0) {
+		mutex_unlock(&nvdimm->sec_mutex);
+		return rc;
+	}
+
 	/* look for a key from cached key */
 	key = nvdimm_lookup_user_key(dev, keyid);
 	if (!key) {
@@ -232,6 +271,12 @@ int nvdimm_security_unlock_dimm(struct nvdimm *nvdimm)
 		return 0;
 
 	mutex_lock(&nvdimm->sec_mutex);
+	rc = nvdimm_check_security_busy(nvdimm);
+	if (rc < 0) {
+		mutex_unlock(&nvdimm->sec_mutex);
+		return rc;
+	}
+
 	/*
 	 * If the pre-OS has unlocked the DIMM, we will attempt to send
 	 * the key from request_key() to the hardware for verification.
@@ -294,7 +339,7 @@ int nvdimm_security_change_key(struct nvdimm *nvdimm,
 		unsigned int old_keyid, unsigned int new_keyid)
 {
 	int rc;
-	struct key *key, *old_key;
+	struct key *key = NULL, *old_key = NULL;
 	void *old_data = NULL, *new_data;
 	struct device *dev = &nvdimm->dev;
 	struct encrypted_key_payload *epayload, *old_epayload;
@@ -306,6 +351,10 @@ int nvdimm_security_change_key(struct nvdimm *nvdimm,
 		return -EBUSY;
 
 	mutex_lock(&nvdimm->sec_mutex);
+	rc = nvdimm_check_security_busy(nvdimm);
+	if (rc < 0)
+		goto out;
+
 	/* look for a key from cached key if exists */
 	old_key = nvdimm_lookup_user_key(dev, old_keyid);
 	if (old_key)
diff --git a/include/linux/libnvdimm.h b/include/linux/libnvdimm.h
index 1071fe12081b..f3941836b93d 100644
--- a/include/linux/libnvdimm.h
+++ b/include/linux/libnvdimm.h
@@ -38,6 +38,8 @@ enum {
 	NDD_UNARMED = 1,
 	/* locked memory devices should not be accessed */
 	NDD_LOCKED = 2,
+	/* memory under security wipes should not be accessed */
+	NDD_SECURITY_BUSY = 3,
 
 	/* need to set a limit somewhere, but yes, this is likely overkill */
 	ND_IOCTL_MAX_BUFLEN = SZ_4M,

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

[PATCH 05/11] acpi/nfit, libnvdimm/security: Add security DSM overwrite support

[Dave Jiang]

We are adding support for the security calls of ovewrite and query
overwrite from Intel DSM spec v1.7. This will allow triggering of
overwrite on Intel NVDIMMs. The overwrite operation can take tens
of minutes. When the overwrite DSM is issued successfully, the NVDIMMs
will be unaccessible. The kernel will do backoff polling to detect when
the overwrite process is completed. According to the DSM spec v1.7,
the 128G NVDIMMs can take up to 15mins to perform overwrite and larger
DIMMs will take longer.

Signed-off-by: Dave Jiang <dave.jiang@intel.com>
---
 drivers/acpi/nfit/intel.c  |  113 +++++++++++++++++++++++++++++++++++++++++++
 drivers/acpi/nfit/intel.h  |    3 +
 drivers/acpi/nfit/nfit.h   |    1 
 drivers/nvdimm/core.c      |    3 +
 drivers/nvdimm/dimm_devs.c |   22 ++++++++
 drivers/nvdimm/nd-core.h   |    4 ++
 drivers/nvdimm/nd.h        |    8 +++
 drivers/nvdimm/security.c  |  116 ++++++++++++++++++++++++++++++++++++++++++++
 include/linux/libnvdimm.h  |    4 ++
 9 files changed, 273 insertions(+), 1 deletion(-)

diff --git a/drivers/acpi/nfit/intel.c b/drivers/acpi/nfit/intel.c
index f8f63f483859..f6d2b217f1c8 100644
--- a/drivers/acpi/nfit/intel.c
+++ b/drivers/acpi/nfit/intel.c
@@ -20,6 +20,112 @@
 
 #include <asm/smp.h> /* for wbinvd_on_all_cpus() on !SMP builds */
 
+static int intel_dimm_security_query_overwrite(struct nvdimm *nvdimm)
+{
+	int cmd_rc, rc = 0;
+	struct nfit_mem *nfit_mem = nvdimm_provider_data(nvdimm);
+	struct {
+		struct nd_cmd_pkg pkg;
+		struct nd_intel_query_overwrite cmd;
+	} nd_cmd = {
+		.pkg = {
+			.nd_command = NVDIMM_INTEL_QUERY_OVERWRITE,
+			.nd_family = NVDIMM_FAMILY_INTEL,
+			.nd_size_in = 0,
+			.nd_size_out = ND_INTEL_STATUS_SIZE,
+			.nd_fw_size = ND_INTEL_STATUS_SIZE,
+		},
+		.cmd = {
+			.status = 0,
+		},
+	};
+
+	if (!test_bit(NVDIMM_INTEL_QUERY_OVERWRITE, &nfit_mem->dsm_mask))
+		return -ENOTTY;
+
+	rc = nvdimm_ctl(nvdimm, ND_CMD_CALL, &nd_cmd, sizeof(nd_cmd), &cmd_rc);
+	if (rc < 0)
+		goto out;
+	if (cmd_rc < 0) {
+		rc = cmd_rc;
+		goto out;
+	}
+
+	switch (nd_cmd.cmd.status) {
+	case 0:
+		break;
+	case ND_INTEL_STATUS_OQUERY_INPROGRESS:
+		rc = -EBUSY;
+		goto out;
+	default:
+		rc = -ENXIO;
+		goto out;
+	}
+
+	/* flush all cache before we make the nvdimms available */
+	wbinvd_on_all_cpus();
+	nfit_mem->overwrite = false;
+
+out:
+	return rc;
+}
+
+static int intel_dimm_security_overwrite(struct nvdimm *nvdimm,
+		const struct nvdimm_key_data *nkey)
+{
+	int cmd_rc, rc = 0;
+	struct nfit_mem *nfit_mem = nvdimm_provider_data(nvdimm);
+	struct {
+		struct nd_cmd_pkg pkg;
+		struct nd_intel_overwrite cmd;
+	} nd_cmd = {
+		.pkg = {
+			.nd_command = NVDIMM_INTEL_OVERWRITE,
+			.nd_family = NVDIMM_FAMILY_INTEL,
+			.nd_size_in = ND_INTEL_PASSPHRASE_SIZE,
+			.nd_size_out = ND_INTEL_STATUS_SIZE,
+			.nd_fw_size = ND_INTEL_STATUS_SIZE,
+		},
+		.cmd = {
+			.status = 0,
+		},
+	};
+
+	if (!test_bit(NVDIMM_INTEL_OVERWRITE, &nfit_mem->dsm_mask))
+		return -ENOTTY;
+
+	/* flush all cache before we erase DIMM */
+	wbinvd_on_all_cpus();
+	memcpy(nd_cmd.cmd.passphrase, nkey->data,
+			sizeof(nd_cmd.cmd.passphrase));
+	rc = nvdimm_ctl(nvdimm, ND_CMD_CALL, &nd_cmd, sizeof(nd_cmd), &cmd_rc);
+	if (rc < 0)
+		goto out;
+	if (cmd_rc < 0) {
+		rc = cmd_rc;
+		goto out;
+	}
+
+	switch (nd_cmd.cmd.status) {
+	case 0:
+		nfit_mem->overwrite = true;
+		break;
+	case ND_INTEL_STATUS_OVERWRITE_UNSUPPORTED:
+		rc = -ENOTSUPP;
+		break;
+	case ND_INTEL_STATUS_INVALID_PASS:
+		rc = -EINVAL;
+		goto out;
+	case ND_INTEL_STATUS_INVALID_STATE:
+	default:
+		rc = -ENXIO;
+		goto out;
+	}
+
+ out:
+	return rc;
+}
+
 static int intel_dimm_security_erase(struct nvdimm *nvdimm,
 		const struct nvdimm_key_data *nkey)
 {
@@ -319,6 +425,11 @@ static int intel_dimm_security_state(struct nvdimm *nvdimm,
 		return 0;
 	}
 
+	if (nfit_mem->overwrite == true) {
+		*state = NVDIMM_SECURITY_OVERWRITE;
+		return 0;
+	}
+
 	*state = NVDIMM_SECURITY_DISABLED;
 	rc = nvdimm_ctl(nvdimm, ND_CMD_CALL, &nd_cmd, sizeof(nd_cmd), &cmd_rc);
 	if (rc < 0)
@@ -367,5 +478,7 @@ static const struct nvdimm_security_ops __intel_security_ops = {
 	.disable = intel_dimm_security_disable,
 	.freeze_lock = intel_dimm_security_freeze_lock,
 	.erase = intel_dimm_security_erase,
+	.overwrite = intel_dimm_security_overwrite,
+	.query_overwrite = intel_dimm_security_query_overwrite,
 };
 const struct nvdimm_security_ops *intel_security_ops = &__intel_security_ops;
diff --git a/drivers/acpi/nfit/intel.h b/drivers/acpi/nfit/intel.h
index 0af5b6fd0aea..a4e01de966dc 100644
--- a/drivers/acpi/nfit/intel.h
+++ b/drivers/acpi/nfit/intel.h
@@ -50,12 +50,15 @@ extern const struct nvdimm_security_ops *intel_security_ops;
 #define ND_INTEL_STATUS_NOT_READY	9
 #define ND_INTEL_STATUS_INVALID_STATE	10
 #define ND_INTEL_STATUS_INVALID_PASS	11
+#define ND_INTEL_STATUS_OVERWRITE_UNSUPPORTED	0x10007
+#define ND_INTEL_STATUS_OQUERY_INPROGRESS	0x10007
 
 #define ND_INTEL_SEC_STATE_ENABLED	0x02
 #define ND_INTEL_SEC_STATE_LOCKED	0x04
 #define ND_INTEL_SEC_STATE_FROZEN	0x08
 #define ND_INTEL_SEC_STATE_PLIMIT	0x10
 #define ND_INTEL_SEC_STATE_UNSUPPORTED	0x20
+#define ND_INTEL_SEC_STATE_OVERWRITE	0x40
 
 struct nd_intel_get_security_state {
 	u32 status;
diff --git a/drivers/acpi/nfit/nfit.h b/drivers/acpi/nfit/nfit.h
index 00de13bc5714..d50dbb4cdeaf 100644
--- a/drivers/acpi/nfit/nfit.h
+++ b/drivers/acpi/nfit/nfit.h
@@ -204,6 +204,7 @@ struct nfit_mem {
 	unsigned long flags;
 	u32 dirty_shutdown;
 	int family;
+	bool overwrite;
 };
 
 struct acpi_nfit_desc {
diff --git a/drivers/nvdimm/core.c b/drivers/nvdimm/core.c
index acce050856a8..b2496c06178b 100644
--- a/drivers/nvdimm/core.c
+++ b/drivers/nvdimm/core.c
@@ -437,6 +437,9 @@ static __init int libnvdimm_init(void)
 {
 	int rc;
 
+	rc = nvdimm_devs_init();
+	if (rc)
+		return rc;
 	rc = nvdimm_bus_init();
 	if (rc)
 		return rc;
diff --git a/drivers/nvdimm/dimm_devs.c b/drivers/nvdimm/dimm_devs.c
index 269f401a5e68..b613c131bfb9 100644
--- a/drivers/nvdimm/dimm_devs.c
+++ b/drivers/nvdimm/dimm_devs.c
@@ -25,6 +25,7 @@
 #include "nd.h"
 
 static DEFINE_IDA(dimm_ida);
+struct workqueue_struct *nvdimm_wq;
 
 /*
  * Retrieve bus and dimm handle and return if this bus supports
@@ -386,6 +387,8 @@ __weak ssize_t security_show(struct device *dev,
 		return sprintf(buf, "locked\n");
 	case NVDIMM_SECURITY_FROZEN:
 		return sprintf(buf, "frozen\n");
+	case NVDIMM_SECURITY_OVERWRITE:
+		return sprintf(buf, "overwrite\n");
 	case NVDIMM_SECURITY_UNSUPPORTED:
 	default:
 		return sprintf(buf, "unsupported\n");
@@ -432,6 +435,11 @@ static ssize_t security_store(struct device *dev,
 			return -EINVAL;
 		dev_dbg(dev, "erase %#x\n", old_key);
 		rc = nvdimm_security_erase(nvdimm, old_key);
+	} else if (sysfs_streq(cmd, "overwrite")) {
+		if (rc != 2)
+			return -EINVAL;
+		dev_dbg(dev, "overwrite %u\n", old_key);
+		rc = nvdimm_security_overwrite(nvdimm, old_key);
 	} else
 		return -EINVAL;
 
@@ -492,6 +500,8 @@ struct nvdimm *__nvdimm_create(struct nvdimm_bus *nvdimm_bus, void *provider_dat
 	dev->type = &nvdimm_device_type;
 	dev->devt = MKDEV(nvdimm_major, nvdimm->id);
 	dev->groups = groups;
+	nvdimm->overwrite_tmo = 0;
+	INIT_DELAYED_WORK(&nvdimm->dwork, nvdimm_overwrite_query);
 	nd_device_register(dev);
 
 	return nvdimm;
@@ -768,7 +778,17 @@ int nvdimm_bus_check_dimm_count(struct nvdimm_bus *nvdimm_bus, int dimm_count)
 }
 EXPORT_SYMBOL_GPL(nvdimm_bus_check_dimm_count);
 
-void __exit nvdimm_devs_exit(void)
+int __init nvdimm_devs_init(void)
+{
+	nvdimm_wq = create_singlethread_workqueue("nvdimm");
+	if (!nvdimm_wq)
+		return -ENOMEM;
+
+	return 0;
+}
+
+void  nvdimm_devs_exit(void)
 {
+	destroy_workqueue(nvdimm_wq);
 	ida_destroy(&dimm_ida);
 }
diff --git a/drivers/nvdimm/nd-core.h b/drivers/nvdimm/nd-core.h
index cfb87f71db8d..20a8216c503d 100644
--- a/drivers/nvdimm/nd-core.h
+++ b/drivers/nvdimm/nd-core.h
@@ -22,6 +22,7 @@
 extern struct list_head nvdimm_bus_list;
 extern struct mutex nvdimm_bus_list_mutex;
 extern int nvdimm_major;
+extern struct workqueue_struct *nvdimm_wq;
 
 struct nvdimm_bus {
 	struct nvdimm_bus_descriptor *nd_desc;
@@ -46,6 +47,8 @@ struct nvdimm {
 	const struct nvdimm_security_ops *security_ops;
 	enum nvdimm_security_state state;
 	struct mutex sec_mutex;
+	struct delayed_work dwork;
+	unsigned int overwrite_tmo;
 };
 
 /**
@@ -81,6 +84,7 @@ struct nvdimm_bus *walk_to_nvdimm_bus(struct device *nd_dev);
 int __init nvdimm_bus_init(void);
 void nvdimm_bus_exit(void);
 void nvdimm_devs_exit(void);
+int nvdimm_devs_init(void);
 void nd_region_devs_exit(void);
 void nd_region_probe_success(struct nvdimm_bus *nvdimm_bus, struct device *dev);
 struct nd_region;
diff --git a/drivers/nvdimm/nd.h b/drivers/nvdimm/nd.h
index 6cb1cd4a39d0..ba41582c4cc2 100644
--- a/drivers/nvdimm/nd.h
+++ b/drivers/nvdimm/nd.h
@@ -435,6 +435,9 @@ int nvdimm_security_change_key(struct nvdimm *nvdimm, unsigned int old_keyid,
 int nvdimm_security_disable(struct nvdimm *nvdimm, unsigned int keyid);
 int nvdimm_security_freeze_lock(struct nvdimm *nvdimm);
 int nvdimm_security_erase(struct nvdimm *nvdimm, unsigned int keyid);
+int nvdimm_security_overwrite(struct nvdimm *nvdimm, unsigned int keyid);
+void nvdimm_overwrite_query(struct work_struct *work);
+
 #else
 static inline int nvdimm_security_unlock_dimm(struct nvdimm *nvdimm)
 {
@@ -468,5 +471,10 @@ static inline int nvdimm_security_erase(struct nvdimm *nvdimm,
 {
 	return -EOPNOTSUPP;
 }
+int nvdimm_security_overwrite(struct nvdimm *nvdimm, unsigned int keyid)
+{
+	return -EOPNOTSUPP;
+}
+void nvdimm_overwrite_query(struct work_struct *work) {}
 #endif
 #endif /* __ND_H__ */
diff --git a/drivers/nvdimm/security.c b/drivers/nvdimm/security.c
index 2a83be47798e..725acd211114 100644
--- a/drivers/nvdimm/security.c
+++ b/drivers/nvdimm/security.c
@@ -93,6 +93,122 @@ int nvdimm_security_get_state(struct nvdimm *nvdimm)
 	return nvdimm->security_ops->state(nvdimm, &nvdimm->state);
 }
 
+void nvdimm_overwrite_query(struct work_struct *work)
+{
+	struct nvdimm *nvdimm;
+	struct nvdimm_bus *nvdimm_bus;
+	int rc;
+	unsigned int tmo;
+
+	nvdimm = container_of(work, typeof(*nvdimm), dwork.work);
+	nvdimm_bus = walk_to_nvdimm_bus(&nvdimm->dev);
+	tmo = nvdimm->overwrite_tmo;
+
+	if (!nvdimm->security_ops)
+		return;
+
+	rc = nvdimm->security_ops->query_overwrite(nvdimm);
+	if (rc == -EBUSY) {
+
+		/* setup delayed work again */
+		tmo += 10;
+		queue_delayed_work(nvdimm_wq, &nvdimm->dwork, tmo * HZ);
+		nvdimm->overwrite_tmo = min(15U * 60U, tmo);
+		return;
+	}
+
+	if (rc < 0)
+		dev_warn(&nvdimm->dev, "Overwrite failed\n");
+	else
+		dev_info(&nvdimm->dev, "Overwrite completed\n");
+
+	nvdimm->overwrite_tmo = 0;
+	nvdimm_clear_security_busy(nvdimm);
+	nvdimm_security_get_state(nvdimm);
+}
+
+int nvdimm_security_overwrite(struct nvdimm *nvdimm, unsigned int keyid)
+{
+	struct nvdimm_bus *nvdimm_bus = walk_to_nvdimm_bus(&nvdimm->dev);
+	struct device *dev = &nvdimm->dev;
+	struct key *key = NULL;
+	struct encrypted_key_payload *epayload;
+	void *data;
+	int rc;
+	char pass[NVDIMM_PASSPHRASE_LEN];
+
+	if (!nvdimm->security_ops)
+		return 0;
+
+	nvdimm_bus_lock(&nvdimm_bus->dev);
+	mutex_lock(&nvdimm->sec_mutex);
+	rc = nvdimm_check_security_busy(nvdimm);
+	if (rc)
+		goto out;
+
+	nvdimm_set_security_busy(nvdimm);
+
+	if (atomic_read(&nvdimm->busy)) {
+		dev_warn(dev, "Unable to overwrite while DIMM active.\n");
+		nvdimm_clear_security_busy(nvdimm);
+		rc = -EBUSY;
+		goto out;
+	}
+
+	if (dev_get_drvdata(dev)) {
+		dev_warn(dev, "Unable to overwrite while DIMM active.\n");
+		nvdimm_clear_security_busy(nvdimm);
+		rc = -EINVAL;
+		goto out;
+	}
+
+	if (nvdimm->state == NVDIMM_SECURITY_UNSUPPORTED) {
+		dev_warn(dev, "Attempt to overwrite in wrong state.\n");
+		nvdimm_clear_security_busy(nvdimm);
+		rc = -EOPNOTSUPP;
+		goto out;
+	}
+
+	key = nvdimm_lookup_user_key(dev, keyid);
+	if (!key && keyid != 0) {
+		dev_dbg(dev, "Unable to get and verify key\n");
+		nvdimm_clear_security_busy(nvdimm);
+		rc = -ENOKEY;
+		goto out;
+	}
+
+	if (key) {
+		down_read(&key->sem);
+		epayload = dereference_key_locked(key);
+		data = epayload->decrypted_data;
+		memcpy(pass, data, NVDIMM_PASSPHRASE_LEN);
+		up_read(&key->sem);
+	} else
+		memset(pass, 0, NVDIMM_PASSPHRASE_LEN);
+	/*
+	 * Overwrite can execute w/o passphrase. we will let the hardware
+	 * determine whether we need a passphrase or not.
+	 */
+	rc = nvdimm->security_ops->overwrite(nvdimm,
+			(const struct nvdimm_key_data *)pass);
+	if (rc == 0) {
+		/* clear out security data */
+		if (key)
+			memset(pass, 0, NVDIMM_PASSPHRASE_LEN);
+		nvdimm->state = NVDIMM_SECURITY_OVERWRITE;
+		queue_delayed_work(nvdimm_wq, &nvdimm->dwork, 0);
+	} else
+		nvdimm_clear_security_busy(nvdimm);
+
+ out:
+	key_put(key);
+	mutex_unlock(&nvdimm->sec_mutex);
+	nvdimm_bus_unlock(&nvdimm_bus->dev);
+	nvdimm_security_get_state(nvdimm);
+	return rc;
+
+}
+
 int nvdimm_security_erase(struct nvdimm *nvdimm, unsigned int keyid)
 {
 	int rc = 0;
diff --git a/include/linux/libnvdimm.h b/include/linux/libnvdimm.h
index f3941836b93d..479421ce62c0 100644
--- a/include/linux/libnvdimm.h
+++ b/include/linux/libnvdimm.h
@@ -163,6 +163,7 @@ enum nvdimm_security_state {
 	NVDIMM_SECURITY_UNLOCKED,
 	NVDIMM_SECURITY_LOCKED,
 	NVDIMM_SECURITY_FROZEN,
+	NVDIMM_SECURITY_OVERWRITE,
 	NVDIMM_SECURITY_UNSUPPORTED,
 };
 
@@ -186,6 +187,9 @@ struct nvdimm_security_ops {
 	int (*freeze_lock)(struct nvdimm *nvdimm);
 	int (*erase)(struct nvdimm *nvdimm,
 			const struct nvdimm_key_data *nkey);
+	int (*overwrite)(struct nvdimm *nvdimm,
+			const struct nvdimm_key_data *nkey);
+	int (*query_overwrite)(struct nvdimm *nvdimm);
 };
 
 void badrange_init(struct badrange *badrange);

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

[PATCH 06/11] tools/testing/nvdimm: Add overwrite support for nfit_test

[Dave Jiang]

With the implementation of Intel NVDIMM DSM overwrite, we are adding unit
test to nfit_test for testing of overwrite operation.

Signed-off-by: Dave Jiang <dave.jiang@intel.com>
---
 drivers/acpi/nfit/intel.h        |    1 +
 tools/testing/nvdimm/dimm_devs.c |    2 +
 tools/testing/nvdimm/test/nfit.c |   55 ++++++++++++++++++++++++++++++++++++++
 3 files changed, 58 insertions(+)

diff --git a/drivers/acpi/nfit/intel.h b/drivers/acpi/nfit/intel.h
index a4e01de966dc..0140d9b6a0b7 100644
--- a/drivers/acpi/nfit/intel.h
+++ b/drivers/acpi/nfit/intel.h
@@ -52,6 +52,7 @@ extern const struct nvdimm_security_ops *intel_security_ops;
 #define ND_INTEL_STATUS_INVALID_PASS	11
 #define ND_INTEL_STATUS_OVERWRITE_UNSUPPORTED	0x10007
 #define ND_INTEL_STATUS_OQUERY_INPROGRESS	0x10007
+#define ND_INTEL_STATUS_OQUERY_SEQUENCE_ERR	0x20007
 
 #define ND_INTEL_SEC_STATE_ENABLED	0x02
 #define ND_INTEL_SEC_STATE_LOCKED	0x04
diff --git a/tools/testing/nvdimm/dimm_devs.c b/tools/testing/nvdimm/dimm_devs.c
index de3a22988dc5..3f8ad3d445d2 100644
--- a/tools/testing/nvdimm/dimm_devs.c
+++ b/tools/testing/nvdimm/dimm_devs.c
@@ -29,6 +29,8 @@ ssize_t security_show(struct device *dev,
 		return sprintf(buf, "locked\n");
 	case NVDIMM_SECURITY_FROZEN:
 		return sprintf(buf, "frozen\n");
+	case NVDIMM_SECURITY_OVERWRITE:
+		return sprintf(buf, "overwrite\n");
 	case NVDIMM_SECURITY_UNSUPPORTED:
 	default:
 		return sprintf(buf, "unsupported\n");
diff --git a/tools/testing/nvdimm/test/nfit.c b/tools/testing/nvdimm/test/nfit.c
index e4b560494e0a..c885fe136f42 100644
--- a/tools/testing/nvdimm/test/nfit.c
+++ b/tools/testing/nvdimm/test/nfit.c
@@ -149,6 +149,7 @@ static int dimm_fail_cmd_code[NUM_DCR];
 struct nfit_test_sec {
 	u8 state;
 	u8 passphrase[32];
+	u64 overwrite_end_time;
 } dimm_sec_info[NUM_DCR];
 
 static const struct nd_intel_smart smart_def = {
@@ -1073,6 +1074,50 @@ static int nd_intel_test_cmd_secure_erase(struct nfit_test *t,
 	return 0;
 }
 
+static int nd_intel_test_cmd_overwrite(struct nfit_test *t,
+		struct nd_intel_overwrite *nd_cmd,
+		unsigned int buf_len, int dimm)
+{
+	struct device *dev = &t->pdev.dev;
+	struct nfit_test_sec *sec = &dimm_sec_info[dimm];
+
+	if ((sec->state & ND_INTEL_SEC_STATE_ENABLED) &&
+			memcmp(nd_cmd->passphrase, sec->passphrase,
+				ND_INTEL_PASSPHRASE_SIZE) != 0) {
+		nd_cmd->status = ND_INTEL_STATUS_INVALID_PASS;
+		dev_dbg(dev, "overwrite: wrong passphrase\n");
+		return 0;
+	}
+
+	memset(sec->passphrase, 0, ND_INTEL_PASSPHRASE_SIZE);
+	sec->state = ND_INTEL_SEC_STATE_OVERWRITE;
+	dev_dbg(dev, "overwrite progressing.\n");
+	sec->overwrite_end_time = get_jiffies_64() + 5 * HZ;
+
+	return 0;
+}
+
+static int nd_intel_test_cmd_query_overwrite(struct nfit_test *t,
+		struct nd_intel_query_overwrite *nd_cmd,
+		unsigned int buf_len, int dimm)
+{
+	struct device *dev = &t->pdev.dev;
+	struct nfit_test_sec *sec = &dimm_sec_info[dimm];
+
+	if (!(sec->state & ND_INTEL_SEC_STATE_OVERWRITE)) {
+		nd_cmd->status = ND_INTEL_STATUS_OQUERY_SEQUENCE_ERR;
+		return 0;
+	}
+
+	if (time_is_before_jiffies64(sec->overwrite_end_time)) {
+		sec->overwrite_end_time = 0;
+		sec->state = 0;
+		dev_dbg(dev, "overwrite is complete\n");
+	} else
+		nd_cmd->status = ND_INTEL_STATUS_OQUERY_INPROGRESS;
+	return 0;
+}
+
 static int get_dimm(struct nfit_mem *nfit_mem, unsigned int func)
 {
 	int i;
@@ -1144,6 +1189,14 @@ static int nfit_test_ctl(struct nvdimm_bus_descriptor *nd_desc,
 				rc = nd_intel_test_cmd_secure_erase(t,
 						buf, buf_len, i);
 				break;
+			case NVDIMM_INTEL_OVERWRITE:
+				rc = nd_intel_test_cmd_overwrite(t,
+						buf, buf_len, i - t->dcr_idx);
+				break;
+			case NVDIMM_INTEL_QUERY_OVERWRITE:
+				rc = nd_intel_test_cmd_query_overwrite(t,
+						buf, buf_len, i - t->dcr_idx);
+				break;
 			case ND_INTEL_ENABLE_LSS_STATUS:
 				rc = nd_intel_test_cmd_set_lss_status(t,
 						buf, buf_len);
@@ -2379,6 +2432,8 @@ static void nfit_test0_setup(struct nfit_test *t)
 	set_bit(NVDIMM_INTEL_UNLOCK_UNIT, &acpi_desc->dimm_cmd_force_en);
 	set_bit(NVDIMM_INTEL_FREEZE_LOCK, &acpi_desc->dimm_cmd_force_en);
 	set_bit(NVDIMM_INTEL_SECURE_ERASE, &acpi_desc->dimm_cmd_force_en);
+	set_bit(NVDIMM_INTEL_OVERWRITE, &acpi_desc->dimm_cmd_force_en);
+	set_bit(NVDIMM_INTEL_QUERY_OVERWRITE, &acpi_desc->dimm_cmd_force_en);
 }
 
 static void nfit_test1_setup(struct nfit_test *t)

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

[PATCH 07/11] libnvdimm/security: add overwrite status notification

[Dave Jiang]

Adding sysfs notification for when overwrite has completed to allow
user monitoring app to be aware of overwrite completion status.

Signed-off-by: Dave Jiang <dave.jiang@intel.com>
---
 drivers/acpi/nfit/core.c   |    5 +++++
 drivers/nvdimm/dimm_devs.c |   10 ++++++++++
 drivers/nvdimm/nd-core.h   |    1 +
 drivers/nvdimm/security.c  |    2 ++
 include/linux/libnvdimm.h  |    1 +
 5 files changed, 19 insertions(+)

diff --git a/drivers/acpi/nfit/core.c b/drivers/acpi/nfit/core.c
index de4e00059277..3e6c7b653872 100644
--- a/drivers/acpi/nfit/core.c
+++ b/drivers/acpi/nfit/core.c
@@ -2033,6 +2033,11 @@ static int acpi_nfit_register_dimms(struct acpi_nfit_desc *acpi_desc)
 		if (!nvdimm)
 			continue;
 
+		rc = nvdimm_setup_security_events(nvdimm);
+		if (rc < 0)
+			dev_warn(acpi_desc->dev,
+					"no security event setup failed\n");
+
 		nfit_kernfs = sysfs_get_dirent(nvdimm_kobj(nvdimm)->sd, "nfit");
 		if (nfit_kernfs)
 			nfit_mem->flags_attr = sysfs_get_dirent(nfit_kernfs,
diff --git a/drivers/nvdimm/dimm_devs.c b/drivers/nvdimm/dimm_devs.c
index b613c131bfb9..39e40074b5df 100644
--- a/drivers/nvdimm/dimm_devs.c
+++ b/drivers/nvdimm/dimm_devs.c
@@ -508,6 +508,16 @@ struct nvdimm *__nvdimm_create(struct nvdimm_bus *nvdimm_bus, void *provider_dat
 }
 EXPORT_SYMBOL_GPL(__nvdimm_create);
 
+int nvdimm_setup_security_events(struct nvdimm *nvdimm)
+{
+	nvdimm->overwrite_state = sysfs_get_dirent(nvdimm->dev.kobj.sd,
+			"security");
+	if (!nvdimm->overwrite_state)
+		return -ENODEV;
+	return 0;
+}
+EXPORT_SYMBOL_GPL(nvdimm_setup_security_events);
+
 int alias_dpa_busy(struct device *dev, void *data)
 {
 	resource_size_t map_end, blk_start, new;
diff --git a/drivers/nvdimm/nd-core.h b/drivers/nvdimm/nd-core.h
index 20a8216c503d..b96e1b10e3eb 100644
--- a/drivers/nvdimm/nd-core.h
+++ b/drivers/nvdimm/nd-core.h
@@ -49,6 +49,7 @@ struct nvdimm {
 	struct mutex sec_mutex;
 	struct delayed_work dwork;
 	unsigned int overwrite_tmo;
+	struct kernfs_node *overwrite_state;
 };
 
 /**
diff --git a/drivers/nvdimm/security.c b/drivers/nvdimm/security.c
index 725acd211114..f5ba633545b7 100644
--- a/drivers/nvdimm/security.c
+++ b/drivers/nvdimm/security.c
@@ -122,6 +122,8 @@ void nvdimm_overwrite_query(struct work_struct *work)
 	else
 		dev_info(&nvdimm->dev, "Overwrite completed\n");
 
+	if (nvdimm->overwrite_state)
+		sysfs_notify_dirent(nvdimm->overwrite_state);
 	nvdimm->overwrite_tmo = 0;
 	nvdimm_clear_security_busy(nvdimm);
 	nvdimm_security_get_state(nvdimm);
diff --git a/include/linux/libnvdimm.h b/include/linux/libnvdimm.h
index 479421ce62c0..c3c5a1c6b1b7 100644
--- a/include/linux/libnvdimm.h
+++ b/include/linux/libnvdimm.h
@@ -227,6 +227,7 @@ static inline struct nvdimm *nvdimm_create(struct nvdimm_bus *nvdimm_bus,
 			cmd_mask, num_flush, flush_wpq, NULL, NULL);
 }
 
+int nvdimm_setup_security_events(struct nvdimm *nvdimm);
 const struct nd_cmd_desc *nd_cmd_dimm_desc(int cmd);
 const struct nd_cmd_desc *nd_cmd_bus_desc(int cmd);
 u32 nd_cmd_in_size(struct nvdimm *nvdimm, int cmd,

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

[PATCH 08/11] libnvdimm/security: add documentation for ovewrite

[Dave Jiang]

Add overwrite command usages to security documentation.

Signed-off-by: Dave Jiang <dave.jiang@intel.com>
---
 Documentation/nvdimm/security.txt |   16 +++++++++++++---
 1 file changed, 13 insertions(+), 3 deletions(-)

diff --git a/Documentation/nvdimm/security.txt b/Documentation/nvdimm/security.txt
index 11240ce48755..dfe70a8fa25b 100644
--- a/Documentation/nvdimm/security.txt
+++ b/Documentation/nvdimm/security.txt
@@ -96,9 +96,19 @@ its keyid should be passed in via sysfs.
 The command format for doing a secure erase is:
 erase <current keyid>
 
-An "old" key with the passphrase payload that is tied to the nvdimm should be
-injected with a key description that does not have the "nvdimm:" prefix and
-its keyid should be passed in via sysfs.
+9. Overwrite
+------------
+The command format for doing an overwrite is:
+overwrite <current keyid>
+
+Overwrite can be done without a key if security is not enabled. A key serial
+of 0 can be passed in to indicate no key.
+
+The sysfs attribute "security" can be polled to wait on overwrite completion.
+Overwrite can last tens of minutes or more depending on nvdimm size.
+
+An encrypted key with the current key passphrase that is tied to the nvdimm
+should be injected and its keyid should be passed in via sysfs.
 
 [1]: http://pmem.io/documents/NVDIMM_DSM_Interface-V1.7.pdf
 [2]: http://www.t13.org/documents/UploadedDocuments/docs2006/e05179r4-ACS-SecurityClarifications.pdf

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

[PATCH 09/11] acpi/nfit, libnvdimm/security: add Intel DSM 1.8 master passphrase support

[Dave Jiang]

With Intel DSM 1.8 [1] two new security DSMs are introduced. Enable/update
master passphrase and master secure erase. The master passphrase allows
a secure erase to be performed without the user passphrase that is set on
the NVDIMM. The commands of master_update and master_erase are added to
the sysfs knob in order to initiate the DSMs. They are similar in opeartion
mechanism compare to update and erase.

[1]: http://pmem.io/documents/NVDIMM_DSM_Interface-V1.8.pdf

Signed-off-by: Dave Jiang <dave.jiang@intel.com>
---
 Documentation/nvdimm/security.txt |   23 ++++++
 drivers/acpi/nfit/core.c          |    2 +
 drivers/acpi/nfit/intel.c         |  132 ++++++++++++++++++++++++++++++++++++-
 drivers/acpi/nfit/intel.h         |   18 +++++
 drivers/acpi/nfit/nfit.h          |    6 +-
 drivers/nvdimm/dimm_devs.c        |   16 ++++
 drivers/nvdimm/nd-core.h          |    1 
 drivers/nvdimm/nd.h               |    5 +
 drivers/nvdimm/security.c         |   34 ++++++++--
 include/linux/libnvdimm.h         |   15 ++++
 10 files changed, 236 insertions(+), 16 deletions(-)

diff --git a/Documentation/nvdimm/security.txt b/Documentation/nvdimm/security.txt
index dfe70a8fa25b..a2d954349417 100644
--- a/Documentation/nvdimm/security.txt
+++ b/Documentation/nvdimm/security.txt
@@ -110,5 +110,28 @@ Overwrite can last tens of minutes or more depending on nvdimm size.
 An encrypted key with the current key passphrase that is tied to the nvdimm
 should be injected and its keyid should be passed in via sysfs.
 
+10. Master Update
+-----------------
+The command format for doing a master update is:
+update <old keyid> <new keyid>
+
+The operating mechansim for master update is identical to update except the
+master passphrase key is passed to the kernel. The master passphrase key
+is just another encrypted key.
+
+This command is only available when security is disabled.
+
+11. Master Erase
+----------------
+The command format for doing a master erase is:
+master_erase <current keyid>
+
+This command has the same operating mechanism as erase except the master
+passphrase key is passed to the kernel. The master passphrase key is just
+another encrypted key.
+
+This command is only available when the master security is enabled, indicated
+by the extended security status.
+
 [1]: http://pmem.io/documents/NVDIMM_DSM_Interface-V1.7.pdf
 [2]: http://www.t13.org/documents/UploadedDocuments/docs2006/e05179r4-ACS-SecurityClarifications.pdf
diff --git a/drivers/acpi/nfit/core.c b/drivers/acpi/nfit/core.c
index 3e6c7b653872..867e6fea3737 100644
--- a/drivers/acpi/nfit/core.c
+++ b/drivers/acpi/nfit/core.c
@@ -389,6 +389,8 @@ static u8 nfit_dsm_revid(unsigned family, unsigned func)
 			[NVDIMM_INTEL_SECURE_ERASE] = 2,
 			[NVDIMM_INTEL_OVERWRITE] = 2,
 			[NVDIMM_INTEL_QUERY_OVERWRITE] = 2,
+			[NVDIMM_INTEL_SET_MASTER_PASSPHRASE] = 2,
+			[NVDIMM_INTEL_MASTER_SECURE_ERASE] = 2,
 		},
 	};
 	u8 id;
diff --git a/drivers/acpi/nfit/intel.c b/drivers/acpi/nfit/intel.c
index f6d2b217f1c8..4e47fc2b524c 100644
--- a/drivers/acpi/nfit/intel.c
+++ b/drivers/acpi/nfit/intel.c
@@ -20,6 +20,123 @@
 
 #include <asm/smp.h> /* for wbinvd_on_all_cpus() on !SMP builds */
 
+static int intel_dimm_security_master_erase(struct nvdimm *nvdimm,
+		const struct nvdimm_key_data *nkey)
+{
+	int cmd_rc, rc = 0;
+	struct nfit_mem *nfit_mem = nvdimm_provider_data(nvdimm);
+	struct {
+		struct nd_cmd_pkg pkg;
+		struct nd_intel_secure_erase cmd;
+	} nd_cmd = {
+		.pkg = {
+			.nd_command = NVDIMM_INTEL_MASTER_SECURE_ERASE,
+			.nd_family = NVDIMM_FAMILY_INTEL,
+			.nd_size_in = ND_INTEL_PASSPHRASE_SIZE,
+			.nd_size_out = ND_INTEL_STATUS_SIZE,
+			.nd_fw_size = ND_INTEL_STATUS_SIZE,
+		},
+		.cmd = {
+			.status = 0,
+		},
+	};
+
+	if (!test_bit(NVDIMM_INTEL_MASTER_SECURE_ERASE, &nfit_mem->dsm_mask))
+		return -ENOTTY;
+
+	/* flush all cache before we erase DIMM */
+	wbinvd_on_all_cpus();
+	memcpy(nd_cmd.cmd.passphrase, nkey->data,
+			sizeof(nd_cmd.cmd.passphrase));
+	rc = nvdimm_ctl(nvdimm, ND_CMD_CALL, &nd_cmd, sizeof(nd_cmd), &cmd_rc);
+	if (rc < 0)
+		goto out;
+	if (cmd_rc < 0) {
+		rc = cmd_rc;
+		goto out;
+	}
+
+	switch (nd_cmd.cmd.status) {
+	case 0:
+		break;
+	case ND_INTEL_STATUS_NOT_SUPPORTED:
+		rc = -EOPNOTSUPP;
+		goto out;
+	case ND_INTEL_STATUS_INVALID_PASS:
+		rc = -EINVAL;
+		goto out;
+	case ND_INTEL_STATUS_INVALID_STATE:
+	default:
+		rc = -ENXIO;
+		goto out;
+	}
+
+	/* DIMM erased, invalidate all CPU caches before we read it */
+	wbinvd_on_all_cpus();
+
+ out:
+	return rc;
+}
+
+static int intel_dimm_security_master_update_passphrase(struct nvdimm *nvdimm,
+		const struct nvdimm_key_data *old_data,
+		const struct nvdimm_key_data *new_data)
+{
+	int cmd_rc, rc = 0;
+	struct nfit_mem *nfit_mem = nvdimm_provider_data(nvdimm);
+	struct {
+		struct nd_cmd_pkg pkg;
+		struct nd_intel_set_passphrase cmd;
+	} nd_cmd = {
+		.pkg = {
+			.nd_command = NVDIMM_INTEL_SET_MASTER_PASSPHRASE,
+			.nd_family = NVDIMM_FAMILY_INTEL,
+			.nd_size_in = ND_INTEL_PASSPHRASE_SIZE * 2,
+			.nd_size_out = ND_INTEL_STATUS_SIZE,
+			.nd_fw_size = ND_INTEL_STATUS_SIZE,
+		},
+		.cmd = {
+			.status = 0,
+		},
+	};
+
+	if (!test_bit(NVDIMM_INTEL_SET_MASTER_PASSPHRASE,
+				&nfit_mem->dsm_mask))
+		return -ENOTTY;
+
+	if (old_data)
+		memcpy(nd_cmd.cmd.old_pass, old_data->data,
+				sizeof(nd_cmd.cmd.old_pass));
+	else
+		memset(nd_cmd.cmd.old_pass, 0, sizeof(nd_cmd.cmd.old_pass));
+	memcpy(nd_cmd.cmd.new_pass, new_data->data,
+			sizeof(nd_cmd.cmd.new_pass));
+	rc = nvdimm_ctl(nvdimm, ND_CMD_CALL, &nd_cmd, sizeof(nd_cmd), &cmd_rc);
+	if (rc < 0)
+		goto out;
+	if (cmd_rc < 0) {
+		rc = cmd_rc;
+		goto out;
+	}
+
+	switch (nd_cmd.cmd.status) {
+	case 0:
+		break;
+	case ND_INTEL_STATUS_NOT_SUPPORTED:
+		rc = -EOPNOTSUPP;
+		goto out;
+	case ND_INTEL_STATUS_INVALID_PASS:
+		rc = -EINVAL;
+		goto out;
+	case ND_INTEL_STATUS_INVALID_STATE:
+	default:
+		rc = -ENXIO;
+		goto out;
+	}
+
+ out:
+	return rc;
+}
 static int intel_dimm_security_query_overwrite(struct nvdimm *nvdimm)
 {
 	int cmd_rc, rc = 0;
@@ -397,7 +514,8 @@ static int intel_dimm_security_unlock(struct nvdimm *nvdimm,
 }
 
 static int intel_dimm_security_state(struct nvdimm *nvdimm,
-		enum nvdimm_security_state *state)
+		enum nvdimm_security_state *state,
+		enum nvdimm_security_extended_state *ext_state)
 {
 	int cmd_rc, rc = 0;
 	struct nfit_mem *nfit_mem = nvdimm_provider_data(nvdimm);
@@ -465,9 +583,17 @@ static int intel_dimm_security_state(struct nvdimm *nvdimm,
 	} else
 		*state = NVDIMM_SECURITY_DISABLED;
 
+	*ext_state = NVDIMM_SECURITY_MASTER_DISABLED;
+	if (nd_cmd.cmd.extended_state & ND_INTEL_SEC_ESTATE_ENABLED)
+		*ext_state = NVDIMM_SECURITY_MASTER_ENABLED;
+	else if (nd_cmd.cmd.extended_state & ND_INTEL_SEC_ESTATE_PLIMIT)
+		*ext_state = NVDIMM_SECURITY_MASTER_FROZEN;
+
  out:
-	if (rc < 0)
+	if (rc < 0) {
 		*state = NVDIMM_SECURITY_INVALID;
+		*ext_state = NVDIMM_SECURITY_MASTER_INVALID;
+	}
 	return rc;
 }
 
@@ -480,5 +606,7 @@ static const struct nvdimm_security_ops __intel_security_ops = {
 	.erase = intel_dimm_security_erase,
 	.overwrite = intel_dimm_security_overwrite,
 	.query_overwrite = intel_dimm_security_query_overwrite,
+	.master_change_key = intel_dimm_security_master_update_passphrase,
+	.master_erase = intel_dimm_security_master_erase,
 };
 const struct nvdimm_security_ops *intel_security_ops = &__intel_security_ops;
diff --git a/drivers/acpi/nfit/intel.h b/drivers/acpi/nfit/intel.h
index 0140d9b6a0b7..0c33f49d0272 100644
--- a/drivers/acpi/nfit/intel.h
+++ b/drivers/acpi/nfit/intel.h
@@ -46,6 +46,7 @@ extern const struct nvdimm_security_ops *intel_security_ops;
 #define ND_INTEL_STATUS_SIZE		4
 #define ND_INTEL_PASSPHRASE_SIZE	32
 
+#define ND_INTEL_STATUS_NOT_SUPPORTED	1
 #define ND_INTEL_STATUS_RETRY		5
 #define ND_INTEL_STATUS_NOT_READY	9
 #define ND_INTEL_STATUS_INVALID_STATE	10
@@ -61,9 +62,13 @@ extern const struct nvdimm_security_ops *intel_security_ops;
 #define ND_INTEL_SEC_STATE_UNSUPPORTED	0x20
 #define ND_INTEL_SEC_STATE_OVERWRITE	0x40
 
+#define ND_INTEL_SEC_ESTATE_ENABLED	0x01
+#define ND_INTEL_SEC_ESTATE_PLIMIT	0x02
+
 struct nd_intel_get_security_state {
 	u32 status;
-	u32 reserved;
+	u8 extended_state;
+	u8 reserved[3];
 	u8 state;
 	u8 reserved1[3];
 } __packed;
@@ -101,6 +106,17 @@ struct nd_intel_overwrite {
 struct nd_intel_query_overwrite {
 	u32 status;
 } __packed;
+
+struct nd_intel_set_master_passphrase {
+	u8 old_pass[ND_INTEL_PASSPHRASE_SIZE];
+	u8 new_pass[ND_INTEL_PASSPHRASE_SIZE];
+	u32 status;
+} __packed;
+
+struct nd_intel_master_secure_erase {
+	u8 passphrase[ND_INTEL_PASSPHRASE_SIZE];
+	u32 status;
+} __packed;
 #else /* CONFIG_X86 */
 #define intel_security_ops (NULL)
 #endif /* CONFIG_X86 */
diff --git a/drivers/acpi/nfit/nfit.h b/drivers/acpi/nfit/nfit.h
index d50dbb4cdeaf..e0ee54049c89 100644
--- a/drivers/acpi/nfit/nfit.h
+++ b/drivers/acpi/nfit/nfit.h
@@ -68,13 +68,17 @@ enum nvdimm_family_cmds {
 	NVDIMM_INTEL_SECURE_ERASE = 24,
 	NVDIMM_INTEL_OVERWRITE = 25,
 	NVDIMM_INTEL_QUERY_OVERWRITE = 26,
+	NVDIMM_INTEL_SET_MASTER_PASSPHRASE = 27,
+	NVDIMM_INTEL_MASTER_SECURE_ERASE = 28,
 };
 
 #define NVDIMM_INTEL_SECURITY_CMDMASK \
 (1 << NVDIMM_INTEL_GET_SECURITY_STATE | 1 << NVDIMM_INTEL_SET_PASSPHRASE \
 | 1 << NVDIMM_INTEL_DISABLE_PASSPHRASE | 1 << NVDIMM_INTEL_UNLOCK_UNIT \
 | 1 << NVDIMM_INTEL_FREEZE_LOCK | 1 << NVDIMM_INTEL_SECURE_ERASE \
-| 1 << NVDIMM_INTEL_OVERWRITE | 1 << NVDIMM_INTEL_QUERY_OVERWRITE)
+| 1 << NVDIMM_INTEL_OVERWRITE | 1 << NVDIMM_INTEL_QUERY_OVERWRITE \
+| 1 << NVDIMM_INTEL_SET_MASTER_PASSPHRASE \
+| 1 << NVDIMM_INTEL_MASTER_SECURE_ERASE)
 
 #define NVDIMM_INTEL_CMDMASK \
 (NVDIMM_STANDARD_CMDMASK | 1 << NVDIMM_INTEL_GET_MODES \
diff --git a/drivers/nvdimm/dimm_devs.c b/drivers/nvdimm/dimm_devs.c
index 39e40074b5df..fdb32c30d01d 100644
--- a/drivers/nvdimm/dimm_devs.c
+++ b/drivers/nvdimm/dimm_devs.c
@@ -421,7 +421,8 @@ static ssize_t security_store(struct device *dev,
 		if (rc != 3)
 			return -EINVAL;
 		dev_dbg(dev, "update %#x %#x\n", old_key, new_key);
-		rc = nvdimm_security_change_key(nvdimm, old_key, new_key);
+		rc = nvdimm_security_change_key(nvdimm, old_key, new_key,
+				false);
 	} else if (sysfs_streq(cmd, "disable")) {
 		if (rc != 2)
 			return -EINVAL;
@@ -434,12 +435,23 @@ static ssize_t security_store(struct device *dev,
 		if (rc != 2)
 			return -EINVAL;
 		dev_dbg(dev, "erase %#x\n", old_key);
-		rc = nvdimm_security_erase(nvdimm, old_key);
+		rc = nvdimm_security_erase(nvdimm, old_key, false);
 	} else if (sysfs_streq(cmd, "overwrite")) {
 		if (rc != 2)
 			return -EINVAL;
 		dev_dbg(dev, "overwrite %u\n", old_key);
 		rc = nvdimm_security_overwrite(nvdimm, old_key);
+	} else if (sysfs_streq(cmd, "master_update")) {
+		if (rc != 3)
+			return -EINVAL;
+		dev_dbg(dev, "master_update %#x %#x\n", old_key, new_key);
+		rc = nvdimm_security_change_key(nvdimm,
+				old_key, new_key, true);
+	} else if (sysfs_streq(cmd, "master_erase")) {
+		if (rc != 2)
+			return -EINVAL;
+		dev_dbg(dev, "master_erase %#x\n", old_key);
+		rc = nvdimm_security_erase(nvdimm, old_key, true);
 	} else
 		return -EINVAL;
 
diff --git a/drivers/nvdimm/nd-core.h b/drivers/nvdimm/nd-core.h
index b96e1b10e3eb..98facc66dd88 100644
--- a/drivers/nvdimm/nd-core.h
+++ b/drivers/nvdimm/nd-core.h
@@ -46,6 +46,7 @@ struct nvdimm {
 	const char *dimm_id;
 	const struct nvdimm_security_ops *security_ops;
 	enum nvdimm_security_state state;
+	enum nvdimm_security_extended_state ext_state;
 	struct mutex sec_mutex;
 	struct delayed_work dwork;
 	unsigned int overwrite_tmo;
diff --git a/drivers/nvdimm/nd.h b/drivers/nvdimm/nd.h
index ba41582c4cc2..11a268ffa63d 100644
--- a/drivers/nvdimm/nd.h
+++ b/drivers/nvdimm/nd.h
@@ -431,10 +431,11 @@ bool pmem_should_map_pages(struct device *dev);
 int nvdimm_security_unlock_dimm(struct nvdimm *nvdimm);
 int nvdimm_security_get_state(struct nvdimm *nvdimm);
 int nvdimm_security_change_key(struct nvdimm *nvdimm, unsigned int old_keyid,
-		unsigned int new_keyid);
+		unsigned int new_keyid, bool master);
 int nvdimm_security_disable(struct nvdimm *nvdimm, unsigned int keyid);
 int nvdimm_security_freeze_lock(struct nvdimm *nvdimm);
-int nvdimm_security_erase(struct nvdimm *nvdimm, unsigned int keyid);
+int nvdimm_security_erase(struct nvdimm *nvdimm, unsigned int keyid,
+		bool master);
 int nvdimm_security_overwrite(struct nvdimm *nvdimm, unsigned int keyid);
 void nvdimm_overwrite_query(struct work_struct *work);
 
diff --git a/drivers/nvdimm/security.c b/drivers/nvdimm/security.c
index f5ba633545b7..2e835e6a7b74 100644
--- a/drivers/nvdimm/security.c
+++ b/drivers/nvdimm/security.c
@@ -90,7 +90,8 @@ int nvdimm_security_get_state(struct nvdimm *nvdimm)
 	if (!nvdimm->security_ops)
 		return 0;
 
-	return nvdimm->security_ops->state(nvdimm, &nvdimm->state);
+	return nvdimm->security_ops->state(nvdimm, &nvdimm->state,
+			&nvdimm->ext_state);
 }
 
 void nvdimm_overwrite_query(struct work_struct *work)
@@ -211,7 +212,8 @@ int nvdimm_security_overwrite(struct nvdimm *nvdimm, unsigned int keyid)
 
 }
 
-int nvdimm_security_erase(struct nvdimm *nvdimm, unsigned int keyid)
+int nvdimm_security_erase(struct nvdimm *nvdimm, unsigned int keyid,
+		bool master)
 {
 	int rc = 0;
 	struct key *key;
@@ -243,6 +245,12 @@ int nvdimm_security_erase(struct nvdimm *nvdimm, unsigned int keyid)
 		goto out;
 	}
 
+	if (nvdimm->ext_state != NVDIMM_SECURITY_MASTER_ENABLED && master) {
+		dev_warn(dev, "Attempt to secure erase in wrong master state.\n");
+		rc = -EOPNOTSUPP;
+		goto out;
+	}
+
 	key = nvdimm_lookup_user_key(dev, keyid);
 	if (!key) {
 		dev_dbg(dev, "Unable to get and verify key\n");
@@ -253,8 +261,12 @@ int nvdimm_security_erase(struct nvdimm *nvdimm, unsigned int keyid)
 	down_read(&key->sem);
 	epayload = dereference_key_locked(key);
 	data = epayload->decrypted_data;
-	rc = nvdimm->security_ops->erase(nvdimm,
-			(const struct nvdimm_key_data *)data);
+	if (master)
+		rc = nvdimm->security_ops->master_erase(nvdimm,
+				(const struct nvdimm_key_data *)data);
+	else
+		rc = nvdimm->security_ops->erase(nvdimm,
+				(const struct nvdimm_key_data *)data);
 	up_read(&key->sem);
 	key_put(key);
 
@@ -454,7 +466,7 @@ int nvdimm_security_unlock_dimm(struct nvdimm *nvdimm)
 }
 
 int nvdimm_security_change_key(struct nvdimm *nvdimm,
-		unsigned int old_keyid, unsigned int new_keyid)
+		unsigned int old_keyid, unsigned int new_keyid, bool master)
 {
 	int rc;
 	struct key *key = NULL, *old_key = NULL;
@@ -468,6 +480,10 @@ int nvdimm_security_change_key(struct nvdimm *nvdimm,
 	if (nvdimm->state == NVDIMM_SECURITY_FROZEN)
 		return -EBUSY;
 
+	/* can only change master passphrase when security is disabled */
+	if (nvdimm->ext_state != NVDIMM_SECURITY_MASTER_ENABLED && master)
+		return -EOPNOTSUPP;
+
 	mutex_lock(&nvdimm->sec_mutex);
 	rc = nvdimm_check_security_busy(nvdimm);
 	if (rc < 0)
@@ -499,8 +515,12 @@ int nvdimm_security_change_key(struct nvdimm *nvdimm,
 		old_data = old_epayload->decrypted_data;
 	}
 
-	rc = nvdimm->security_ops->change_key(nvdimm, old_data,
-			new_data);
+	if (master)
+		rc = nvdimm->security_ops->master_change_key(nvdimm, old_data,
+				new_data);
+	else
+		rc = nvdimm->security_ops->change_key(nvdimm, old_data,
+				new_data);
 	if (rc)
 		dev_warn(dev, "key update failed: %d\n", rc);
 
diff --git a/include/linux/libnvdimm.h b/include/linux/libnvdimm.h
index c3c5a1c6b1b7..3d14f73c7bca 100644
--- a/include/linux/libnvdimm.h
+++ b/include/linux/libnvdimm.h
@@ -167,6 +167,13 @@ enum nvdimm_security_state {
 	NVDIMM_SECURITY_UNSUPPORTED,
 };
 
+enum nvdimm_security_extended_state {
+	NVDIMM_SECURITY_MASTER_INVALID = 0,
+	NVDIMM_SECURITY_MASTER_DISABLED,
+	NVDIMM_SECURITY_MASTER_ENABLED,
+	NVDIMM_SECURITY_MASTER_FROZEN,
+};
+
 #define NVDIMM_PASSPHRASE_LEN		32
 #define NVDIMM_KEY_DESC_LEN		22
 
@@ -176,7 +183,8 @@ struct nvdimm_key_data {
 
 struct nvdimm_security_ops {
 	int (*state)(struct nvdimm *nvdimm,
-			enum nvdimm_security_state *state);
+			enum nvdimm_security_state *state,
+			enum nvdimm_security_extended_state *ex_state);
 	int (*unlock)(struct nvdimm *nvdimm,
 			const struct nvdimm_key_data *nkey);
 	int (*change_key)(struct nvdimm *nvdimm,
@@ -190,6 +198,11 @@ struct nvdimm_security_ops {
 	int (*overwrite)(struct nvdimm *nvdimm,
 			const struct nvdimm_key_data *nkey);
 	int (*query_overwrite)(struct nvdimm *nvdimm);
+	int (*master_change_key)(struct nvdimm *nvdimm,
+			const struct nvdimm_key_data *old_key,
+			const struct nvdimm_key_data *new_key);
+	int (*master_erase)(struct nvdimm *nvdimm,
+			const struct nvdimm_key_data *nkey);
 };
 
 void badrange_init(struct badrange *badrange);

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

[PATCH 10/11] tools/testing/nvdimm: add Intel DSM 1.8 support for nfit_test

[Dave Jiang]

Adding test support for new Intel DSM from v1.8. The ability of simulating
master passphrase update and master secure erase have been added to
nfit_test.

Signed-off-by: Dave Jiang <dave.jiang@intel.com>
---
 tools/testing/nvdimm/test/nfit.c |   86 ++++++++++++++++++++++++++++++++++++++
 1 file changed, 86 insertions(+)

diff --git a/tools/testing/nvdimm/test/nfit.c b/tools/testing/nvdimm/test/nfit.c
index c885fe136f42..602f6703614e 100644
--- a/tools/testing/nvdimm/test/nfit.c
+++ b/tools/testing/nvdimm/test/nfit.c
@@ -148,7 +148,9 @@ static unsigned long dimm_fail_cmd_flags[NUM_DCR];
 static int dimm_fail_cmd_code[NUM_DCR];
 struct nfit_test_sec {
 	u8 state;
+	u8 ext_state;
 	u8 passphrase[32];
+	u8 master_passphrase[32];
 	u64 overwrite_end_time;
 } dimm_sec_info[NUM_DCR];
 
@@ -951,6 +953,7 @@ static int nd_intel_test_cmd_security_status(struct nfit_test *t,
 
 	nd_cmd->status = 0;
 	nd_cmd->state = sec->state;
+	nd_cmd->extended_state = sec->ext_state;
 	dev_dbg(dev, "security state (%#x) returned\n", nd_cmd->state);
 
 	return 0;
@@ -1067,7 +1070,9 @@ static int nd_intel_test_cmd_secure_erase(struct nfit_test *t,
 		dev_dbg(dev, "secure erase: wrong passphrase\n");
 	} else {
 		memset(sec->passphrase, 0, ND_INTEL_PASSPHRASE_SIZE);
+		memset(sec->master_passphrase, 0, ND_INTEL_PASSPHRASE_SIZE);
 		sec->state = 0;
+		sec->ext_state = ND_INTEL_SEC_ESTATE_ENABLED;
 		dev_dbg(dev, "secure erase: done\n");
 	}
 
@@ -1112,12 +1117,69 @@ static int nd_intel_test_cmd_query_overwrite(struct nfit_test *t,
 	if (time_is_before_jiffies64(sec->overwrite_end_time)) {
 		sec->overwrite_end_time = 0;
 		sec->state = 0;
+		sec->ext_state = ND_INTEL_SEC_ESTATE_ENABLED;
 		dev_dbg(dev, "overwrite is complete\n");
 	} else
 		nd_cmd->status = ND_INTEL_STATUS_OQUERY_INPROGRESS;
 	return 0;
 }
 
+static int nd_intel_test_cmd_master_set_pass(struct nfit_test *t,
+		struct nd_intel_set_master_passphrase *nd_cmd,
+		unsigned int buf_len, int dimm)
+{
+	struct device *dev = &t->pdev.dev;
+	struct nfit_test_sec *sec = &dimm_sec_info[dimm];
+
+	if (!(sec->ext_state & ND_INTEL_SEC_ESTATE_ENABLED)) {
+		nd_cmd->status = ND_INTEL_STATUS_NOT_SUPPORTED;
+		dev_dbg(dev, "master set passphrase in wrong state\n");
+	} else if (sec->ext_state & ND_INTEL_SEC_ESTATE_PLIMIT) {
+		nd_cmd->status = ND_INTEL_STATUS_INVALID_STATE;
+		dev_dbg(dev, "master set passphrase in wrong security state\n");
+	} else if (memcmp(nd_cmd->old_pass, sec->master_passphrase,
+				ND_INTEL_PASSPHRASE_SIZE) != 0) {
+		nd_cmd->status = ND_INTEL_STATUS_INVALID_PASS;
+		dev_dbg(dev, "master set passphrase wrong passphrase\n");
+	} else {
+		memcpy(sec->master_passphrase, nd_cmd->new_pass,
+				ND_INTEL_PASSPHRASE_SIZE);
+		nd_cmd->status = 0;
+		dev_dbg(dev, "master passphrase updated\n");
+	}
+
+	return 0;
+}
+
+static int nd_intel_test_cmd_master_secure_erase(struct nfit_test *t,
+		struct nd_intel_master_secure_erase *nd_cmd,
+		unsigned int buf_len, int dimm)
+{
+	struct device *dev = &t->pdev.dev;
+	struct nfit_test_sec *sec = &dimm_sec_info[dimm];
+
+	if (!(sec->ext_state & ND_INTEL_SEC_ESTATE_ENABLED)) {
+		nd_cmd->status = ND_INTEL_STATUS_NOT_SUPPORTED;
+		dev_dbg(dev, "master erase in wrong state\n");
+	} else if (sec->ext_state & ND_INTEL_SEC_ESTATE_PLIMIT) {
+		nd_cmd->status = ND_INTEL_STATUS_INVALID_STATE;
+		dev_dbg(dev, "master erase in wrong security state\n");
+	} else if (memcmp(nd_cmd->passphrase, sec->master_passphrase,
+				ND_INTEL_PASSPHRASE_SIZE) != 0) {
+		nd_cmd->status = ND_INTEL_STATUS_INVALID_PASS;
+		dev_dbg(dev, "master secure erase: wrong passphrase\n");
+	} else {
+		memset(sec->master_passphrase, 0, ND_INTEL_PASSPHRASE_SIZE);
+		sec->ext_state = ND_INTEL_SEC_ESTATE_ENABLED;
+		memset(sec->passphrase, 0, ND_INTEL_PASSPHRASE_SIZE);
+		sec->state = 0;
+		dev_dbg(dev, "master secure erase: done\n");
+	}
+
+	return 0;
+}
+
+
 static int get_dimm(struct nfit_mem *nfit_mem, unsigned int func)
 {
 	int i;
@@ -1197,6 +1259,14 @@ static int nfit_test_ctl(struct nvdimm_bus_descriptor *nd_desc,
 				rc = nd_intel_test_cmd_query_overwrite(t,
 						buf, buf_len, i - t->dcr_idx);
 				break;
+			case NVDIMM_INTEL_SET_MASTER_PASSPHRASE:
+				rc = nd_intel_test_cmd_master_set_pass(t,
+						buf, buf_len, i);
+				break;
+			case NVDIMM_INTEL_MASTER_SECURE_ERASE:
+				rc = nd_intel_test_cmd_master_secure_erase(t,
+						buf, buf_len, i);
+				break;
 			case ND_INTEL_ENABLE_LSS_STATUS:
 				rc = nd_intel_test_cmd_set_lss_status(t,
 						buf, buf_len);
@@ -1575,6 +1645,17 @@ static int nfit_test_dimm_init(struct nfit_test *t)
 	return 0;
 }
 
+static void security_init(struct nfit_test *t)
+{
+	int i;
+
+	for (i = 0; i < t->num_dcr; i++) {
+		struct nfit_test_sec *sec = &dimm_sec_info[i];
+
+		sec->ext_state = ND_INTEL_SEC_ESTATE_ENABLED;
+	}
+}
+
 static void smart_init(struct nfit_test *t)
 {
 	int i;
@@ -1653,6 +1734,7 @@ static int nfit_test0_alloc(struct nfit_test *t)
 	if (nfit_test_dimm_init(t))
 		return -ENOMEM;
 	smart_init(t);
+	security_init(t);
 	return ars_state_init(&t->pdev.dev, &t->ars_state);
 }
 
@@ -2434,6 +2516,10 @@ static void nfit_test0_setup(struct nfit_test *t)
 	set_bit(NVDIMM_INTEL_SECURE_ERASE, &acpi_desc->dimm_cmd_force_en);
 	set_bit(NVDIMM_INTEL_OVERWRITE, &acpi_desc->dimm_cmd_force_en);
 	set_bit(NVDIMM_INTEL_QUERY_OVERWRITE, &acpi_desc->dimm_cmd_force_en);
+	set_bit(NVDIMM_INTEL_SET_MASTER_PASSPHRASE,
+			&acpi_desc->dimm_cmd_force_en);
+	set_bit(NVDIMM_INTEL_MASTER_SECURE_ERASE,
+			&acpi_desc->dimm_cmd_force_en);
 }
 
 static void nfit_test1_setup(struct nfit_test *t)

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

[PATCH 11/11] acpi/nfit: prevent indiscriminate DSM payload dumping for security DSMs

[Dave Jiang]

Right now when debug is enabled, we dump the command buffer
indescriminately. This exposes the clear text payload for security DSMs.
Introducing a kernel config to only dump the payload if the config option
is turned on so the production kernels can leave this option off and not
expose the passphrases.

Signed-off-by: Dave Jiang <dave.jiang@intel.com>
---
 drivers/acpi/nfit/Kconfig |    7 +++++++
 drivers/acpi/nfit/core.c  |   24 +++++++++++++++++++++---
 2 files changed, 28 insertions(+), 3 deletions(-)

diff --git a/drivers/acpi/nfit/Kconfig b/drivers/acpi/nfit/Kconfig
index f7c57e33499e..a0a8eabda2e8 100644
--- a/drivers/acpi/nfit/Kconfig
+++ b/drivers/acpi/nfit/Kconfig
@@ -13,3 +13,10 @@ config ACPI_NFIT
 
 	  To compile this driver as a module, choose M here:
 	  the module will be called nfit.
+
+config NFIT_SECURITY_DEBUG
+	bool "Turn on debugging for NVDIMM security features"
+	depends on ACPI_NFIT
+	help
+	  Turning on debug output for NVDIMM security DSM commands. This
+	  should not be turned on on a production kernel.
diff --git a/drivers/acpi/nfit/core.c b/drivers/acpi/nfit/core.c
index 867e6fea3737..baaf5308de35 100644
--- a/drivers/acpi/nfit/core.c
+++ b/drivers/acpi/nfit/core.c
@@ -405,6 +405,21 @@ static u8 nfit_dsm_revid(unsigned family, unsigned func)
 	return id;
 }
 
+static bool is_security_cmd(unsigned int cmd, unsigned int func,
+		unsigned int family)
+{
+	if (cmd != ND_CMD_CALL)
+		return false;
+
+	if (family == NVDIMM_FAMILY_INTEL) {
+		if (func >= NVDIMM_INTEL_GET_SECURITY_STATE &&
+				func <= NVDIMM_INTEL_MASTER_SECURE_ERASE)
+			return true;
+	}
+
+	return false;
+}
+
 int acpi_nfit_ctl(struct nvdimm_bus_descriptor *nd_desc, struct nvdimm *nvdimm,
 		unsigned int cmd, void *buf, unsigned int buf_len, int *cmd_rc)
 {
@@ -489,9 +504,12 @@ int acpi_nfit_ctl(struct nvdimm_bus_descriptor *nd_desc, struct nvdimm *nvdimm,
 
 	dev_dbg(dev, "%s cmd: %d: func: %d input length: %d\n",
 		dimm_name, cmd, func, in_buf.buffer.length);
-	print_hex_dump_debug("nvdimm in  ", DUMP_PREFIX_OFFSET, 4, 4,
-			in_buf.buffer.pointer,
-			min_t(u32, 256, in_buf.buffer.length), true);
+	if ((call_pkg && !is_security_cmd(cmd, func, call_pkg->nd_family)) ||
+			IS_ENABLED(CONFIG_NFIT_SECURITY_DEBUG)) {
+		print_hex_dump_debug("nvdimm in  ", DUMP_PREFIX_OFFSET, 4, 4,
+				in_buf.buffer.pointer,
+				min_t(u32, 256, in_buf.buffer.length), true);
+	}
 
 	/* call the BIOS, prefer the named methods over _DSM if available */
 	if (nvdimm && cmd == ND_CMD_GET_CONFIG_SIZE

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

Re: [PATCH 02/11] libnvdimm/security: change clear text nvdimm keys to encrypted keys

[Dan Williams]

On Fri, Nov 9, 2018 at 2:13 PM Dave Jiang <dave.jiang@intel.com> wrote:
>
> In order to make nvdimm more secure, encrypted keys will be used instead of
> clear text keys. A master key will be created to seal encrypted nvdimm
> keys. The master key can be a trusted key generated from TPM 2.0 or a less
> secure user key.
>
> In the process of this conversion, the kernel cached key will be removed
> in order to simplify the verification process. The hardware will be used to
> verify the decrypted user payload directly.
>
> Signed-off-by: Dave Jiang <dave.jiang@intel.com>
> ---
>  Documentation/nvdimm/security.txt |   29 ++-
>  drivers/nvdimm/dimm.c             |    3
>  drivers/nvdimm/dimm_devs.c        |    2
>  drivers/nvdimm/nd-core.h          |    3
>  drivers/nvdimm/nd.h               |    5 -
>  drivers/nvdimm/security.c         |  316 ++++++++++---------------------------
>  6 files changed, 108 insertions(+), 250 deletions(-)

Remove twice the amount of code that it adds and gains features /
security, nice!
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

RE: [PATCH 07/11] libnvdimm/security: add overwrite status notification

[Elliott, Robert (Persistent Memory)]

> -----Original Message-----
> From: Linux-nvdimm <linux-nvdimm-bounces@lists.01.org> On Behalf Of
> Dave Jiang
> Sent: Friday, November 09, 2018 4:14 PM
> Subject: [PATCH 07/11] libnvdimm/security: add overwrite status
> notification
> 
...
> @@ -2033,6 +2033,11 @@ static int acpi_nfit_register_dimms(struct
> acpi_nfit_desc *acpi_desc)
>  		if (!nvdimm)
>  			continue;
> 
> +		rc = nvdimm_setup_security_events(nvdimm);
> +		if (rc < 0)
> +			dev_warn(acpi_desc->dev,
> +					"no security event setup
> failed\n");

That seems like a double negative.

Printing the rc value (or better yet, the string for it)
is always helpful too.

---
Robert Elliott, HPE Persistent Memory


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

RE: [PATCH 10/11] tools/testing/nvdimm: add Intel DSM 1.8 support for nfit_test

[Elliott, Robert (Persistent Memory)]

> -----Original Message-----
> From: Linux-nvdimm <linux-nvdimm-bounces@lists.01.org> On Behalf Of
> Dave Jiang
> Sent: Friday, November 09, 2018 4:15 PM
> To: dan.j.williams@intel.com; zohar@linux.vnet.ibm.com
> Cc: linux-nvdimm@lists.01.org
> Subject: [PATCH 10/11] tools/testing/nvdimm: add Intel DSM 1.8
> support for nfit_test
...
> +static int nd_intel_test_cmd_master_set_pass(struct nfit_test *t,
> +		struct nd_intel_set_master_passphrase *nd_cmd,
> +		unsigned int buf_len, int dimm)
> +{
> +	struct device *dev = &t->pdev.dev;
> +	struct nfit_test_sec *sec = &dimm_sec_info[dimm];
> +
> +	if (!(sec->ext_state & ND_INTEL_SEC_ESTATE_ENABLED)) {
> +		nd_cmd->status = ND_INTEL_STATUS_NOT_SUPPORTED;
> +		dev_dbg(dev, "master set passphrase in wrong state\n");

"master set passphrase:" for consistency

> +	} else if (sec->ext_state & ND_INTEL_SEC_ESTATE_PLIMIT) {
> +		nd_cmd->status = ND_INTEL_STATUS_INVALID_STATE;
> +		dev_dbg(dev, "master set passphrase in wrong security
> state\n");

"master set passphrase:" for consistency

> +	} else if (memcmp(nd_cmd->old_pass, sec->master_passphrase,
> +				ND_INTEL_PASSPHRASE_SIZE) != 0) {
> +		nd_cmd->status = ND_INTEL_STATUS_INVALID_PASS;
> +		dev_dbg(dev, "master set passphrase wrong
> passphrase\n");

"master set passphrase:" for consistency

> +	} else {
> +		memcpy(sec->master_passphrase, nd_cmd->new_pass,
> +				ND_INTEL_PASSPHRASE_SIZE);
> +		nd_cmd->status = 0;
> +		dev_dbg(dev, "master passphrase updated\n");

"master set passphrase:" for consistency

> +	}
> +
> +	return 0;
> +}
> +
> +static int nd_intel_test_cmd_master_secure_erase(struct nfit_test
> *t,
> +		struct nd_intel_master_secure_erase *nd_cmd,
> +		unsigned int buf_len, int dimm)
> +{
> +	struct device *dev = &t->pdev.dev;
> +	struct nfit_test_sec *sec = &dimm_sec_info[dimm];
> +
> +	if (!(sec->ext_state & ND_INTEL_SEC_ESTATE_ENABLED)) {
> +		nd_cmd->status = ND_INTEL_STATUS_NOT_SUPPORTED;
> +		dev_dbg(dev, "master erase in wrong state\n");

"master secure erase: " for consistency

> +	} else if (sec->ext_state & ND_INTEL_SEC_ESTATE_PLIMIT) {
> +		nd_cmd->status = ND_INTEL_STATUS_INVALID_STATE;
> +		dev_dbg(dev, "master erase in wrong security state\n");

"master secure erase: " for consistency

> +	} else if (memcmp(nd_cmd->passphrase, sec->master_passphrase,
> +				ND_INTEL_PASSPHRASE_SIZE) != 0) {
> +		nd_cmd->status = ND_INTEL_STATUS_INVALID_PASS;
> +		dev_dbg(dev, "master secure erase: wrong passphrase\n");
> +	} else {
> +		memset(sec->master_passphrase, 0,
> ND_INTEL_PASSPHRASE_SIZE);
> +		sec->ext_state = ND_INTEL_SEC_ESTATE_ENABLED;
> +		memset(sec->passphrase, 0, ND_INTEL_PASSPHRASE_SIZE);
> +		sec->state = 0;
> +		dev_dbg(dev, "master secure erase: done\n");
> +	}
> +
> +	return 0;
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

Re: [PATCH 02/11] libnvdimm/security: change clear text nvdimm keys to encrypted keys

[Mimi Zohar]

On Fri, 2018-11-09 at 15:13 -0700, Dave Jiang wrote:
> In order to make nvdimm more secure, encrypted keys will be used instead of
> clear text keys. A master key will be created to seal encrypted nvdimm
> keys. The master key can be a trusted key generated from TPM 2.0 or a less
> secure user key.

Trusted keys also work for TPM 1.2.  Are you intentionally limiting
the master key to TPM 2.0?

Traditionally there is a single master key for the system, which would
be sealed to a set of boot time PCR values.  After decrypting all of
the encrypted keys, the master key would be removed from the keyring
and a PCR extended.  Extending a PCR would prevent the master key from
being unsealed again and used to decrypt encrypted keys, without
rebooting the system.  Normally this would be done before pivoting
root.

If you're not referring to the system master key and are intentionally
limiting usage to TPM 2.0, more details on the master key security
requirements should be included.

Using trusted keys that are encrypted/decrypted using a user key
should really be limited to testing environments.

> 
> In the process of this conversion, the kernel cached key will be removed
> in order to simplify the verification process. The hardware will be used to
> verify the decrypted user payload directly.

Making this sort of change implies there is no concern in breaking
existing userspace.  Either the code hasn't yet been upstreamed or
there are not any users. If the code hasn't been upstreamed, it would
make more sense to squash the git history:

- making code review easier
- making the git history bisect safe

Mimi

> 
> Signed-off-by: Dave Jiang <dave.jiang@intel.com>
> ---
>  Documentation/nvdimm/security.txt |   29 ++-
>  drivers/nvdimm/dimm.c             |    3 
>  drivers/nvdimm/dimm_devs.c        |    2 
>  drivers/nvdimm/nd-core.h          |    3 
>  drivers/nvdimm/nd.h               |    5 -
>  drivers/nvdimm/security.c         |  316 ++++++++++---------------------------
>  6 files changed, 108 insertions(+), 250 deletions(-)
> 
> diff --git a/Documentation/nvdimm/security.txt b/Documentation/nvdimm/security.txt
> index 50cbb6cb96a1..11240ce48755 100644
> --- a/Documentation/nvdimm/security.txt
> +++ b/Documentation/nvdimm/security.txt
> @@ -39,34 +39,39 @@ The DIMM id would be provided along with the key payload (passphrase) to
>  the kernel.
> 
>  The security keys are managed on the basis of a single key per DIMM. The
> -key "passphrase" is expected to be 32bytes long or padded to 32bytes. This is
> +key "passphrase" is expected to be 32bytes long. This is
>  similar to the ATA security specification [2]. A key is initially acquired
>  via the request_key() kernel API call and retrieved from userspace. It is up to
>  the user to provide an upcall application to retrieve the key in whatever
>  fashion meets their security requirements.
> 
> -A nvdimm user logon key has the description format of:
> +A nvdimm encrypted key has the description format of:
>  nvdimm:<bus-provider-specific-unique-id>
> 
> +See Documentation/security/keys/trusted-encrypted.rst for details on encrypted
> +keys. The encrypted key for the DIMM is generated from a master key that is
> +either a trusted key created via TPM2 or a less secure user key. The payload
> +for the nvdimm encrypted key is randomly generated by the kernel and is not
> +visible to the user. The user is responsible for retaining the encrypted key
> +blob generated from the encrypted key to be loaded at the next boot session.
> +
>  4. Unlocking
>  ------------
>  When the DIMMs are being enumerated by the kernel, the kernel will attempt to
> -retrieve the key from its keyring. If that fails, it will attempt to
>  acquire the key from the userspace upcall function. This is the only time
>  a locked DIMM can be unlocked. Once unlocked, the DIMM will remain unlocked
>  until reboot.
> 
>  5. Update
>  ---------
> -When doing an update, it is expected that the new key with the 64bit payload of
> -format described above is added via the keyutils API or utility. The update
> -command written to the sysfs attribute will be with the format:
> +When doing an update, it is expected that the new key is added via the
> +keyutils API or utility. The update command written to the sysfs attribute
> +will be with the format:
>  update <old keyid> <new keyid>
> 
> -It is expected that a user logon key has been injected via keyutils to provide
> -the payload for the update operation. The kernel will take the new user key,
> -attempt the update operation with the nvdimm, and replace the existing key's
> -payload with the new passphrase.
> +It is expected that a encrypted key has been injected via keyutils to provide
> +the payload for the update operation. The kernel will take the new user key
> +and the old user key to attempt the update operation.
> 
>  If there is no old key id due to a security enabling, then a 0 should be
>  passed in.  If a nvdimm has an existing passphrase, then an "old" key should
> @@ -80,7 +85,7 @@ frozen by a user with root privelege.
>  7. Disable
>  ----------
>  The security disable command format is:
> -disable <old keyid>
> +disable <current keyid>
> 
>  An "old" key with the passphrase payload that is tied to the nvdimm should be
>  injected with a key description that does not have the "nvdimm:" prefix and
> @@ -89,7 +94,7 @@ its keyid should be passed in via sysfs.
>  8. Secure Erase
>  ---------------
>  The command format for doing a secure erase is:
> -erase <old keyid>
> +erase <current keyid>
> 
>  An "old" key with the passphrase payload that is tied to the nvdimm should be
>  injected with a key description that does not have the "nvdimm:" prefix and
> diff --git a/drivers/nvdimm/dimm.c b/drivers/nvdimm/dimm.c
> index 225a8474c44a..c3886a22b5e4 100644
> --- a/drivers/nvdimm/dimm.c
> +++ b/drivers/nvdimm/dimm.c
> @@ -112,9 +112,6 @@ static int nvdimm_probe(struct device *dev)
>  static int nvdimm_remove(struct device *dev)
>  {
>  	struct nvdimm_drvdata *ndd = dev_get_drvdata(dev);
> -	struct nvdimm *nvdimm = to_nvdimm(dev);
> -
> -	nvdimm_security_release(nvdimm);
> 
>  	if (!ndd)
>  		return 0;
> diff --git a/drivers/nvdimm/dimm_devs.c b/drivers/nvdimm/dimm_devs.c
> index 5d34251c4f3b..269f401a5e68 100644
> --- a/drivers/nvdimm/dimm_devs.c
> +++ b/drivers/nvdimm/dimm_devs.c
> @@ -485,7 +485,7 @@ struct nvdimm *__nvdimm_create(struct nvdimm_bus *nvdimm_bus, void *provider_dat
>  	nvdimm->num_flush = num_flush;
>  	nvdimm->flush_wpq = flush_wpq;
>  	atomic_set(&nvdimm->busy, 0);
> -	mutex_init(&nvdimm->key_mutex);
> +	mutex_init(&nvdimm->sec_mutex);
>  	dev = &nvdimm->dev;
>  	dev_set_name(dev, "nmem%d", nvdimm->id);
>  	dev->parent = &nvdimm_bus->dev;
> diff --git a/drivers/nvdimm/nd-core.h b/drivers/nvdimm/nd-core.h
> index 6e2877996a9b..cfb87f71db8d 100644
> --- a/drivers/nvdimm/nd-core.h
> +++ b/drivers/nvdimm/nd-core.h
> @@ -45,8 +45,7 @@ struct nvdimm {
>  	const char *dimm_id;
>  	const struct nvdimm_security_ops *security_ops;
>  	enum nvdimm_security_state state;
> -	struct key *key;
> -	struct mutex key_mutex;
> +	struct mutex sec_mutex;
>  };
> 
>  /**
> diff --git a/drivers/nvdimm/nd.h b/drivers/nvdimm/nd.h
> index 892f127bfd38..f8d8f0a2a40d 100644
> --- a/drivers/nvdimm/nd.h
> +++ b/drivers/nvdimm/nd.h
> @@ -428,7 +428,6 @@ bool pmem_should_map_pages(struct device *dev);
> 
>  #ifdef CONFIG_NVDIMM_SECURITY
>  int nvdimm_security_unlock_dimm(struct nvdimm *nvdimm);
> -void nvdimm_security_release(struct nvdimm *nvdimm);
>  int nvdimm_security_get_state(struct nvdimm *nvdimm);
>  int nvdimm_security_change_key(struct nvdimm *nvdimm, unsigned int old_keyid,
>  		unsigned int new_keyid);
> @@ -441,10 +440,6 @@ static inline int nvdimm_security_unlock_dimm(struct nvdimm *nvdimm)
>  	return 0;
>  }
> 
> -static inline void nvdimm_security_release(struct nvdimm *nvdimm)
> -{
> -}
> -
>  static inline int nvdimm_security_get_state(struct nvdimm *nvdimm)
>  {
>  	return -EOPNOTSUPP;
> diff --git a/drivers/nvdimm/security.c b/drivers/nvdimm/security.c
> index 5aacc590b4c0..ee741199d623 100644
> --- a/drivers/nvdimm/security.c
> +++ b/drivers/nvdimm/security.c
> @@ -10,43 +10,10 @@
>  #include <linux/key.h>
>  #include <linux/key-type.h>
>  #include <keys/user-type.h>
> +#include <keys/encrypted-type.h>
>  #include "nd-core.h"
>  #include "nd.h"
> 
> -/*
> - * Replacing the user key with a kernel key. The function expects that
> - * we hold the sem for the key passed in. The function will release that
> - * sem when done process. We will also hold the sem for the valid new key
> - * returned.
> - */
> -static struct key *make_kernel_key(struct key *key)
> -{
> -	struct key *new_key;
> -	struct user_key_payload *payload;
> -	int rc;
> -
> -	new_key = key_alloc(&key_type_logon, key->description,
> -			GLOBAL_ROOT_UID, GLOBAL_ROOT_GID, current_cred(),
> -			KEY_POS_ALL & ~KEY_POS_SETATTR,
> -			KEY_ALLOC_NOT_IN_QUOTA, NULL);
> -	if (IS_ERR(new_key))
> -		return NULL;
> -
> -	payload = key->payload.data[0];
> -	rc = key_instantiate_and_link(new_key, payload->data,
> -			payload->datalen, NULL, NULL);
> -	up_read(&key->sem);
> -	if (rc < 0) {
> -		key_put(new_key);
> -		return NULL;
> -	}
> -
> -	key_put(key);
> -
> -	down_read(&new_key->sem);
> -	return new_key;
> -}
> -
>  /*
>   * Retrieve user injected key
>   */
> @@ -61,6 +28,10 @@ static struct key *nvdimm_lookup_user_key(struct device *dev,
>  		return NULL;
> 
>  	key = key_ref_to_ptr(keyref);
> +	if (key->type != &key_type_encrypted) {
> +		key_put(key);
> +		return NULL;
> +	}
>  	dev_dbg(dev, "%s: key found: %#x\n", __func__, key_serial(key));
> 
>  	return key;
> @@ -76,7 +47,7 @@ static struct key *nvdimm_request_key(struct nvdimm *nvdimm)
>  	struct device *dev = &nvdimm->dev;
> 
>  	sprintf(desc, "%s%s", NVDIMM_PREFIX, nvdimm->dimm_id);
> -	key = request_key(&key_type_logon, desc, "");
> +	key = request_key(&key_type_encrypted, desc, "");
>  	if (IS_ERR(key)) {
>  		if (PTR_ERR(key) == -ENOKEY)
>  			dev_warn(dev, "request_key() found no key\n");
> @@ -88,52 +59,6 @@ static struct key *nvdimm_request_key(struct nvdimm *nvdimm)
>  	return key;
>  }
> 
> -struct key *nvdimm_get_and_verify_key(struct nvdimm *nvdimm,
> -		unsigned int user_key_id)
> -{
> -	int rc;
> -	struct key *user_key, *key;
> -	struct device *dev = &nvdimm->dev;
> -	struct user_key_payload *upayload, *payload;
> -
> -	lockdep_assert_held(&nvdimm->key_mutex);
> -	key = nvdimm->key;
> -	if (!key) {
> -		dev_dbg(dev, "No cached kernel key\n");
> -		return ERR_PTR(-EAGAIN);;
> -	}
> -	dev_dbg(dev, "cached_key: %#x\n", key_serial(key));
> -
> -	user_key = nvdimm_lookup_user_key(dev, user_key_id);
> -	if (!user_key) {
> -		dev_dbg(dev, "Old user key lookup failed\n");
> -		return ERR_PTR(-EINVAL);
> -	}
> -	dev_dbg(dev, "user_key: %#x\n", key_serial(user_key));
> -
> -	down_read(&key->sem);
> -	down_read(&user_key->sem);
> -	payload = key->payload.data[0];
> -	upayload = user_key->payload.data[0];
> -
> -	rc = memcmp(payload->data, upayload->data, NVDIMM_PASSPHRASE_LEN);
> -	up_read(&user_key->sem);
> -	key_put(user_key);
> -	up_read(&key->sem);
> -
> -	if (rc != 0) {
> -		dev_warn(dev, "Supplied old user key fails check.\n");
> -		return ERR_PTR(-EINVAL);
> -	}
> -	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)
> @@ -146,14 +71,15 @@ int nvdimm_security_erase(struct nvdimm *nvdimm, unsigned int keyid)
>  {
>  	int rc = 0;
>  	struct key *key;
> -	struct user_key_payload *payload;
> +	struct encrypted_key_payload *epayload;
>  	struct device *dev = &nvdimm->dev;
> +	void *data;
> 
>  	if (!nvdimm->security_ops)
>  		return -EOPNOTSUPP;
> 
>  	nvdimm_bus_lock(dev);
> -	mutex_lock(&nvdimm->key_mutex);
> +	mutex_lock(&nvdimm->sec_mutex);
>  	if (atomic_read(&nvdimm->busy)) {
>  		dev_warn(dev, "Unable to secure erase while DIMM active.\n");
>  		rc = -EBUSY;
> @@ -166,26 +92,23 @@ int nvdimm_security_erase(struct nvdimm *nvdimm, unsigned int keyid)
>  		goto out;
>  	}
> 
> -	/* look for a key from cached key if exists */
> -	key = nvdimm_get_and_verify_key(nvdimm, keyid);
> -	if (IS_ERR(key)) {
> +	key = nvdimm_lookup_user_key(dev, keyid);
> +	if (!key) {
>  		dev_dbg(dev, "Unable to get and verify key\n");
> -		rc = PTR_ERR(key);
> +		rc = -ENOKEY;
>  		goto out;
>  	}
> 
>  	down_read(&key->sem);
> -	payload = key->payload.data[0];
> +	epayload = dereference_key_locked(key);
> +	data = epayload->decrypted_data;
>  	rc = nvdimm->security_ops->erase(nvdimm,
> -			(const struct nvdimm_key_data *)payload->data);
> +			(const struct nvdimm_key_data *)data);
>  	up_read(&key->sem);
> -
> -	/* remove key since secure erase kills the passphrase */
> -	key_destroy(key);
> -	nvdimm->key = NULL;
> +	key_put(key);
> 
>   out:
> -	mutex_unlock(&nvdimm->key_mutex);
> +	mutex_unlock(&nvdimm->sec_mutex);
>  	nvdimm_bus_unlock(dev);
>  	nvdimm_security_get_state(nvdimm);
>  	return rc;
> @@ -201,11 +124,15 @@ int nvdimm_security_freeze_lock(struct nvdimm *nvdimm)
>  	if (nvdimm->state == NVDIMM_SECURITY_UNSUPPORTED)
>  		return -EOPNOTSUPP;
> 
> +	mutex_lock(&nvdimm->sec_mutex);
>  	rc = nvdimm->security_ops->freeze_lock(nvdimm);
> -	if (rc < 0)
> +	if (rc < 0) {
> +		mutex_unlock(&nvdimm->sec_mutex);
>  		return rc;
> +	}
> 
>  	nvdimm_security_get_state(nvdimm);
> +	mutex_unlock(&nvdimm->sec_mutex);
>  	return 0;
>  }
> 
> @@ -213,8 +140,9 @@ int nvdimm_security_disable(struct nvdimm *nvdimm, unsigned int keyid)
>  {
>  	int rc;
>  	struct key *key;
> -	struct user_key_payload *payload;
> +	struct encrypted_key_payload *epayload;
>  	struct device *dev = &nvdimm->dev;
> +	void *data;
> 
>  	if (!nvdimm->security_ops)
>  		return -EOPNOTSUPP;
> @@ -222,57 +150,52 @@ int nvdimm_security_disable(struct nvdimm *nvdimm, unsigned int keyid)
>  	if (nvdimm->state == NVDIMM_SECURITY_UNSUPPORTED)
>  		return -EOPNOTSUPP;
> 
> -	mutex_lock(&nvdimm->key_mutex);
> +	mutex_lock(&nvdimm->sec_mutex);
>  	/* look for a key from cached key */
> -	key = nvdimm_get_and_verify_key(nvdimm, keyid);
> -	if (IS_ERR(key)) {
> -		mutex_unlock(&nvdimm->key_mutex);
> -		return PTR_ERR(key);
> +	key = nvdimm_lookup_user_key(dev, keyid);
> +	if (!key) {
> +		mutex_unlock(&nvdimm->sec_mutex);
> +		return -ENOKEY;
>  	}
> 
>  	down_read(&key->sem);
> -	payload = key->payload.data[0];
> +	epayload = dereference_key_locked(key);
> +	data = epayload->decrypted_data;
> 
>  	rc = nvdimm->security_ops->disable(nvdimm,
> -			(const struct nvdimm_key_data *)payload->data);
> +			(const struct nvdimm_key_data *)data);
>  	up_read(&key->sem);
> -	if (rc < 0) {
> +	if (rc < 0)
>  		dev_warn(dev, "security disable failed\n");
> -		goto out;
> -	}
> 
> -	/* If we succeed then remove the key */
> -	key_destroy(key);
> -	nvdimm->key = NULL;
> -
> - out:
> -	mutex_unlock(&nvdimm->key_mutex);
> +	key_put(key);
>  	nvdimm_security_get_state(nvdimm);
> +	mutex_unlock(&nvdimm->sec_mutex);
>  	return rc;
>  }
> 
> -static int nvdimm_self_verify_key(struct nvdimm *nvdimm)
> +static struct key *nvdimm_self_verify_key(struct nvdimm *nvdimm)
>  {
>  	struct key *key;
> -	struct user_key_payload *payload;
> +	struct encrypted_key_payload *epayload;
>  	void *data;
>  	int rc;
> 
> -	lockdep_assert_held(&nvdimm->key_mutex);
> -
> +	lockdep_assert_held(&nvdimm->sec_mutex);
>  	key = nvdimm_request_key(nvdimm);
>  	if (!key)
> -		return -ENOKEY;
> +		return NULL;
> +
> +	down_read(&key->sem);
> +	epayload = dereference_key_locked(key);
> +	data = epayload->decrypted_data;
> 
> -	if (key->datalen != NVDIMM_PASSPHRASE_LEN) {
> +	if (epayload->decrypted_datalen != NVDIMM_PASSPHRASE_LEN) {
>  		key_put(key);
> -		return -EINVAL;
> +		key = NULL;
> +		goto out;
>  	}
> 
> -	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.
> @@ -280,19 +203,21 @@ static int nvdimm_self_verify_key(struct nvdimm *nvdimm)
>  	rc = nvdimm->security_ops->change_key(nvdimm, data, data);
>  	if (rc < 0) {
>  		key_put(key);
> -		return rc;
> +		key = NULL;
>  	}
> +
> +out:
>  	up_read(&key->sem);
> -	nvdimm->key = key;
> -	return 0;
> +	return key;
>  }
> 
>  int nvdimm_security_unlock_dimm(struct nvdimm *nvdimm)
>  {
> -	struct key *key;
> -	int rc = -ENXIO;
> -	struct user_key_payload *payload;
> +	struct key *key = NULL;
> +	int rc;
> +	struct encrypted_key_payload *epayload;
>  	struct device *dev = &nvdimm->dev;
> +	void *data;
> 
>  	if (!nvdimm->security_ops)
>  		return 0;
> @@ -301,7 +226,7 @@ int nvdimm_security_unlock_dimm(struct nvdimm *nvdimm)
>  			nvdimm->state == NVDIMM_SECURITY_DISABLED)
>  		return 0;
> 
> -	mutex_lock(&nvdimm->key_mutex);
> +	mutex_lock(&nvdimm->sec_mutex);
>  	/*
>  	 * If the pre-OS has unlocked the DIMM, we will attempt to send
>  	 * the key from request_key() to the hardware for verification.
> @@ -310,60 +235,50 @@ int nvdimm_security_unlock_dimm(struct nvdimm *nvdimm)
>  	 * other security operations.
>  	 */
>  	if (nvdimm->state == NVDIMM_SECURITY_UNLOCKED) {
> -		rc = nvdimm_self_verify_key(nvdimm);
> -		if (rc < 0) {
> +		key = nvdimm_self_verify_key(nvdimm);
> +		if (!key) {
>  			rc = nvdimm_security_freeze_lock(nvdimm);
> -			mutex_unlock(&nvdimm->key_mutex);
> -			return rc;
> +			mutex_unlock(&nvdimm->sec_mutex);
> +			return -ENOKEY;
>  		}
>  	}
> 
> -	key = nvdimm->key;
> -	if (!key) {
> +	if (!key)
>  		key = nvdimm_request_key(nvdimm);
> -		if (key && key->datalen != NVDIMM_PASSPHRASE_LEN) {
> -			key_put(key);
> -			key = NULL;
> -			rc = -EINVAL;
> -		}
> -	}
> +
>  	if (!key) {
> -		mutex_unlock(&nvdimm->key_mutex);
> -		return rc;
> +		mutex_unlock(&nvdimm->sec_mutex);
> +		return -ENOKEY;
>  	}
> 
>  	dev_dbg(dev, "%s: key: %#x\n", __func__, key_serial(key));
>  	down_read(&key->sem);
> -	payload = key->payload.data[0];
> +	epayload = dereference_key_locked(key);
> +	data = epayload->decrypted_data;
> +
> +	if (epayload->decrypted_datalen != NVDIMM_PASSPHRASE_LEN) {
> +		up_read(&key->sem);
> +		key_put(key);
> +		mutex_unlock(&nvdimm->sec_mutex);
> +		return -EINVAL;
> +	}
> +
>  	rc = nvdimm->security_ops->unlock(nvdimm,
> -			(const struct nvdimm_key_data *)payload->data);
> +			(const struct nvdimm_key_data *)data);
>  	up_read(&key->sem);
> 
>  	if (rc == 0) {
> -		if (!nvdimm->key)
> -			nvdimm->key = key;
>  		nvdimm->state = NVDIMM_SECURITY_UNLOCKED;
>  		dev_dbg(dev, "DIMM %s unlocked\n", dev_name(dev));
> -	} else {
> -		key_invalidate(key);
> -		key_put(key);
> -		nvdimm->key = NULL;
> +	} else
>  		dev_warn(dev, "Failed to unlock dimm: %s\n", dev_name(dev));
> -	}
> 
> -	mutex_unlock(&nvdimm->key_mutex);
> +	key_put(key);
>  	nvdimm_security_get_state(nvdimm);
> +	mutex_unlock(&nvdimm->sec_mutex);
>  	return rc;
>  }
> 
> -void nvdimm_security_release(struct nvdimm *nvdimm)
> -{
> -	mutex_lock(&nvdimm->key_mutex);
> -	key_put(nvdimm->key);
> -	nvdimm->key = NULL;
> -	mutex_unlock(&nvdimm->key_mutex);
> -}
> -
>  int nvdimm_security_change_key(struct nvdimm *nvdimm,
>  		unsigned int old_keyid, unsigned int new_keyid)
>  {
> @@ -371,7 +286,7 @@ int nvdimm_security_change_key(struct nvdimm *nvdimm,
>  	struct key *key, *old_key;
>  	void *old_data = NULL, *new_data;
>  	struct device *dev = &nvdimm->dev;
> -	struct user_key_payload *payload, *old_payload;
> +	struct encrypted_key_payload *epayload, *old_epayload;
> 
>  	if (!nvdimm->security_ops)
>  		return -EOPNOTSUPP;
> @@ -379,15 +294,10 @@ int nvdimm_security_change_key(struct nvdimm *nvdimm,
>  	if (nvdimm->state == NVDIMM_SECURITY_FROZEN)
>  		return -EBUSY;
> 
> -	mutex_lock(&nvdimm->key_mutex);
> +	mutex_lock(&nvdimm->sec_mutex);
>  	/* look for a key from cached key if exists */
> -	old_key = nvdimm_get_and_verify_key(nvdimm, old_keyid);
> -	if (IS_ERR(old_key) && PTR_ERR(old_key) == -EAGAIN)
> -		old_key = NULL;
> -	else if (IS_ERR(old_key)) {
> -		mutex_unlock(&nvdimm->key_mutex);
> -		return PTR_ERR(old_key);
> -	} else
> +	old_key = nvdimm_lookup_user_key(dev, old_keyid);
> +	if (old_key)
>  		dev_dbg(dev, "%s: old key: %#x\n", __func__,
>  				key_serial(old_key));
> 
> @@ -402,76 +312,28 @@ int nvdimm_security_change_key(struct nvdimm *nvdimm,
>  	dev_dbg(dev, "%s: new key: %#x\n", __func__, key_serial(key));
> 
>  	down_read(&key->sem);
> -	payload = key->payload.data[0];
> -	if (payload->datalen != NVDIMM_PASSPHRASE_LEN) {
> -		rc = -EINVAL;
> -		up_read(&key->sem);
> -		goto out;
> -	}
> -
> -	/*
> -	 * Since there is no existing key this user key will become the
> -	 * kernel's key.
> -	 */
> -	if (!old_key) {
> -		key = make_kernel_key(key);
> -		if (!key) {
> -			rc = -ENOMEM;
> -			goto out;
> -		}
> -	}
> -
> -	/*
> -	 * We don't need to release key->sem here because
> -	 * make_kernel_key() will have upgraded the user key to kernel
> -	 * and handled the semaphore handoff.
> -	 */
> -	payload = key->payload.data[0];
> +	epayload = dereference_key_locked(key);
> +	new_data = epayload->decrypted_data;
> 
>  	if (old_key) {
>  		down_read(&old_key->sem);
> -		old_payload = old_key->payload.data[0];
> -		old_data = old_payload->data;
> +		old_epayload = dereference_key_locked(old_key);
> +		old_data = old_epayload->decrypted_data;
>  	}
> 
> -	new_data = payload->data;
> -
>  	rc = nvdimm->security_ops->change_key(nvdimm, old_data,
>  			new_data);
>  	if (rc)
>  		dev_warn(dev, "key update failed: %d\n", rc);
> 
> -	if (old_key) {
> +	if (old_key)
>  		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);
> -
> -	if (!old_key) {
> -		if (rc == 0) {
> -			dev_dbg(dev, "key cached: %#x\n", key_serial(key));
> -			nvdimm->key = key;
> -		} else
> -			key_destroy(key);
> -	}
>  	nvdimm_security_get_state(nvdimm);
> 
>   out:
> -	mutex_unlock(&nvdimm->key_mutex);
> +	mutex_unlock(&nvdimm->sec_mutex);
> +	key_put(old_key);
> +	key_put(key);
>  	return rc;
>  }
> 

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

Re: [PATCH 02/11] libnvdimm/security: change clear text nvdimm keys to encrypted keys

[Dan Williams]

[ add keyrings and lkml ]

On Sun, Nov 11, 2018 at 9:28 AM Mimi Zohar <zohar@linux.ibm.com> wrote:
>
> On Fri, 2018-11-09 at 15:13 -0700, Dave Jiang wrote:
> > In order to make nvdimm more secure, encrypted keys will be used instead of
> > clear text keys. A master key will be created to seal encrypted nvdimm
> > keys. The master key can be a trusted key generated from TPM 2.0 or a less
> > secure user key.
>
> Trusted keys also work for TPM 1.2.  Are you intentionally limiting
> the master key to TPM 2.0?

TPM 1.2 is supported from a software perspective, however the
intersection of hardware platforms deploying security enabled NVDIMMs
and TPM 1.2 might be a null set.

> Traditionally there is a single master key for the system, which would
> be sealed to a set of boot time PCR values.  After decrypting all of
> the encrypted keys, the master key would be removed from the keyring
> and a PCR extended.  Extending a PCR would prevent the master key from
> being unsealed again and used to decrypt encrypted keys, without
> rebooting the system.  Normally this would be done before pivoting
> root.
>
> If you're not referring to the system master key and are intentionally
> limiting usage to TPM 2.0, more details on the master key security
> requirements should be included.

Oh, interesting point. I think we had been assuming a local +
unsealed-at-runtime nvdimm master key rather than a system-wide master
key. Yes, we need to rethink this in terms of supporting a sealed
system-key. This would seem to limit security actions, outside of
unlock, to always requiring a reboot. I.e. the nominal case is that we
boot up and unlock the DIMMs, but any subsequent security operation
like erase, or change-passphrase would require rebooting into an
environment where the system-master key is unsealed. I do think
re-provisioning keys and erasing DIMM contents are sufficiently
exceptional events that a reboot requirement is tolerable.

Is there already existing tooling around this to be able to schedule
master-key related actions to be deferred to an initrd environment?

> Using trusted keys that are encrypted/decrypted using a user key
> should really be limited to testing environments.

Makes sense.

> >
> > In the process of this conversion, the kernel cached key will be removed
> > in order to simplify the verification process. The hardware will be used to
> > verify the decrypted user payload directly.
>
> Making this sort of change implies there is no concern in breaking
> existing userspace.  Either the code hasn't yet been upstreamed or
> there are not any users. If the code hasn't been upstreamed, it would
> make more sense to squash the git history:
>
> - making code review easier
> - making the git history bisect safe

Yes, the old scheme is not upstream. I'll do the squash once we've
finalized the key-management details.

Thanks for the help Mimi.
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

Re: [PATCH 02/11] libnvdimm/security: change clear text nvdimm keys to encrypted keys

[Mimi Zohar]

> > Traditionally there is a single master key for the system, which would
> > be sealed to a set of boot time PCR values.  After decrypting all of
> > the encrypted keys, the master key would be removed from the keyring
> > and a PCR extended.  Extending a PCR would prevent the master key from
> > being unsealed again and used to decrypt encrypted keys, without
> > rebooting the system.  Normally this would be done before pivoting
> > root.
> >
> > If you're not referring to the system master key and are intentionally
> > limiting usage to TPM 2.0, more details on the master key security
> > requirements should be included.
> 
> Oh, interesting point. I think we had been assuming a local +
> unsealed-at-runtime nvdimm master key rather than a system-wide master
> key. Yes, we need to rethink this in terms of supporting a sealed
> system-key. This would seem to limit security actions, outside of
> unlock, to always requiring a reboot. I.e. the nominal case is that we
> boot up and unlock the DIMMs, but any subsequent security operation
> like erase, or change-passphrase would require rebooting into an
> environment where the system-master key is unsealed. I do think
> re-provisioning keys and erasing DIMM contents are sufficiently
> exceptional events that a reboot requirement is tolerable.

> Is there already existing tooling around this to be able to schedule
> master-key related actions to be deferred to an initrd environment?

There's the original dracut support for loading a masterkey, which is
used by the EVM and ecryptfs dracut modules.  After the last usage,
the masterkey needs to be removed from the keyring.

Different people over the years have wanted to add support for
calculating the boot time expected PCRs values in order to reseal keys
(trusted key update), but I haven't looked to see if there are any
open source tools available.

Mimi

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

Re: [PATCH 02/11] libnvdimm/security: change clear text nvdimm keys to encrypted keys

[Dave Jiang]

On 11/11/2018 1:09 PM, Mimi Zohar wrote:
>>> Traditionally there is a single master key for the system, which would
>>> be sealed to a set of boot time PCR values.  After decrypting all of
>>> the encrypted keys, the master key would be removed from the keyring
>>> and a PCR extended.  Extending a PCR would prevent the master key from
>>> being unsealed again and used to decrypt encrypted keys, without
>>> rebooting the system.  Normally this would be done before pivoting
>>> root.
>>>
>>> If you're not referring to the system master key and are intentionally
>>> limiting usage to TPM 2.0, more details on the master key security
>>> requirements should be included.
>> Oh, interesting point. I think we had been assuming a local +
>> unsealed-at-runtime nvdimm master key rather than a system-wide master
>> key. Yes, we need to rethink this in terms of supporting a sealed
>> system-key. This would seem to limit security actions, outside of
>> unlock, to always requiring a reboot. I.e. the nominal case is that we
>> boot up and unlock the DIMMs, but any subsequent security operation
>> like erase, or change-passphrase would require rebooting into an
>> environment where the system-master key is unsealed. I do think
>> re-provisioning keys and erasing DIMM contents are sufficiently
>> exceptional events that a reboot requirement is tolerable.
>> Is there already existing tooling around this to be able to schedule
>> master-key related actions to be deferred to an initrd environment?
> There's the original dracut support for loading a masterkey, which is
> used by the EVM and ecryptfs dracut modules.  After the last usage,
> the masterkey needs to be removed from the keyring.

How does one generate new encrypted keys with the system masterkey 
removed from the keyring?


>
> Different people over the years have wanted to add support for
> calculating the boot time expected PCRs values in order to reseal keys
> (trusted key update), but I haven't looked to see if there are any
> open source tools available.
>
> Mimi
>
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

Re: [PATCH 02/11] libnvdimm/security: change clear text nvdimm keys to encrypted keys

[Dave Jiang]

On 11/11/2018 10:27 AM, Mimi Zohar wrote:
> On Fri, 2018-11-09 at 15:13 -0700, Dave Jiang wrote:
>> In order to make nvdimm more secure, encrypted keys will be used instead of
>> clear text keys. A master key will be created to seal encrypted nvdimm
>> keys. The master key can be a trusted key generated from TPM 2.0 or a less
>> secure user key.
> Trusted keys also work for TPM 1.2.  Are you intentionally limiting
> the master key to TPM 2.0?
>
> Traditionally there is a single master key for the system, which would
> be sealed to a set of boot time PCR values.  After decrypting all of
> the encrypted keys, the master key would be removed from the keyring
> and a PCR extended.  Extending a PCR would prevent the master key from
> being unsealed again and used to decrypt encrypted keys, without
> rebooting the system.  Normally this would be done before pivoting
> root.
>
> If you're not referring to the system master key and are intentionally
> limiting usage to TPM 2.0, more details on the master key security
> requirements should be included.
>
> Using trusted keys that are encrypted/decrypted using a user key
> should really be limited to testing environments.

Do you have any recommendation for systems that do not support TPM?



>
>> In the process of this conversion, the kernel cached key will be removed
>> in order to simplify the verification process. The hardware will be used to
>> verify the decrypted user payload directly.
> Making this sort of change implies there is no concern in breaking
> existing userspace.  Either the code hasn't yet been upstreamed or
> there are not any users. If the code hasn't been upstreamed, it would
> make more sense to squash the git history:
>
> - making code review easier
> - making the git history bisect safe
>
> Mimi
>
>> Signed-off-by: Dave Jiang <dave.jiang@intel.com>
>> ---
>>   Documentation/nvdimm/security.txt |   29 ++-
>>   drivers/nvdimm/dimm.c             |    3
>>   drivers/nvdimm/dimm_devs.c        |    2
>>   drivers/nvdimm/nd-core.h          |    3
>>   drivers/nvdimm/nd.h               |    5 -
>>   drivers/nvdimm/security.c         |  316 ++++++++++---------------------------
>>   6 files changed, 108 insertions(+), 250 deletions(-)
>>
>> diff --git a/Documentation/nvdimm/security.txt b/Documentation/nvdimm/security.txt
>> index 50cbb6cb96a1..11240ce48755 100644
>> --- a/Documentation/nvdimm/security.txt
>> +++ b/Documentation/nvdimm/security.txt
>> @@ -39,34 +39,39 @@ The DIMM id would be provided along with the key payload (passphrase) to
>>   the kernel.
>>
>>   The security keys are managed on the basis of a single key per DIMM. The
>> -key "passphrase" is expected to be 32bytes long or padded to 32bytes. This is
>> +key "passphrase" is expected to be 32bytes long. This is
>>   similar to the ATA security specification [2]. A key is initially acquired
>>   via the request_key() kernel API call and retrieved from userspace. It is up to
>>   the user to provide an upcall application to retrieve the key in whatever
>>   fashion meets their security requirements.
>>
>> -A nvdimm user logon key has the description format of:
>> +A nvdimm encrypted key has the description format of:
>>   nvdimm:<bus-provider-specific-unique-id>
>>
>> +See Documentation/security/keys/trusted-encrypted.rst for details on encrypted
>> +keys. The encrypted key for the DIMM is generated from a master key that is
>> +either a trusted key created via TPM2 or a less secure user key. The payload
>> +for the nvdimm encrypted key is randomly generated by the kernel and is not
>> +visible to the user. The user is responsible for retaining the encrypted key
>> +blob generated from the encrypted key to be loaded at the next boot session.
>> +
>>   4. Unlocking
>>   ------------
>>   When the DIMMs are being enumerated by the kernel, the kernel will attempt to
>> -retrieve the key from its keyring. If that fails, it will attempt to
>>   acquire the key from the userspace upcall function. This is the only time
>>   a locked DIMM can be unlocked. Once unlocked, the DIMM will remain unlocked
>>   until reboot.
>>
>>   5. Update
>>   ---------
>> -When doing an update, it is expected that the new key with the 64bit payload of
>> -format described above is added via the keyutils API or utility. The update
>> -command written to the sysfs attribute will be with the format:
>> +When doing an update, it is expected that the new key is added via the
>> +keyutils API or utility. The update command written to the sysfs attribute
>> +will be with the format:
>>   update <old keyid> <new keyid>
>>
>> -It is expected that a user logon key has been injected via keyutils to provide
>> -the payload for the update operation. The kernel will take the new user key,
>> -attempt the update operation with the nvdimm, and replace the existing key's
>> -payload with the new passphrase.
>> +It is expected that a encrypted key has been injected via keyutils to provide
>> +the payload for the update operation. The kernel will take the new user key
>> +and the old user key to attempt the update operation.
>>
>>   If there is no old key id due to a security enabling, then a 0 should be
>>   passed in.  If a nvdimm has an existing passphrase, then an "old" key should
>> @@ -80,7 +85,7 @@ frozen by a user with root privelege.
>>   7. Disable
>>   ----------
>>   The security disable command format is:
>> -disable <old keyid>
>> +disable <current keyid>
>>
>>   An "old" key with the passphrase payload that is tied to the nvdimm should be
>>   injected with a key description that does not have the "nvdimm:" prefix and
>> @@ -89,7 +94,7 @@ its keyid should be passed in via sysfs.
>>   8. Secure Erase
>>   ---------------
>>   The command format for doing a secure erase is:
>> -erase <old keyid>
>> +erase <current keyid>
>>
>>   An "old" key with the passphrase payload that is tied to the nvdimm should be
>>   injected with a key description that does not have the "nvdimm:" prefix and
>> diff --git a/drivers/nvdimm/dimm.c b/drivers/nvdimm/dimm.c
>> index 225a8474c44a..c3886a22b5e4 100644
>> --- a/drivers/nvdimm/dimm.c
>> +++ b/drivers/nvdimm/dimm.c
>> @@ -112,9 +112,6 @@ static int nvdimm_probe(struct device *dev)
>>   static int nvdimm_remove(struct device *dev)
>>   {
>>   	struct nvdimm_drvdata *ndd = dev_get_drvdata(dev);
>> -	struct nvdimm *nvdimm = to_nvdimm(dev);
>> -
>> -	nvdimm_security_release(nvdimm);
>>
>>   	if (!ndd)
>>   		return 0;
>> diff --git a/drivers/nvdimm/dimm_devs.c b/drivers/nvdimm/dimm_devs.c
>> index 5d34251c4f3b..269f401a5e68 100644
>> --- a/drivers/nvdimm/dimm_devs.c
>> +++ b/drivers/nvdimm/dimm_devs.c
>> @@ -485,7 +485,7 @@ struct nvdimm *__nvdimm_create(struct nvdimm_bus *nvdimm_bus, void *provider_dat
>>   	nvdimm->num_flush = num_flush;
>>   	nvdimm->flush_wpq = flush_wpq;
>>   	atomic_set(&nvdimm->busy, 0);
>> -	mutex_init(&nvdimm->key_mutex);
>> +	mutex_init(&nvdimm->sec_mutex);
>>   	dev = &nvdimm->dev;
>>   	dev_set_name(dev, "nmem%d", nvdimm->id);
>>   	dev->parent = &nvdimm_bus->dev;
>> diff --git a/drivers/nvdimm/nd-core.h b/drivers/nvdimm/nd-core.h
>> index 6e2877996a9b..cfb87f71db8d 100644
>> --- a/drivers/nvdimm/nd-core.h
>> +++ b/drivers/nvdimm/nd-core.h
>> @@ -45,8 +45,7 @@ struct nvdimm {
>>   	const char *dimm_id;
>>   	const struct nvdimm_security_ops *security_ops;
>>   	enum nvdimm_security_state state;
>> -	struct key *key;
>> -	struct mutex key_mutex;
>> +	struct mutex sec_mutex;
>>   };
>>
>>   /**
>> diff --git a/drivers/nvdimm/nd.h b/drivers/nvdimm/nd.h
>> index 892f127bfd38..f8d8f0a2a40d 100644
>> --- a/drivers/nvdimm/nd.h
>> +++ b/drivers/nvdimm/nd.h
>> @@ -428,7 +428,6 @@ bool pmem_should_map_pages(struct device *dev);
>>
>>   #ifdef CONFIG_NVDIMM_SECURITY
>>   int nvdimm_security_unlock_dimm(struct nvdimm *nvdimm);
>> -void nvdimm_security_release(struct nvdimm *nvdimm);
>>   int nvdimm_security_get_state(struct nvdimm *nvdimm);
>>   int nvdimm_security_change_key(struct nvdimm *nvdimm, unsigned int old_keyid,
>>   		unsigned int new_keyid);
>> @@ -441,10 +440,6 @@ static inline int nvdimm_security_unlock_dimm(struct nvdimm *nvdimm)
>>   	return 0;
>>   }
>>
>> -static inline void nvdimm_security_release(struct nvdimm *nvdimm)
>> -{
>> -}
>> -
>>   static inline int nvdimm_security_get_state(struct nvdimm *nvdimm)
>>   {
>>   	return -EOPNOTSUPP;
>> diff --git a/drivers/nvdimm/security.c b/drivers/nvdimm/security.c
>> index 5aacc590b4c0..ee741199d623 100644
>> --- a/drivers/nvdimm/security.c
>> +++ b/drivers/nvdimm/security.c
>> @@ -10,43 +10,10 @@
>>   #include <linux/key.h>
>>   #include <linux/key-type.h>
>>   #include <keys/user-type.h>
>> +#include <keys/encrypted-type.h>
>>   #include "nd-core.h"
>>   #include "nd.h"
>>
>> -/*
>> - * Replacing the user key with a kernel key. The function expects that
>> - * we hold the sem for the key passed in. The function will release that
>> - * sem when done process. We will also hold the sem for the valid new key
>> - * returned.
>> - */
>> -static struct key *make_kernel_key(struct key *key)
>> -{
>> -	struct key *new_key;
>> -	struct user_key_payload *payload;
>> -	int rc;
>> -
>> -	new_key = key_alloc(&key_type_logon, key->description,
>> -			GLOBAL_ROOT_UID, GLOBAL_ROOT_GID, current_cred(),
>> -			KEY_POS_ALL & ~KEY_POS_SETATTR,
>> -			KEY_ALLOC_NOT_IN_QUOTA, NULL);
>> -	if (IS_ERR(new_key))
>> -		return NULL;
>> -
>> -	payload = key->payload.data[0];
>> -	rc = key_instantiate_and_link(new_key, payload->data,
>> -			payload->datalen, NULL, NULL);
>> -	up_read(&key->sem);
>> -	if (rc < 0) {
>> -		key_put(new_key);
>> -		return NULL;
>> -	}
>> -
>> -	key_put(key);
>> -
>> -	down_read(&new_key->sem);
>> -	return new_key;
>> -}
>> -
>>   /*
>>    * Retrieve user injected key
>>    */
>> @@ -61,6 +28,10 @@ static struct key *nvdimm_lookup_user_key(struct device *dev,
>>   		return NULL;
>>
>>   	key = key_ref_to_ptr(keyref);
>> +	if (key->type != &key_type_encrypted) {
>> +		key_put(key);
>> +		return NULL;
>> +	}
>>   	dev_dbg(dev, "%s: key found: %#x\n", __func__, key_serial(key));
>>
>>   	return key;
>> @@ -76,7 +47,7 @@ static struct key *nvdimm_request_key(struct nvdimm *nvdimm)
>>   	struct device *dev = &nvdimm->dev;
>>
>>   	sprintf(desc, "%s%s", NVDIMM_PREFIX, nvdimm->dimm_id);
>> -	key = request_key(&key_type_logon, desc, "");
>> +	key = request_key(&key_type_encrypted, desc, "");
>>   	if (IS_ERR(key)) {
>>   		if (PTR_ERR(key) == -ENOKEY)
>>   			dev_warn(dev, "request_key() found no key\n");
>> @@ -88,52 +59,6 @@ static struct key *nvdimm_request_key(struct nvdimm *nvdimm)
>>   	return key;
>>   }
>>
>> -struct key *nvdimm_get_and_verify_key(struct nvdimm *nvdimm,
>> -		unsigned int user_key_id)
>> -{
>> -	int rc;
>> -	struct key *user_key, *key;
>> -	struct device *dev = &nvdimm->dev;
>> -	struct user_key_payload *upayload, *payload;
>> -
>> -	lockdep_assert_held(&nvdimm->key_mutex);
>> -	key = nvdimm->key;
>> -	if (!key) {
>> -		dev_dbg(dev, "No cached kernel key\n");
>> -		return ERR_PTR(-EAGAIN);;
>> -	}
>> -	dev_dbg(dev, "cached_key: %#x\n", key_serial(key));
>> -
>> -	user_key = nvdimm_lookup_user_key(dev, user_key_id);
>> -	if (!user_key) {
>> -		dev_dbg(dev, "Old user key lookup failed\n");
>> -		return ERR_PTR(-EINVAL);
>> -	}
>> -	dev_dbg(dev, "user_key: %#x\n", key_serial(user_key));
>> -
>> -	down_read(&key->sem);
>> -	down_read(&user_key->sem);
>> -	payload = key->payload.data[0];
>> -	upayload = user_key->payload.data[0];
>> -
>> -	rc = memcmp(payload->data, upayload->data, NVDIMM_PASSPHRASE_LEN);
>> -	up_read(&user_key->sem);
>> -	key_put(user_key);
>> -	up_read(&key->sem);
>> -
>> -	if (rc != 0) {
>> -		dev_warn(dev, "Supplied old user key fails check.\n");
>> -		return ERR_PTR(-EINVAL);
>> -	}
>> -	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)
>> @@ -146,14 +71,15 @@ int nvdimm_security_erase(struct nvdimm *nvdimm, unsigned int keyid)
>>   {
>>   	int rc = 0;
>>   	struct key *key;
>> -	struct user_key_payload *payload;
>> +	struct encrypted_key_payload *epayload;
>>   	struct device *dev = &nvdimm->dev;
>> +	void *data;
>>
>>   	if (!nvdimm->security_ops)
>>   		return -EOPNOTSUPP;
>>
>>   	nvdimm_bus_lock(dev);
>> -	mutex_lock(&nvdimm->key_mutex);
>> +	mutex_lock(&nvdimm->sec_mutex);
>>   	if (atomic_read(&nvdimm->busy)) {
>>   		dev_warn(dev, "Unable to secure erase while DIMM active.\n");
>>   		rc = -EBUSY;
>> @@ -166,26 +92,23 @@ int nvdimm_security_erase(struct nvdimm *nvdimm, unsigned int keyid)
>>   		goto out;
>>   	}
>>
>> -	/* look for a key from cached key if exists */
>> -	key = nvdimm_get_and_verify_key(nvdimm, keyid);
>> -	if (IS_ERR(key)) {
>> +	key = nvdimm_lookup_user_key(dev, keyid);
>> +	if (!key) {
>>   		dev_dbg(dev, "Unable to get and verify key\n");
>> -		rc = PTR_ERR(key);
>> +		rc = -ENOKEY;
>>   		goto out;
>>   	}
>>
>>   	down_read(&key->sem);
>> -	payload = key->payload.data[0];
>> +	epayload = dereference_key_locked(key);
>> +	data = epayload->decrypted_data;
>>   	rc = nvdimm->security_ops->erase(nvdimm,
>> -			(const struct nvdimm_key_data *)payload->data);
>> +			(const struct nvdimm_key_data *)data);
>>   	up_read(&key->sem);
>> -
>> -	/* remove key since secure erase kills the passphrase */
>> -	key_destroy(key);
>> -	nvdimm->key = NULL;
>> +	key_put(key);
>>
>>    out:
>> -	mutex_unlock(&nvdimm->key_mutex);
>> +	mutex_unlock(&nvdimm->sec_mutex);
>>   	nvdimm_bus_unlock(dev);
>>   	nvdimm_security_get_state(nvdimm);
>>   	return rc;
>> @@ -201,11 +124,15 @@ int nvdimm_security_freeze_lock(struct nvdimm *nvdimm)
>>   	if (nvdimm->state == NVDIMM_SECURITY_UNSUPPORTED)
>>   		return -EOPNOTSUPP;
>>
>> +	mutex_lock(&nvdimm->sec_mutex);
>>   	rc = nvdimm->security_ops->freeze_lock(nvdimm);
>> -	if (rc < 0)
>> +	if (rc < 0) {
>> +		mutex_unlock(&nvdimm->sec_mutex);
>>   		return rc;
>> +	}
>>
>>   	nvdimm_security_get_state(nvdimm);
>> +	mutex_unlock(&nvdimm->sec_mutex);
>>   	return 0;
>>   }
>>
>> @@ -213,8 +140,9 @@ int nvdimm_security_disable(struct nvdimm *nvdimm, unsigned int keyid)
>>   {
>>   	int rc;
>>   	struct key *key;
>> -	struct user_key_payload *payload;
>> +	struct encrypted_key_payload *epayload;
>>   	struct device *dev = &nvdimm->dev;
>> +	void *data;
>>
>>   	if (!nvdimm->security_ops)
>>   		return -EOPNOTSUPP;
>> @@ -222,57 +150,52 @@ int nvdimm_security_disable(struct nvdimm *nvdimm, unsigned int keyid)
>>   	if (nvdimm->state == NVDIMM_SECURITY_UNSUPPORTED)
>>   		return -EOPNOTSUPP;
>>
>> -	mutex_lock(&nvdimm->key_mutex);
>> +	mutex_lock(&nvdimm->sec_mutex);
>>   	/* look for a key from cached key */
>> -	key = nvdimm_get_and_verify_key(nvdimm, keyid);
>> -	if (IS_ERR(key)) {
>> -		mutex_unlock(&nvdimm->key_mutex);
>> -		return PTR_ERR(key);
>> +	key = nvdimm_lookup_user_key(dev, keyid);
>> +	if (!key) {
>> +		mutex_unlock(&nvdimm->sec_mutex);
>> +		return -ENOKEY;
>>   	}
>>
>>   	down_read(&key->sem);
>> -	payload = key->payload.data[0];
>> +	epayload = dereference_key_locked(key);
>> +	data = epayload->decrypted_data;
>>
>>   	rc = nvdimm->security_ops->disable(nvdimm,
>> -			(const struct nvdimm_key_data *)payload->data);
>> +			(const struct nvdimm_key_data *)data);
>>   	up_read(&key->sem);
>> -	if (rc < 0) {
>> +	if (rc < 0)
>>   		dev_warn(dev, "security disable failed\n");
>> -		goto out;
>> -	}
>>
>> -	/* If we succeed then remove the key */
>> -	key_destroy(key);
>> -	nvdimm->key = NULL;
>> -
>> - out:
>> -	mutex_unlock(&nvdimm->key_mutex);
>> +	key_put(key);
>>   	nvdimm_security_get_state(nvdimm);
>> +	mutex_unlock(&nvdimm->sec_mutex);
>>   	return rc;
>>   }
>>
>> -static int nvdimm_self_verify_key(struct nvdimm *nvdimm)
>> +static struct key *nvdimm_self_verify_key(struct nvdimm *nvdimm)
>>   {
>>   	struct key *key;
>> -	struct user_key_payload *payload;
>> +	struct encrypted_key_payload *epayload;
>>   	void *data;
>>   	int rc;
>>
>> -	lockdep_assert_held(&nvdimm->key_mutex);
>> -
>> +	lockdep_assert_held(&nvdimm->sec_mutex);
>>   	key = nvdimm_request_key(nvdimm);
>>   	if (!key)
>> -		return -ENOKEY;
>> +		return NULL;
>> +
>> +	down_read(&key->sem);
>> +	epayload = dereference_key_locked(key);
>> +	data = epayload->decrypted_data;
>>
>> -	if (key->datalen != NVDIMM_PASSPHRASE_LEN) {
>> +	if (epayload->decrypted_datalen != NVDIMM_PASSPHRASE_LEN) {
>>   		key_put(key);
>> -		return -EINVAL;
>> +		key = NULL;
>> +		goto out;
>>   	}
>>
>> -	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.
>> @@ -280,19 +203,21 @@ static int nvdimm_self_verify_key(struct nvdimm *nvdimm)
>>   	rc = nvdimm->security_ops->change_key(nvdimm, data, data);
>>   	if (rc < 0) {
>>   		key_put(key);
>> -		return rc;
>> +		key = NULL;
>>   	}
>> +
>> +out:
>>   	up_read(&key->sem);
>> -	nvdimm->key = key;
>> -	return 0;
>> +	return key;
>>   }
>>
>>   int nvdimm_security_unlock_dimm(struct nvdimm *nvdimm)
>>   {
>> -	struct key *key;
>> -	int rc = -ENXIO;
>> -	struct user_key_payload *payload;
>> +	struct key *key = NULL;
>> +	int rc;
>> +	struct encrypted_key_payload *epayload;
>>   	struct device *dev = &nvdimm->dev;
>> +	void *data;
>>
>>   	if (!nvdimm->security_ops)
>>   		return 0;
>> @@ -301,7 +226,7 @@ int nvdimm_security_unlock_dimm(struct nvdimm *nvdimm)
>>   			nvdimm->state == NVDIMM_SECURITY_DISABLED)
>>   		return 0;
>>
>> -	mutex_lock(&nvdimm->key_mutex);
>> +	mutex_lock(&nvdimm->sec_mutex);
>>   	/*
>>   	 * If the pre-OS has unlocked the DIMM, we will attempt to send
>>   	 * the key from request_key() to the hardware for verification.
>> @@ -310,60 +235,50 @@ int nvdimm_security_unlock_dimm(struct nvdimm *nvdimm)
>>   	 * other security operations.
>>   	 */
>>   	if (nvdimm->state == NVDIMM_SECURITY_UNLOCKED) {
>> -		rc = nvdimm_self_verify_key(nvdimm);
>> -		if (rc < 0) {
>> +		key = nvdimm_self_verify_key(nvdimm);
>> +		if (!key) {
>>   			rc = nvdimm_security_freeze_lock(nvdimm);
>> -			mutex_unlock(&nvdimm->key_mutex);
>> -			return rc;
>> +			mutex_unlock(&nvdimm->sec_mutex);
>> +			return -ENOKEY;
>>   		}
>>   	}
>>
>> -	key = nvdimm->key;
>> -	if (!key) {
>> +	if (!key)
>>   		key = nvdimm_request_key(nvdimm);
>> -		if (key && key->datalen != NVDIMM_PASSPHRASE_LEN) {
>> -			key_put(key);
>> -			key = NULL;
>> -			rc = -EINVAL;
>> -		}
>> -	}
>> +
>>   	if (!key) {
>> -		mutex_unlock(&nvdimm->key_mutex);
>> -		return rc;
>> +		mutex_unlock(&nvdimm->sec_mutex);
>> +		return -ENOKEY;
>>   	}
>>
>>   	dev_dbg(dev, "%s: key: %#x\n", __func__, key_serial(key));
>>   	down_read(&key->sem);
>> -	payload = key->payload.data[0];
>> +	epayload = dereference_key_locked(key);
>> +	data = epayload->decrypted_data;
>> +
>> +	if (epayload->decrypted_datalen != NVDIMM_PASSPHRASE_LEN) {
>> +		up_read(&key->sem);
>> +		key_put(key);
>> +		mutex_unlock(&nvdimm->sec_mutex);
>> +		return -EINVAL;
>> +	}
>> +
>>   	rc = nvdimm->security_ops->unlock(nvdimm,
>> -			(const struct nvdimm_key_data *)payload->data);
>> +			(const struct nvdimm_key_data *)data);
>>   	up_read(&key->sem);
>>
>>   	if (rc == 0) {
>> -		if (!nvdimm->key)
>> -			nvdimm->key = key;
>>   		nvdimm->state = NVDIMM_SECURITY_UNLOCKED;
>>   		dev_dbg(dev, "DIMM %s unlocked\n", dev_name(dev));
>> -	} else {
>> -		key_invalidate(key);
>> -		key_put(key);
>> -		nvdimm->key = NULL;
>> +	} else
>>   		dev_warn(dev, "Failed to unlock dimm: %s\n", dev_name(dev));
>> -	}
>>
>> -	mutex_unlock(&nvdimm->key_mutex);
>> +	key_put(key);
>>   	nvdimm_security_get_state(nvdimm);
>> +	mutex_unlock(&nvdimm->sec_mutex);
>>   	return rc;
>>   }
>>
>> -void nvdimm_security_release(struct nvdimm *nvdimm)
>> -{
>> -	mutex_lock(&nvdimm->key_mutex);
>> -	key_put(nvdimm->key);
>> -	nvdimm->key = NULL;
>> -	mutex_unlock(&nvdimm->key_mutex);
>> -}
>> -
>>   int nvdimm_security_change_key(struct nvdimm *nvdimm,
>>   		unsigned int old_keyid, unsigned int new_keyid)
>>   {
>> @@ -371,7 +286,7 @@ int nvdimm_security_change_key(struct nvdimm *nvdimm,
>>   	struct key *key, *old_key;
>>   	void *old_data = NULL, *new_data;
>>   	struct device *dev = &nvdimm->dev;
>> -	struct user_key_payload *payload, *old_payload;
>> +	struct encrypted_key_payload *epayload, *old_epayload;
>>
>>   	if (!nvdimm->security_ops)
>>   		return -EOPNOTSUPP;
>> @@ -379,15 +294,10 @@ int nvdimm_security_change_key(struct nvdimm *nvdimm,
>>   	if (nvdimm->state == NVDIMM_SECURITY_FROZEN)
>>   		return -EBUSY;
>>
>> -	mutex_lock(&nvdimm->key_mutex);
>> +	mutex_lock(&nvdimm->sec_mutex);
>>   	/* look for a key from cached key if exists */
>> -	old_key = nvdimm_get_and_verify_key(nvdimm, old_keyid);
>> -	if (IS_ERR(old_key) && PTR_ERR(old_key) == -EAGAIN)
>> -		old_key = NULL;
>> -	else if (IS_ERR(old_key)) {
>> -		mutex_unlock(&nvdimm->key_mutex);
>> -		return PTR_ERR(old_key);
>> -	} else
>> +	old_key = nvdimm_lookup_user_key(dev, old_keyid);
>> +	if (old_key)
>>   		dev_dbg(dev, "%s: old key: %#x\n", __func__,
>>   				key_serial(old_key));
>>
>> @@ -402,76 +312,28 @@ int nvdimm_security_change_key(struct nvdimm *nvdimm,
>>   	dev_dbg(dev, "%s: new key: %#x\n", __func__, key_serial(key));
>>
>>   	down_read(&key->sem);
>> -	payload = key->payload.data[0];
>> -	if (payload->datalen != NVDIMM_PASSPHRASE_LEN) {
>> -		rc = -EINVAL;
>> -		up_read(&key->sem);
>> -		goto out;
>> -	}
>> -
>> -	/*
>> -	 * Since there is no existing key this user key will become the
>> -	 * kernel's key.
>> -	 */
>> -	if (!old_key) {
>> -		key = make_kernel_key(key);
>> -		if (!key) {
>> -			rc = -ENOMEM;
>> -			goto out;
>> -		}
>> -	}
>> -
>> -	/*
>> -	 * We don't need to release key->sem here because
>> -	 * make_kernel_key() will have upgraded the user key to kernel
>> -	 * and handled the semaphore handoff.
>> -	 */
>> -	payload = key->payload.data[0];
>> +	epayload = dereference_key_locked(key);
>> +	new_data = epayload->decrypted_data;
>>
>>   	if (old_key) {
>>   		down_read(&old_key->sem);
>> -		old_payload = old_key->payload.data[0];
>> -		old_data = old_payload->data;
>> +		old_epayload = dereference_key_locked(old_key);
>> +		old_data = old_epayload->decrypted_data;
>>   	}
>>
>> -	new_data = payload->data;
>> -
>>   	rc = nvdimm->security_ops->change_key(nvdimm, old_data,
>>   			new_data);
>>   	if (rc)
>>   		dev_warn(dev, "key update failed: %d\n", rc);
>>
>> -	if (old_key) {
>> +	if (old_key)
>>   		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);
>> -
>> -	if (!old_key) {
>> -		if (rc == 0) {
>> -			dev_dbg(dev, "key cached: %#x\n", key_serial(key));
>> -			nvdimm->key = key;
>> -		} else
>> -			key_destroy(key);
>> -	}
>>   	nvdimm_security_get_state(nvdimm);
>>
>>    out:
>> -	mutex_unlock(&nvdimm->key_mutex);
>> +	mutex_unlock(&nvdimm->sec_mutex);
>> +	key_put(old_key);
>> +	key_put(key);
>>   	return rc;
>>   }
>>
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

Re: [PATCH 02/11] libnvdimm/security: change clear text nvdimm keys to encrypted keys

[Mimi Zohar]

On Mon, 2018-11-12 at 08:42 -0700, Dave Jiang wrote:

> How does one generate new encrypted keys with the system masterkey 
> removed from the keyring?

I don't think you can.

Mimi

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

Re: [PATCH 02/11] libnvdimm/security: change clear text nvdimm keys to encrypted keys

[Mimi Zohar]

On Mon, 2018-11-12 at 08:45 -0700, Dave Jiang wrote:

> > Using trusted keys that are encrypted/decrypted using a user key
> > should really be limited to testing environments.
> 
> Do you have any recommendation for systems that do not support TPM?

The TPM provides certain security guarantees, which "user" type keys
do not.  If the system doesn't provide either a discrete or firmware
TPM, use a software TPM.

Mimi
" 

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

Re: [PATCH 02/11] libnvdimm/security: change clear text nvdimm keys to encrypted keys

[Dave Jiang]

On 11/12/18 11:49 AM, Mimi Zohar wrote:
> On Mon, 2018-11-12 at 08:42 -0700, Dave Jiang wrote:
> 
>> How does one generate new encrypted keys with the system masterkey 
>> removed from the keyring?
> 
> I don't think you can.

Are there any documentations available showing system master-key usages?
So far the docs (i.e. EVM setup) I've seen generates a trusted master
key and then create the encrypted keys from that. I'm missing the
understanding of how to generate encrypted keys from the system master
key initially during setup. Thanks!
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

Re: [PATCH 07/11] libnvdimm/security: add overwrite status notification

[Dave Jiang]

On 11/9/18 7:59 PM, Elliott, Robert (Persistent Memory) wrote:
>> -----Original Message-----
>> From: Linux-nvdimm <linux-nvdimm-bounces@lists.01.org> On Behalf Of
>> Dave Jiang
>> Sent: Friday, November 09, 2018 4:14 PM
>> Subject: [PATCH 07/11] libnvdimm/security: add overwrite status
>> notification
>>
> ...
>> @@ -2033,6 +2033,11 @@ static int acpi_nfit_register_dimms(struct
>> acpi_nfit_desc *acpi_desc)
>>  if (!nvdimm)
>>  continue;
>>
>> +rc = nvdimm_setup_security_events(nvdimm);
>> +if (rc < 0)
>> +dev_warn(acpi_desc->dev,
>> +"no security event setup
>> failed\n");
> 
> That seems like a double negative.
> 
> Printing the rc value (or better yet, the string for it)
> is always helpful too.

Thanks Robert. Will fix.

> 
> ---
> Robert Elliott, HPE Persistent Memory
> 
> 
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

Re: [PATCH 10/11] tools/testing/nvdimm: add Intel DSM 1.8 support for nfit_test

[Dave Jiang]

On 11/9/18 8:15 PM, Elliott, Robert (Persistent Memory) wrote:
> 
> 
>> -----Original Message-----
>> From: Linux-nvdimm <linux-nvdimm-bounces@lists.01.org> On Behalf Of
>> Dave Jiang
>> Sent: Friday, November 09, 2018 4:15 PM
>> To: dan.j.williams@intel.com; zohar@linux.vnet.ibm.com
>> Cc: linux-nvdimm@lists.01.org
>> Subject: [PATCH 10/11] tools/testing/nvdimm: add Intel DSM 1.8
>> support for nfit_test
> ...
>> +static int nd_intel_test_cmd_master_set_pass(struct nfit_test *t,
>> +struct nd_intel_set_master_passphrase *nd_cmd,
>> +unsigned int buf_len, int dimm)
>> +{
>> +struct device *dev = &t->pdev.dev;
>> +struct nfit_test_sec *sec = &dimm_sec_info[dimm];
>> +
>> +if (!(sec->ext_state & ND_INTEL_SEC_ESTATE_ENABLED)) {
>> +nd_cmd->status = ND_INTEL_STATUS_NOT_SUPPORTED;
>> +dev_dbg(dev, "master set passphrase in wrong state\n");
> 
> "master set passphrase:" for consistency

Thanks! Will update.

> 
>> +} else if (sec->ext_state & ND_INTEL_SEC_ESTATE_PLIMIT) {
>> +nd_cmd->status = ND_INTEL_STATUS_INVALID_STATE;
>> +dev_dbg(dev, "master set passphrase in wrong security
>> state\n");
> 
> "master set passphrase:" for consistency
> 
>> +} else if (memcmp(nd_cmd->old_pass, sec->master_passphrase,
>> +ND_INTEL_PASSPHRASE_SIZE) != 0) {
>> +nd_cmd->status = ND_INTEL_STATUS_INVALID_PASS;
>> +dev_dbg(dev, "master set passphrase wrong
>> passphrase\n");
> 
> "master set passphrase:" for consistency
> 
>> +} else {
>> +memcpy(sec->master_passphrase, nd_cmd->new_pass,
>> +ND_INTEL_PASSPHRASE_SIZE);
>> +nd_cmd->status = 0;
>> +dev_dbg(dev, "master passphrase updated\n");
> 
> "master set passphrase:" for consistency
> 
>> +}
>> +
>> +return 0;
>> +}
>> +
>> +static int nd_intel_test_cmd_master_secure_erase(struct nfit_test
>> *t,
>> +struct nd_intel_master_secure_erase *nd_cmd,
>> +unsigned int buf_len, int dimm)
>> +{
>> +struct device *dev = &t->pdev.dev;
>> +struct nfit_test_sec *sec = &dimm_sec_info[dimm];
>> +
>> +if (!(sec->ext_state & ND_INTEL_SEC_ESTATE_ENABLED)) {
>> +nd_cmd->status = ND_INTEL_STATUS_NOT_SUPPORTED;
>> +dev_dbg(dev, "master erase in wrong state\n");
> 
> "master secure erase: " for consistency
> 
>> +} else if (sec->ext_state & ND_INTEL_SEC_ESTATE_PLIMIT) {
>> +nd_cmd->status = ND_INTEL_STATUS_INVALID_STATE;
>> +dev_dbg(dev, "master erase in wrong security state\n");
> 
> "master secure erase: " for consistency
> 
>> +} else if (memcmp(nd_cmd->passphrase, sec->master_passphrase,
>> +ND_INTEL_PASSPHRASE_SIZE) != 0) {
>> +nd_cmd->status = ND_INTEL_STATUS_INVALID_PASS;
>> +dev_dbg(dev, "master secure erase: wrong passphrase\n");
>> +} else {
>> +memset(sec->master_passphrase, 0,
>> ND_INTEL_PASSPHRASE_SIZE);
>> +sec->ext_state = ND_INTEL_SEC_ESTATE_ENABLED;
>> +memset(sec->passphrase, 0, ND_INTEL_PASSPHRASE_SIZE);
>> +sec->state = 0;
>> +dev_dbg(dev, "master secure erase: done\n");
>> +}
>> +
>> +return 0;
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

Re: [PATCH 09/11] acpi/nfit, libnvdimm/security: add Intel DSM 1.8 master passphrase support

[Dan Williams]

On Fri, Nov 9, 2018 at 2:14 PM Dave Jiang <dave.jiang@intel.com> wrote:
>
> With Intel DSM 1.8 [1] two new security DSMs are introduced. Enable/update
> master passphrase and master secure erase. The master passphrase allows
> a secure erase to be performed without the user passphrase that is set on
> the NVDIMM. The commands of master_update and master_erase are added to
> the sysfs knob in order to initiate the DSMs. They are similar in opeartion
> mechanism compare to update and erase.
>
> [1]: http://pmem.io/documents/NVDIMM_DSM_Interface-V1.8.pdf
>
> Signed-off-by: Dave Jiang <dave.jiang@intel.com>
> ---
>  Documentation/nvdimm/security.txt |   23 ++++++
>  drivers/acpi/nfit/core.c          |    2 +
>  drivers/acpi/nfit/intel.c         |  132 ++++++++++++++++++++++++++++++++++++-
>  drivers/acpi/nfit/intel.h         |   18 +++++
>  drivers/acpi/nfit/nfit.h          |    6 +-
>  drivers/nvdimm/dimm_devs.c        |   16 ++++
>  drivers/nvdimm/nd-core.h          |    1
>  drivers/nvdimm/nd.h               |    5 +
>  drivers/nvdimm/security.c         |   34 ++++++++--
>  include/linux/libnvdimm.h         |   15 ++++
>  10 files changed, 236 insertions(+), 16 deletions(-)
>
[..]
>  static int intel_dimm_security_query_overwrite(struct nvdimm *nvdimm)
>  {
>         int cmd_rc, rc = 0;
> @@ -397,7 +514,8 @@ static int intel_dimm_security_unlock(struct nvdimm *nvdimm,
>  }
>
>  static int intel_dimm_security_state(struct nvdimm *nvdimm,
> -               enum nvdimm_security_state *state)
> +               enum nvdimm_security_state *state,
> +               enum nvdimm_security_extended_state *ext_state)

Rather than require 2 state pointers to be passed in, just have a
single state a flags parameter to indicate which state is being
retrieved.

>  {
>         int cmd_rc, rc = 0;
>         struct nfit_mem *nfit_mem = nvdimm_provider_data(nvdimm);
> @@ -465,9 +583,17 @@ static int intel_dimm_security_state(struct nvdimm *nvdimm,
[..]
> diff --git a/include/linux/libnvdimm.h b/include/linux/libnvdimm.h
> index c3c5a1c6b1b7..3d14f73c7bca 100644
> --- a/include/linux/libnvdimm.h
> +++ b/include/linux/libnvdimm.h
> @@ -167,6 +167,13 @@ enum nvdimm_security_state {
>         NVDIMM_SECURITY_UNSUPPORTED,
>  };
>
> +enum nvdimm_security_extended_state {
> +       NVDIMM_SECURITY_MASTER_INVALID = 0,
> +       NVDIMM_SECURITY_MASTER_DISABLED,
> +       NVDIMM_SECURITY_MASTER_ENABLED,
> +       NVDIMM_SECURITY_MASTER_FROZEN,
> +};

I don't see a need for nvdimm_security_extended_state. Just have 2
instances of nvdimm_security_state one for master and one for the base
state.

>  struct nvdimm_security_ops {
>         int (*state)(struct nvdimm *nvdimm,
> -                       enum nvdimm_security_state *state);
> +                       enum nvdimm_security_state *state,
> +                       enum nvdimm_security_extended_state *ex_state);
>         int (*unlock)(struct nvdimm *nvdimm,
>                         const struct nvdimm_key_data *nkey);
>         int (*change_key)(struct nvdimm *nvdimm,
> @@ -190,6 +198,11 @@ struct nvdimm_security_ops {
>         int (*overwrite)(struct nvdimm *nvdimm,
>                         const struct nvdimm_key_data *nkey);
>         int (*query_overwrite)(struct nvdimm *nvdimm);
> +       int (*master_change_key)(struct nvdimm *nvdimm,
> +                       const struct nvdimm_key_data *old_key,
> +                       const struct nvdimm_key_data *new_key);
> +       int (*master_erase)(struct nvdimm *nvdimm,
> +                       const struct nvdimm_key_data *nkey);

Let's not add more operations for what is effectively a modified
version of the existing ops, just pass a 'flags' parameter to those
other ops.
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

Re: [PATCH 01/11] keys-encrypted: add nvdimm key format type to encrypted keys

[Dan Williams]

On Fri, Nov 9, 2018 at 2:13 PM Dave Jiang <dave.jiang@intel.com> wrote:
>
> Adding nvdimm key format type to encrypted keys in order to limit the size

s/Adding/Add an/

> of the key to 32-bytes.
>
> Signed-off-by: Dave Jiang <dave.jiang@intel.com>
> ---
>  security/keys/encrypted-keys/encrypted.c |   29 ++++++++++++++++++++---------
>  1 file changed, 20 insertions(+), 9 deletions(-)
>
> diff --git a/security/keys/encrypted-keys/encrypted.c b/security/keys/encrypted-keys/encrypted.c
> index d92cbf9687c3..182b4f136bdf 100644
> --- a/security/keys/encrypted-keys/encrypted.c
> +++ b/security/keys/encrypted-keys/encrypted.c
> @@ -45,6 +45,7 @@ static const char hmac_alg[] = "hmac(sha256)";
>  static const char blkcipher_alg[] = "cbc(aes)";
>  static const char key_format_default[] = "default";
>  static const char key_format_ecryptfs[] = "ecryptfs";
> +static const char key_format_nvdimm[] = "nvdimm";
>  static unsigned int ivsize;
>  static int blksize;
>
> @@ -54,6 +55,7 @@ static int blksize;
>  #define HASH_SIZE SHA256_DIGEST_SIZE
>  #define MAX_DATA_SIZE 4096
>  #define MIN_DATA_SIZE  20
> +#define KEY_NVDIMM_PAYLOAD_LEN 32
>
>  static struct crypto_shash *hash_tfm;
>
> @@ -62,12 +64,13 @@ enum {
>  };
>
>  enum {
> -       Opt_error = -1, Opt_default, Opt_ecryptfs
> +       Opt_error = -1, Opt_default, Opt_ecryptfs, Opt_nvdimm
>  };
>
>  static const match_table_t key_format_tokens = {
>         {Opt_default, "default"},
>         {Opt_ecryptfs, "ecryptfs"},
> +       {Opt_nvdimm, "nvdimm"},
>         {Opt_error, NULL}
>  };
>
> @@ -195,6 +198,7 @@ static int datablob_parse(char *datablob, const char **format,
>         key_format = match_token(p, key_format_tokens, args);
>         switch (key_format) {
>         case Opt_ecryptfs:
> +       case Opt_nvdimm:
>         case Opt_default:
>                 *format = p;
>                 *master_desc = strsep(&datablob, " \t");
> @@ -625,15 +629,22 @@ static struct encrypted_key_payload *encrypted_key_alloc(struct key *key,
>         format_len = (!format) ? strlen(key_format_default) : strlen(format);
>         decrypted_datalen = dlen;
>         payload_datalen = decrypted_datalen;
> -       if (format && !strcmp(format, key_format_ecryptfs)) {
> -               if (dlen != ECRYPTFS_MAX_KEY_BYTES) {
> -                       pr_err("encrypted_key: keylen for the ecryptfs format "
> -                              "must be equal to %d bytes\n",
> -                              ECRYPTFS_MAX_KEY_BYTES);
> -                       return ERR_PTR(-EINVAL);
> +       if (format) {
> +               if (!strcmp(format, key_format_ecryptfs)) {
> +                       if (dlen != ECRYPTFS_MAX_KEY_BYTES) {
> +                               pr_err("encrypted_key: keylen for the ecryptfs format must be equal to %d bytes\n",
> +                                       ECRYPTFS_MAX_KEY_BYTES);
> +                               return ERR_PTR(-EINVAL);
> +                       }
> +                       decrypted_datalen = ECRYPTFS_MAX_KEY_BYTES;
> +                       payload_datalen = sizeof(struct ecryptfs_auth_tok);
> +               } else if (!strcmp(format, key_format_nvdimm)) {
> +                       if (decrypted_datalen != KEY_NVDIMM_PAYLOAD_LEN) {
> +                               pr_err("encrypted_key: nvdimm key payload incorrect length: %d\n",
> +                                               decrypted_datalen);
> +                               return ERR_PTR(-EINVAL);
> +                       }

I suspect this may not be the last key type that gets added, but I
wonder if we should instead create key-types based on the dlen size.
I.e. create a generic 32-byte key-type "enc32"? That way if another
32-byte requirement key comes along we don't need to come touch this
routine again.
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

Re: [PATCH 01/11] keys-encrypted: add nvdimm key format type to encrypted keys

[Dave Jiang]

On 11/27/18 12:20 AM, Dan Williams wrote:
> On Fri, Nov 9, 2018 at 2:13 PM Dave Jiang <dave.jiang@intel.com> wrote:
>>
>> Adding nvdimm key format type to encrypted keys in order to limit the size
> 
> s/Adding/Add an/
> 
>> of the key to 32-bytes.
>>
>> Signed-off-by: Dave Jiang <dave.jiang@intel.com>
>> ---
>>  security/keys/encrypted-keys/encrypted.c |   29 ++++++++++++++++++++---------
>>  1 file changed, 20 insertions(+), 9 deletions(-)
>>
>> diff --git a/security/keys/encrypted-keys/encrypted.c b/security/keys/encrypted-keys/encrypted.c
>> index d92cbf9687c3..182b4f136bdf 100644
>> --- a/security/keys/encrypted-keys/encrypted.c
>> +++ b/security/keys/encrypted-keys/encrypted.c
>> @@ -45,6 +45,7 @@ static const char hmac_alg[] = "hmac(sha256)";
>>  static const char blkcipher_alg[] = "cbc(aes)";
>>  static const char key_format_default[] = "default";
>>  static const char key_format_ecryptfs[] = "ecryptfs";
>> +static const char key_format_nvdimm[] = "nvdimm";
>>  static unsigned int ivsize;
>>  static int blksize;
>>
>> @@ -54,6 +55,7 @@ static int blksize;
>>  #define HASH_SIZE SHA256_DIGEST_SIZE
>>  #define MAX_DATA_SIZE 4096
>>  #define MIN_DATA_SIZE  20
>> +#define KEY_NVDIMM_PAYLOAD_LEN 32
>>
>>  static struct crypto_shash *hash_tfm;
>>
>> @@ -62,12 +64,13 @@ enum {
>>  };
>>
>>  enum {
>> -       Opt_error = -1, Opt_default, Opt_ecryptfs
>> +       Opt_error = -1, Opt_default, Opt_ecryptfs, Opt_nvdimm
>>  };
>>
>>  static const match_table_t key_format_tokens = {
>>         {Opt_default, "default"},
>>         {Opt_ecryptfs, "ecryptfs"},
>> +       {Opt_nvdimm, "nvdimm"},
>>         {Opt_error, NULL}
>>  };
>>
>> @@ -195,6 +198,7 @@ static int datablob_parse(char *datablob, const char **format,
>>         key_format = match_token(p, key_format_tokens, args);
>>         switch (key_format) {
>>         case Opt_ecryptfs:
>> +       case Opt_nvdimm:
>>         case Opt_default:
>>                 *format = p;
>>                 *master_desc = strsep(&datablob, " \t");
>> @@ -625,15 +629,22 @@ static struct encrypted_key_payload *encrypted_key_alloc(struct key *key,
>>         format_len = (!format) ? strlen(key_format_default) : strlen(format);
>>         decrypted_datalen = dlen;
>>         payload_datalen = decrypted_datalen;
>> -       if (format && !strcmp(format, key_format_ecryptfs)) {
>> -               if (dlen != ECRYPTFS_MAX_KEY_BYTES) {
>> -                       pr_err("encrypted_key: keylen for the ecryptfs format "
>> -                              "must be equal to %d bytes\n",
>> -                              ECRYPTFS_MAX_KEY_BYTES);
>> -                       return ERR_PTR(-EINVAL);
>> +       if (format) {
>> +               if (!strcmp(format, key_format_ecryptfs)) {
>> +                       if (dlen != ECRYPTFS_MAX_KEY_BYTES) {
>> +                               pr_err("encrypted_key: keylen for the ecryptfs format must be equal to %d bytes\n",
>> +                                       ECRYPTFS_MAX_KEY_BYTES);
>> +                               return ERR_PTR(-EINVAL);
>> +                       }
>> +                       decrypted_datalen = ECRYPTFS_MAX_KEY_BYTES;
>> +                       payload_datalen = sizeof(struct ecryptfs_auth_tok);
>> +               } else if (!strcmp(format, key_format_nvdimm)) {
>> +                       if (decrypted_datalen != KEY_NVDIMM_PAYLOAD_LEN) {
>> +                               pr_err("encrypted_key: nvdimm key payload incorrect length: %d\n",
>> +                                               decrypted_datalen);
>> +                               return ERR_PTR(-EINVAL);
>> +                       }
> 
> I suspect this may not be the last key type that gets added, but I
> wonder if we should instead create key-types based on the dlen size.
> I.e. create a generic 32-byte key-type "enc32"? That way if another
> 32-byte requirement key comes along we don't need to come touch this
> routine again.
> 

I'm ok with that if Mimi is.
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

Re: [PATCH 01/11] keys-encrypted: add nvdimm key format type to encrypted keys

[Mimi Zohar]

On Tue, 2018-11-27 at 09:20 -0700, Dave Jiang wrote:
> 
> On 11/27/18 12:20 AM, Dan Williams wrote:
> > On Fri, Nov 9, 2018 at 2:13 PM Dave Jiang <dave.jiang@intel.com> wrote:
> >>
> >> Adding nvdimm key format type to encrypted keys in order to limit the size
> > 
> > s/Adding/Add an/
> > 
> >> of the key to 32-bytes.
> >>
> >> Signed-off-by: Dave Jiang <dave.jiang@intel.com>
> >> ---
> >>  security/keys/encrypted-keys/encrypted.c |   29 ++++++++++++++++++++---------
> >>  1 file changed, 20 insertions(+), 9 deletions(-)
> >>
> >> diff --git a/security/keys/encrypted-keys/encrypted.c b/security/keys/encrypted-keys/encrypted.c
> >> index d92cbf9687c3..182b4f136bdf 100644
> >> --- a/security/keys/encrypted-keys/encrypted.c
> >> +++ b/security/keys/encrypted-keys/encrypted.c
> >> @@ -45,6 +45,7 @@ static const char hmac_alg[] = "hmac(sha256)";
> >>  static const char blkcipher_alg[] = "cbc(aes)";
> >>  static const char key_format_default[] = "default";
> >>  static const char key_format_ecryptfs[] = "ecryptfs";
> >> +static const char key_format_nvdimm[] = "nvdimm";
> >>  static unsigned int ivsize;
> >>  static int blksize;
> >>
> >> @@ -54,6 +55,7 @@ static int blksize;
> >>  #define HASH_SIZE SHA256_DIGEST_SIZE
> >>  #define MAX_DATA_SIZE 4096
> >>  #define MIN_DATA_SIZE  20
> >> +#define KEY_NVDIMM_PAYLOAD_LEN 32
> >>
> >>  static struct crypto_shash *hash_tfm;
> >>
> >> @@ -62,12 +64,13 @@ enum {
> >>  };
> >>
> >>  enum {
> >> -       Opt_error = -1, Opt_default, Opt_ecryptfs
> >> +       Opt_error = -1, Opt_default, Opt_ecryptfs, Opt_nvdimm
> >>  };
> >>
> >>  static const match_table_t key_format_tokens = {
> >>         {Opt_default, "default"},
> >>         {Opt_ecryptfs, "ecryptfs"},
> >> +       {Opt_nvdimm, "nvdimm"},
> >>         {Opt_error, NULL}
> >>  };
> >>
> >> @@ -195,6 +198,7 @@ static int datablob_parse(char *datablob, const char **format,
> >>         key_format = match_token(p, key_format_tokens, args);
> >>         switch (key_format) {
> >>         case Opt_ecryptfs:
> >> +       case Opt_nvdimm:
> >>         case Opt_default:
> >>                 *format = p;
> >>                 *master_desc = strsep(&datablob, " \t");
> >> @@ -625,15 +629,22 @@ static struct encrypted_key_payload *encrypted_key_alloc(struct key *key,
> >>         format_len = (!format) ? strlen(key_format_default) : strlen(format);
> >>         decrypted_datalen = dlen;
> >>         payload_datalen = decrypted_datalen;
> >> -       if (format && !strcmp(format, key_format_ecryptfs)) {
> >> -               if (dlen != ECRYPTFS_MAX_KEY_BYTES) {
> >> -                       pr_err("encrypted_key: keylen for the ecryptfs format "
> >> -                              "must be equal to %d bytes\n",
> >> -                              ECRYPTFS_MAX_KEY_BYTES);
> >> -                       return ERR_PTR(-EINVAL);
> >> +       if (format) {
> >> +               if (!strcmp(format, key_format_ecryptfs)) {
> >> +                       if (dlen != ECRYPTFS_MAX_KEY_BYTES) {
> >> +                               pr_err("encrypted_key: keylen for the ecryptfs format must be equal to %d bytes\n",
> >> +                                       ECRYPTFS_MAX_KEY_BYTES);
> >> +                               return ERR_PTR(-EINVAL);
> >> +                       }
> >> +                       decrypted_datalen = ECRYPTFS_MAX_KEY_BYTES;
> >> +                       payload_datalen = sizeof(struct ecryptfs_auth_tok);
> >> +               } else if (!strcmp(format, key_format_nvdimm)) {
> >> +                       if (decrypted_datalen != KEY_NVDIMM_PAYLOAD_LEN) {
> >> +                               pr_err("encrypted_key: nvdimm key payload incorrect length: %d\n",
> >> +                                               decrypted_datalen);
> >> +                               return ERR_PTR(-EINVAL);
> >> +                       }
> > 
> > I suspect this may not be the last key type that gets added, but I
> > wonder if we should instead create key-types based on the dlen size.
> > I.e. create a generic 32-byte key-type "enc32"? That way if another
> > 32-byte requirement key comes along we don't need to come touch this
> > routine again.
> > 
> 
> I'm ok with that if Mimi is.

If the usage (eg. format: ecryptfs, nvdimm) limits the dlen size(s),
how will defining generic key-types help?  If there are no usage size
limitations, then there would be no usage specific definition.

I must be missing something.

Mimi

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

Re: [PATCH 01/11] keys-encrypted: add nvdimm key format type to encrypted keys

[Dan Williams]

On Tue, Nov 27, 2018 at 10:24 AM Mimi Zohar <zohar@linux.ibm.com> wrote:
>
> On Tue, 2018-11-27 at 09:20 -0700, Dave Jiang wrote:
> >
> > On 11/27/18 12:20 AM, Dan Williams wrote:
> > > On Fri, Nov 9, 2018 at 2:13 PM Dave Jiang <dave.jiang@intel.com> wrote:
> > >>
> > >> Adding nvdimm key format type to encrypted keys in order to limit the size
> > >
> > > s/Adding/Add an/
> > >
> > >> of the key to 32-bytes.
> > >>
> > >> Signed-off-by: Dave Jiang <dave.jiang@intel.com>
> > >> ---
> > >>  security/keys/encrypted-keys/encrypted.c |   29 ++++++++++++++++++++---------
> > >>  1 file changed, 20 insertions(+), 9 deletions(-)
> > >>
> > >> diff --git a/security/keys/encrypted-keys/encrypted.c b/security/keys/encrypted-keys/encrypted.c
> > >> index d92cbf9687c3..182b4f136bdf 100644
> > >> --- a/security/keys/encrypted-keys/encrypted.c
> > >> +++ b/security/keys/encrypted-keys/encrypted.c
> > >> @@ -45,6 +45,7 @@ static const char hmac_alg[] = "hmac(sha256)";
> > >>  static const char blkcipher_alg[] = "cbc(aes)";
> > >>  static const char key_format_default[] = "default";
> > >>  static const char key_format_ecryptfs[] = "ecryptfs";
> > >> +static const char key_format_nvdimm[] = "nvdimm";
> > >>  static unsigned int ivsize;
> > >>  static int blksize;
> > >>
> > >> @@ -54,6 +55,7 @@ static int blksize;
> > >>  #define HASH_SIZE SHA256_DIGEST_SIZE
> > >>  #define MAX_DATA_SIZE 4096
> > >>  #define MIN_DATA_SIZE  20
> > >> +#define KEY_NVDIMM_PAYLOAD_LEN 32
> > >>
> > >>  static struct crypto_shash *hash_tfm;
> > >>
> > >> @@ -62,12 +64,13 @@ enum {
> > >>  };
> > >>
> > >>  enum {
> > >> -       Opt_error = -1, Opt_default, Opt_ecryptfs
> > >> +       Opt_error = -1, Opt_default, Opt_ecryptfs, Opt_nvdimm
> > >>  };
> > >>
> > >>  static const match_table_t key_format_tokens = {
> > >>         {Opt_default, "default"},
> > >>         {Opt_ecryptfs, "ecryptfs"},
> > >> +       {Opt_nvdimm, "nvdimm"},
> > >>         {Opt_error, NULL}
> > >>  };
> > >>
> > >> @@ -195,6 +198,7 @@ static int datablob_parse(char *datablob, const char **format,
> > >>         key_format = match_token(p, key_format_tokens, args);
> > >>         switch (key_format) {
> > >>         case Opt_ecryptfs:
> > >> +       case Opt_nvdimm:
> > >>         case Opt_default:
> > >>                 *format = p;
> > >>                 *master_desc = strsep(&datablob, " \t");
> > >> @@ -625,15 +629,22 @@ static struct encrypted_key_payload *encrypted_key_alloc(struct key *key,
> > >>         format_len = (!format) ? strlen(key_format_default) : strlen(format);
> > >>         decrypted_datalen = dlen;
> > >>         payload_datalen = decrypted_datalen;
> > >> -       if (format && !strcmp(format, key_format_ecryptfs)) {
> > >> -               if (dlen != ECRYPTFS_MAX_KEY_BYTES) {
> > >> -                       pr_err("encrypted_key: keylen for the ecryptfs format "
> > >> -                              "must be equal to %d bytes\n",
> > >> -                              ECRYPTFS_MAX_KEY_BYTES);
> > >> -                       return ERR_PTR(-EINVAL);
> > >> +       if (format) {
> > >> +               if (!strcmp(format, key_format_ecryptfs)) {
> > >> +                       if (dlen != ECRYPTFS_MAX_KEY_BYTES) {
> > >> +                               pr_err("encrypted_key: keylen for the ecryptfs format must be equal to %d bytes\n",
> > >> +                                       ECRYPTFS_MAX_KEY_BYTES);
> > >> +                               return ERR_PTR(-EINVAL);
> > >> +                       }
> > >> +                       decrypted_datalen = ECRYPTFS_MAX_KEY_BYTES;
> > >> +                       payload_datalen = sizeof(struct ecryptfs_auth_tok);
> > >> +               } else if (!strcmp(format, key_format_nvdimm)) {
> > >> +                       if (decrypted_datalen != KEY_NVDIMM_PAYLOAD_LEN) {
> > >> +                               pr_err("encrypted_key: nvdimm key payload incorrect length: %d\n",
> > >> +                                               decrypted_datalen);
> > >> +                               return ERR_PTR(-EINVAL);
> > >> +                       }
> > >
> > > I suspect this may not be the last key type that gets added, but I
> > > wonder if we should instead create key-types based on the dlen size.
> > > I.e. create a generic 32-byte key-type "enc32"? That way if another
> > > 32-byte requirement key comes along we don't need to come touch this
> > > routine again.
> > >
> >
> > I'm ok with that if Mimi is.
>
> If the usage (eg. format: ecryptfs, nvdimm) limits the dlen size(s),
> how will defining generic key-types help?  If there are no usage size
> limitations, then there would be no usage specific definition.
>
> I must be missing something.

...or I did a poor job of describing the problem. I'm just looking
ahead to another potential encrypted-key use case, but instead of
nvdimms this would be to lock / unlock persistent memory "namespace"
objects. If it turns out the namespace key is just another 32-byte
encrypted key should the encrypted-keys core grow support for a new
"namespace" key type, should it reuse the "nvdimm" key type, or should
we define a generic 32-byte payload-size-requirement key type?
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

Re: [PATCH 01/11] keys-encrypted: add nvdimm key format type to encrypted keys

[Mimi Zohar]

On Tue, 2018-11-27 at 11:10 -0800, Dan Williams wrote:
> On Tue, Nov 27, 2018 at 10:24 AM Mimi Zohar <zohar@linux.ibm.com> wrote:
> >
> > On Tue, 2018-11-27 at 09:20 -0700, Dave Jiang wrote:
> > >
> > > On 11/27/18 12:20 AM, Dan Williams wrote:
> > > > On Fri, Nov 9, 2018 at 2:13 PM Dave Jiang <dave.jiang@intel.com> wrote:
> > > >>
> > > >> Adding nvdimm key format type to encrypted keys in order to limit the size
> > > >
> > > > s/Adding/Add an/
> > > >
> > > >> of the key to 32-bytes.
> > > >>
> > > >> Signed-off-by: Dave Jiang <dave.jiang@intel.com>
> > > >> ---
> > > >>  security/keys/encrypted-keys/encrypted.c |   29 ++++++++++++++++++++---------
> > > >>  1 file changed, 20 insertions(+), 9 deletions(-)
> > > >>
> > > >> diff --git a/security/keys/encrypted-keys/encrypted.c b/security/keys/encrypted-keys/encrypted.c
> > > >> index d92cbf9687c3..182b4f136bdf 100644
> > > >> --- a/security/keys/encrypted-keys/encrypted.c
> > > >> +++ b/security/keys/encrypted-keys/encrypted.c
> > > >> @@ -45,6 +45,7 @@ static const char hmac_alg[] = "hmac(sha256)";
> > > >>  static const char blkcipher_alg[] = "cbc(aes)";
> > > >>  static const char key_format_default[] = "default";
> > > >>  static const char key_format_ecryptfs[] = "ecryptfs";
> > > >> +static const char key_format_nvdimm[] = "nvdimm";
> > > >>  static unsigned int ivsize;
> > > >>  static int blksize;
> > > >>
> > > >> @@ -54,6 +55,7 @@ static int blksize;
> > > >>  #define HASH_SIZE SHA256_DIGEST_SIZE
> > > >>  #define MAX_DATA_SIZE 4096
> > > >>  #define MIN_DATA_SIZE  20
> > > >> +#define KEY_NVDIMM_PAYLOAD_LEN 32
> > > >>
> > > >>  static struct crypto_shash *hash_tfm;
> > > >>
> > > >> @@ -62,12 +64,13 @@ enum {
> > > >>  };
> > > >>
> > > >>  enum {
> > > >> -       Opt_error = -1, Opt_default, Opt_ecryptfs
> > > >> +       Opt_error = -1, Opt_default, Opt_ecryptfs, Opt_nvdimm
> > > >>  };
> > > >>
> > > >>  static const match_table_t key_format_tokens = {
> > > >>         {Opt_default, "default"},
> > > >>         {Opt_ecryptfs, "ecryptfs"},
> > > >> +       {Opt_nvdimm, "nvdimm"},
> > > >>         {Opt_error, NULL}
> > > >>  };
> > > >>
> > > >> @@ -195,6 +198,7 @@ static int datablob_parse(char *datablob, const char **format,
> > > >>         key_format = match_token(p, key_format_tokens, args);
> > > >>         switch (key_format) {
> > > >>         case Opt_ecryptfs:
> > > >> +       case Opt_nvdimm:
> > > >>         case Opt_default:
> > > >>                 *format = p;
> > > >>                 *master_desc = strsep(&datablob, " \t");
> > > >> @@ -625,15 +629,22 @@ static struct encrypted_key_payload *encrypted_key_alloc(struct key *key,
> > > >>         format_len = (!format) ? strlen(key_format_default) : strlen(format);
> > > >>         decrypted_datalen = dlen;
> > > >>         payload_datalen = decrypted_datalen;
> > > >> -       if (format && !strcmp(format, key_format_ecryptfs)) {
> > > >> -               if (dlen != ECRYPTFS_MAX_KEY_BYTES) {
> > > >> -                       pr_err("encrypted_key: keylen for the ecryptfs format "
> > > >> -                              "must be equal to %d bytes\n",
> > > >> -                              ECRYPTFS_MAX_KEY_BYTES);
> > > >> -                       return ERR_PTR(-EINVAL);
> > > >> +       if (format) {
> > > >> +               if (!strcmp(format, key_format_ecryptfs)) {
> > > >> +                       if (dlen != ECRYPTFS_MAX_KEY_BYTES) {
> > > >> +                               pr_err("encrypted_key: keylen for the ecryptfs format must be equal to %d bytes\n",
> > > >> +                                       ECRYPTFS_MAX_KEY_BYTES);
> > > >> +                               return ERR_PTR(-EINVAL);
> > > >> +                       }
> > > >> +                       decrypted_datalen = ECRYPTFS_MAX_KEY_BYTES;
> > > >> +                       payload_datalen = sizeof(struct ecryptfs_auth_tok);
> > > >> +               } else if (!strcmp(format, key_format_nvdimm)) {
> > > >> +                       if (decrypted_datalen != KEY_NVDIMM_PAYLOAD_LEN) {
> > > >> +                               pr_err("encrypted_key: nvdimm key payload incorrect length: %d\n",
> > > >> +                                               decrypted_datalen);
> > > >> +                               return ERR_PTR(-EINVAL);
> > > >> +                       }
> > > >
> > > > I suspect this may not be the last key type that gets added, but I
> > > > wonder if we should instead create key-types based on the dlen size.
> > > > I.e. create a generic 32-byte key-type "enc32"? That way if another
> > > > 32-byte requirement key comes along we don't need to come touch this
> > > > routine again.
> > > >
> > >
> > > I'm ok with that if Mimi is.
> >
> > If the usage (eg. format: ecryptfs, nvdimm) limits the dlen size(s),
> > how will defining generic key-types help?  If there are no usage size
> > limitations, then there would be no usage specific definition.
> >
> > I must be missing something.
> 
> ...or I did a poor job of describing the problem. I'm just looking
> ahead to another potential encrypted-key use case, but instead of
> nvdimms this would be to lock / unlock persistent memory "namespace"
> objects. If it turns out the namespace key is just another 32-byte
> encrypted key should the encrypted-keys core grow support for a new
> "namespace" key type, should it reuse the "nvdimm" key type, or should
> we define a generic 32-byte payload-size-requirement key type?

Even with a generic length, there will be format (eg. ecryptfs,
nvdimm, namespace) specific code.  Otherwise you wouldn't be defining
a new format type.

switch(dlen) {
	case enc32:
		if (ecryptfs) goto fail;
		...
		break;
	case enc64:
		if (nvdimm || ns) goto fail;
		...
		break;
	default:
}

Mimi


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

Re: [PATCH 01/11] keys-encrypted: add nvdimm key format type to encrypted keys

[Dan Williams]

On Tue, Nov 27, 2018 at 11:35 AM Mimi Zohar <zohar@linux.ibm.com> wrote:
>
> On Tue, 2018-11-27 at 11:10 -0800, Dan Williams wrote:
> > On Tue, Nov 27, 2018 at 10:24 AM Mimi Zohar <zohar@linux.ibm.com> wrote:
> > >
> > > On Tue, 2018-11-27 at 09:20 -0700, Dave Jiang wrote:
> > > >
> > > > On 11/27/18 12:20 AM, Dan Williams wrote:
> > > > > On Fri, Nov 9, 2018 at 2:13 PM Dave Jiang <dave.jiang@intel.com> wrote:
> > > > >>
> > > > >> Adding nvdimm key format type to encrypted keys in order to limit the size
> > > > >
> > > > > s/Adding/Add an/
> > > > >
> > > > >> of the key to 32-bytes.
> > > > >>
> > > > >> Signed-off-by: Dave Jiang <dave.jiang@intel.com>
> > > > >> ---
> > > > >>  security/keys/encrypted-keys/encrypted.c |   29 ++++++++++++++++++++---------
> > > > >>  1 file changed, 20 insertions(+), 9 deletions(-)
> > > > >>
> > > > >> diff --git a/security/keys/encrypted-keys/encrypted.c b/security/keys/encrypted-keys/encrypted.c
> > > > >> index d92cbf9687c3..182b4f136bdf 100644
> > > > >> --- a/security/keys/encrypted-keys/encrypted.c
> > > > >> +++ b/security/keys/encrypted-keys/encrypted.c
> > > > >> @@ -45,6 +45,7 @@ static const char hmac_alg[] = "hmac(sha256)";
> > > > >>  static const char blkcipher_alg[] = "cbc(aes)";
> > > > >>  static const char key_format_default[] = "default";
> > > > >>  static const char key_format_ecryptfs[] = "ecryptfs";
> > > > >> +static const char key_format_nvdimm[] = "nvdimm";
> > > > >>  static unsigned int ivsize;
> > > > >>  static int blksize;
> > > > >>
> > > > >> @@ -54,6 +55,7 @@ static int blksize;
> > > > >>  #define HASH_SIZE SHA256_DIGEST_SIZE
> > > > >>  #define MAX_DATA_SIZE 4096
> > > > >>  #define MIN_DATA_SIZE  20
> > > > >> +#define KEY_NVDIMM_PAYLOAD_LEN 32
> > > > >>
> > > > >>  static struct crypto_shash *hash_tfm;
> > > > >>
> > > > >> @@ -62,12 +64,13 @@ enum {
> > > > >>  };
> > > > >>
> > > > >>  enum {
> > > > >> -       Opt_error = -1, Opt_default, Opt_ecryptfs
> > > > >> +       Opt_error = -1, Opt_default, Opt_ecryptfs, Opt_nvdimm
> > > > >>  };
> > > > >>
> > > > >>  static const match_table_t key_format_tokens = {
> > > > >>         {Opt_default, "default"},
> > > > >>         {Opt_ecryptfs, "ecryptfs"},
> > > > >> +       {Opt_nvdimm, "nvdimm"},
> > > > >>         {Opt_error, NULL}
> > > > >>  };
> > > > >>
> > > > >> @@ -195,6 +198,7 @@ static int datablob_parse(char *datablob, const char **format,
> > > > >>         key_format = match_token(p, key_format_tokens, args);
> > > > >>         switch (key_format) {
> > > > >>         case Opt_ecryptfs:
> > > > >> +       case Opt_nvdimm:
> > > > >>         case Opt_default:
> > > > >>                 *format = p;
> > > > >>                 *master_desc = strsep(&datablob, " \t");
> > > > >> @@ -625,15 +629,22 @@ static struct encrypted_key_payload *encrypted_key_alloc(struct key *key,
> > > > >>         format_len = (!format) ? strlen(key_format_default) : strlen(format);
> > > > >>         decrypted_datalen = dlen;
> > > > >>         payload_datalen = decrypted_datalen;
> > > > >> -       if (format && !strcmp(format, key_format_ecryptfs)) {
> > > > >> -               if (dlen != ECRYPTFS_MAX_KEY_BYTES) {
> > > > >> -                       pr_err("encrypted_key: keylen for the ecryptfs format "
> > > > >> -                              "must be equal to %d bytes\n",
> > > > >> -                              ECRYPTFS_MAX_KEY_BYTES);
> > > > >> -                       return ERR_PTR(-EINVAL);
> > > > >> +       if (format) {
> > > > >> +               if (!strcmp(format, key_format_ecryptfs)) {
> > > > >> +                       if (dlen != ECRYPTFS_MAX_KEY_BYTES) {
> > > > >> +                               pr_err("encrypted_key: keylen for the ecryptfs format must be equal to %d bytes\n",
> > > > >> +                                       ECRYPTFS_MAX_KEY_BYTES);
> > > > >> +                               return ERR_PTR(-EINVAL);
> > > > >> +                       }
> > > > >> +                       decrypted_datalen = ECRYPTFS_MAX_KEY_BYTES;
> > > > >> +                       payload_datalen = sizeof(struct ecryptfs_auth_tok);
> > > > >> +               } else if (!strcmp(format, key_format_nvdimm)) {
> > > > >> +                       if (decrypted_datalen != KEY_NVDIMM_PAYLOAD_LEN) {
> > > > >> +                               pr_err("encrypted_key: nvdimm key payload incorrect length: %d\n",
> > > > >> +                                               decrypted_datalen);
> > > > >> +                               return ERR_PTR(-EINVAL);
> > > > >> +                       }
> > > > >
> > > > > I suspect this may not be the last key type that gets added, but I
> > > > > wonder if we should instead create key-types based on the dlen size.
> > > > > I.e. create a generic 32-byte key-type "enc32"? That way if another
> > > > > 32-byte requirement key comes along we don't need to come touch this
> > > > > routine again.
> > > > >
> > > >
> > > > I'm ok with that if Mimi is.
> > >
> > > If the usage (eg. format: ecryptfs, nvdimm) limits the dlen size(s),
> > > how will defining generic key-types help?  If there are no usage size
> > > limitations, then there would be no usage specific definition.
> > >
> > > I must be missing something.
> >
> > ...or I did a poor job of describing the problem. I'm just looking
> > ahead to another potential encrypted-key use case, but instead of
> > nvdimms this would be to lock / unlock persistent memory "namespace"
> > objects. If it turns out the namespace key is just another 32-byte
> > encrypted key should the encrypted-keys core grow support for a new
> > "namespace" key type, should it reuse the "nvdimm" key type, or should
> > we define a generic 32-byte payload-size-requirement key type?
>
> Even with a generic length, there will be format (eg. ecryptfs,
> nvdimm, namespace) specific code.  Otherwise you wouldn't be defining
> a new format type.
>
> switch(dlen) {
>         case enc32:
>                 if (ecryptfs) goto fail;
>                 ...
>                 break;
>         case enc64:
>                 if (nvdimm || ns) goto fail;
>                 ...
>                 break;
>         default:
> }
>

I was thinking that the generic-length *is* the format. This does not
work for ecyptfs because it has that:

    payload_datalen = sizeof(struct ecryptfs_auth_tok);

...detail that is ecryptfs specific. For nvdimm the only detail of the
format is the decrypted-data-length.  However, I get the feeling I'm
proposing a solution to a problem that does not exist yet. Let's just
go with the "nvdimm" format for now.
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

Re: [PATCH 01/11] keys-encrypted: add nvdimm key format type to encrypted keys

[Mimi Zohar]

On Tue, 2018-11-27 at 11:48 -0800, Dan Williams wrote:

> I was thinking that the generic-length *is* the format. This does not
> work for ecyptfs because it has that:
> 
>     payload_datalen = sizeof(struct ecryptfs_auth_tok);
> 
> ...detail that is ecryptfs specific. For nvdimm the only detail of the
> format is the decrypted-data-length.  However, I get the feeling I'm
> proposing a solution to a problem that does not exist yet. Let's just
> go with the "nvdimm" format for now.

Ah, that makes more sense now.  Defining "Opt_nvdimm" or the generic
"Opt_enc32" is fine.  Missing from this patch is the update to
Documentation/security/keys/trusted-encrypted.rst.  Otherwise this
patch looks fine.

Mimi

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

Re: [PATCH 01/11] keys-encrypted: add nvdimm key format type to encrypted keys

[Dave Jiang]

On 11/27/18 1:10 PM, Mimi Zohar wrote:
> On Tue, 2018-11-27 at 11:48 -0800, Dan Williams wrote:
> 
>> I was thinking that the generic-length *is* the format. This does not
>> work for ecyptfs because it has that:
>>
>>     payload_datalen = sizeof(struct ecryptfs_auth_tok);
>>
>> ...detail that is ecryptfs specific. For nvdimm the only detail of the
>> format is the decrypted-data-length.  However, I get the feeling I'm
>> proposing a solution to a problem that does not exist yet. Let's just
>> go with the "nvdimm" format for now.
> 
> Ah, that makes more sense now.  Defining "Opt_nvdimm" or the generic
> "Opt_enc32" is fine.  Missing from this patch is the update to
> Documentation/security/keys/trusted-encrypted.rst.  Otherwise this
> patch looks fine.

I'll go with enc32 and update the doc and resubmit with the series.
Thanks Mimi.

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