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

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