From: Dave Jiang <dave.jiang@intel.com>
To: Mimi Zohar <zohar@linux.ibm.com>,
dan.j.williams@intel.com, zohar@linux.vnet.ibm.com
Cc: linux-nvdimm@lists.01.org
Subject: Re: [PATCH 02/11] libnvdimm/security: change clear text nvdimm keys to encrypted keys
Date: Mon, 12 Nov 2018 08:45:03 -0700 [thread overview]
Message-ID: <1cc6edd6-8926-1be1-04f3-85ce59bfdfb4@intel.com> (raw)
In-Reply-To: <1541957268.3734.53.camel@linux.ibm.com>
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
next prev parent reply other threads:[~2018-11-12 15:45 UTC|newest]
Thread overview: 34+ messages / expand[flat|nested] mbox.gz Atom feed top
2018-11-09 22:13 [PATCH 00/11] Additional patches for nvdimm security support Dave Jiang
2018-11-09 22:13 ` [PATCH 01/11] keys-encrypted: add nvdimm key format type to encrypted keys Dave Jiang
2018-11-27 7:20 ` Dan Williams
2018-11-27 16:20 ` Dave Jiang
2018-11-27 18:24 ` Mimi Zohar
2018-11-27 19:10 ` Dan Williams
2018-11-27 19:35 ` Mimi Zohar
2018-11-27 19:48 ` Dan Williams
2018-11-27 20:10 ` Mimi Zohar
2018-11-27 20:15 ` Dave Jiang
2018-11-09 22:13 ` [PATCH 02/11] libnvdimm/security: change clear text nvdimm keys " Dave Jiang
2018-11-10 1:45 ` Dan Williams
2018-11-11 17:27 ` Mimi Zohar
2018-11-11 19:20 ` Dan Williams
2018-11-11 20:09 ` Mimi Zohar
2018-11-12 15:42 ` Dave Jiang
2018-11-12 18:49 ` Mimi Zohar
2018-11-12 20:13 ` Dave Jiang
2018-11-12 15:45 ` Dave Jiang [this message]
2018-11-12 19:04 ` Mimi Zohar
2018-11-09 22:14 ` [PATCH 03/11] libnvdimm/security: add override module param for key self verification Dave Jiang
2018-11-09 22:14 ` [PATCH 04/11] libnvdimm/security: introduce NDD_SECURITY_BUSY flag Dave Jiang
2018-11-09 22:14 ` [PATCH 05/11] acpi/nfit, libnvdimm/security: Add security DSM overwrite support Dave Jiang
2018-11-09 22:14 ` [PATCH 06/11] tools/testing/nvdimm: Add overwrite support for nfit_test Dave Jiang
2018-11-09 22:14 ` [PATCH 07/11] libnvdimm/security: add overwrite status notification Dave Jiang
2018-11-10 2:59 ` Elliott, Robert (Persistent Memory)
2018-11-12 20:26 ` Dave Jiang
2018-11-09 22:14 ` [PATCH 08/11] libnvdimm/security: add documentation for ovewrite Dave Jiang
2018-11-09 22:14 ` [PATCH 09/11] acpi/nfit, libnvdimm/security: add Intel DSM 1.8 master passphrase support Dave Jiang
2018-11-25 21:24 ` Dan Williams
2018-11-09 22:14 ` [PATCH 10/11] tools/testing/nvdimm: add Intel DSM 1.8 support for nfit_test Dave Jiang
2018-11-10 3:15 ` Elliott, Robert (Persistent Memory)
2018-11-12 20:27 ` Dave Jiang
2018-11-09 22:14 ` [PATCH 11/11] acpi/nfit: prevent indiscriminate DSM payload dumping for security DSMs Dave Jiang
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=1cc6edd6-8926-1be1-04f3-85ce59bfdfb4@intel.com \
--to=dave.jiang@intel.com \
--cc=dan.j.williams@intel.com \
--cc=linux-nvdimm@lists.01.org \
--cc=zohar@linux.ibm.com \
--cc=zohar@linux.vnet.ibm.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).