-----Original Message----- From: Linux-nvdimm <linux-nvdimm-bounces(a)lists.01.org&gt; 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");

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(a)intel.com&gt; --- 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; }

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(a)intel.com&gt; > --- > 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; > } >

> -----Original Message----- > From: Linux-nvdimm <linux-nvdimm-bounces(a)lists.01.org&gt; 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

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(a)intel.com&gt; --- 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)

{ 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, +};

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);

On Fri, Nov 9, 2018 at 2:13 PM Dave Jiang <dave.jiang(a)intel.com&gt; 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(a)intel.com&gt; > --- > 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.

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(a)intel.com&gt; 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(a)intel.com&gt; > >> --- > >> 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.

On Tue, 2018-11-27 at 11:10 -0800, Dan Williams wrote: > On Tue, Nov 27, 2018 at 10:24 AM Mimi Zohar <zohar(a)linux.ibm.com&gt; 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(a)intel.com&gt; 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(a)intel.com&gt; > > > >> --- > > > >> 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: }