diff mbox series

[v8,06/12] nfit/libnvdimm: add set passphrase support for Intel nvdimms

Message ID 153549647177.4089.18112512093259339717.stgit@djiang5-desk3.ch.intel.com (mailing list archive)
State New, archived
Headers show
Series Adding security support for nvdimm | expand

Commit Message

Dave Jiang Aug. 28, 2018, 10:47 p.m. UTC
Add support for setting and/or updating passphrase on the Intel nvdimms.
The passphrase is pulled from userspace through the kernel key management.
We trigger the update via writing "update" to the sysfs attribute
"security". The state of the security can also be read via the "security"
attribute. libnvdimm will generically support the key_change API call.

Signed-off-by: Dave Jiang <dave.jiang@intel.com>
---
 drivers/acpi/nfit/intel.c  |   68 ++++++++++++
 drivers/nvdimm/dimm_devs.c |  256 ++++++++++++++++++++++++++++++++++++++++++++
 include/linux/libnvdimm.h  |    5 +
 3 files changed, 329 insertions(+)

Comments

David Howells Sept. 21, 2018, 11:57 p.m. UTC | #1
Dave Jiang <dave.jiang@intel.com> wrote:

> +	new_key = key_alloc(&key_type_logon, key->description,
> +			GLOBAL_ROOT_UID, GLOBAL_ROOT_GID, &init_cred, 0,

KEY_POS_SEARCH?  KEY_USR_VIEW?

> +			KEY_ALLOC_NOT_IN_QUOTA, NULL);
> + ...
> +	down_read(&key->sem);
> +	payload = key->payload.data[0];
> +	rc = key_instantiate_and_link(new_key, payload->data, key->datalen,

payload->datalen, not key->datalen.

> +			nvdimm_keyring, NULL);

Okay, that's a weird way of going about things.  I presume you don't want to
add key to nvdimm_keyring - maybe in case it gets updated whilst you're using
it and your private key isn't in quota?

> +	up_read(&key->sem);
> +	if (rc < 0) {
> +		key_revoke(new_key);
> +		key_put(new_key);
> +		return NULL;
> +	}

Just putting it here should work since it didn't get linked to the keyring if
any errors occurred.  Revoking it too shouldn't be necessary.

> +	key_invalidate(key);
> +	key_put(key);

Why are you invalidating the user's key?

> +	keyref = lookup_user_key(id, 0, 0);

KEY_NEED_SEARCH?  Though I suppose it's not strictly necessary as it's a key
that's private to the kernel.

> +	if (old_keyid != 0) {
> +		old_key = nvdimm_get_key(dev);
> +		if (old_key) {
> +			if (key_serial(old_key) != old_keyid) {

Ummm...  That's not what I meant.  Given the permissions you've set on your
private key, userspace shouldn't be able to find it, let alone give you the
key ID.

What I meant here was to use, say, nvdimm_lookup_user_key() to get a key from
userspace that contains the old password.  You can use the description of the
key to search nvdimm_keyring for the private key and then compare the
passwords.

Then you don't need to passphrases in the new key.

> +	rc = nvdimm_check_key_len(key->datalen, update);

payload->datalen.

> +	down_read(&key->sem);

This needs to be earlier.  The payload attached to the new key can be replaced
by keyctl_update() at any time whilst you're not holding the lock, so you
cannot use key->payload[*] without holding the lock or the RCU read lock.

> +	if (update)
> +		key_invalidate(key);

The key doesn't belong to you - should you really be invalidating it?

> +	else {
> +		key_link(nvdimm_keyring, key);
> +		nvdimm->key = key;
> +		key->perm |= KEY_USR_SEARCH;
> +	}

Um - do you really want to be taking the key into your internal keyring?  Why
aren't you calling nvdimm_replace_key()?  Also, you shouldn't alter the
permission - it's not your key.

> +static int __parse_update(const char *buf, size_t len, unsigned int *old_id,
> +		unsigned int *new_id)
> +{

Try using sscanf()?

David
Dave Jiang Sept. 22, 2018, 12:25 a.m. UTC | #2
On 09/21/2018 04:57 PM, David Howells wrote:
> Dave Jiang <dave.jiang@intel.com> wrote:
> 
>> +	new_key = key_alloc(&key_type_logon, key->description,
>> +			GLOBAL_ROOT_UID, GLOBAL_ROOT_GID, &init_cred, 0,
> 
> KEY_POS_SEARCH?  KEY_USR_VIEW?
> 
>> +			KEY_ALLOC_NOT_IN_QUOTA, NULL);
>> + ...
>> +	down_read(&key->sem);
>> +	payload = key->payload.data[0];
>> +	rc = key_instantiate_and_link(new_key, payload->data, key->datalen,
> 
> payload->datalen, not key->datalen.
> 
>> +			nvdimm_keyring, NULL);
> 
> Okay, that's a weird way of going about things.  I presume you don't want to
> add key to nvdimm_keyring - maybe in case it gets updated whilst you're using
> it and your private key isn't in quota?
> 
>> +	up_read(&key->sem);
>> +	if (rc < 0) {
>> +		key_revoke(new_key);
>> +		key_put(new_key);
>> +		return NULL;
>> +	}
> 
> Just putting it here should work since it didn't get linked to the keyring if
> any errors occurred.  Revoking it too shouldn't be necessary.
> 
>> +	key_invalidate(key);
>> +	key_put(key);
> 
> Why are you invalidating the user's key?
> 
>> +	keyref = lookup_user_key(id, 0, 0);
> 
> KEY_NEED_SEARCH?  Though I suppose it's not strictly necessary as it's a key
> that's private to the kernel.
> 
>> +	if (old_keyid != 0) {
>> +		old_key = nvdimm_get_key(dev);
>> +		if (old_key) {
>> +			if (key_serial(old_key) != old_keyid) {
> 
> Ummm...  That's not what I meant.  Given the permissions you've set on your
> private key, userspace shouldn't be able to find it, let alone give you the
> key ID.
> 
> What I meant here was to use, say, nvdimm_lookup_user_key() to get a key from
> userspace that contains the old password.  You can use the description of the
> key to search nvdimm_keyring for the private key and then compare the
> passwords.

Ok I have a bit of confusion here. When the user injects a new key with
the same description and new passphrase, would that not replace the
existing user key with the old passphrase? Also, if I'm calling
lookup_user_key, where would the key_id come from for the old user key?
I suppose I can cache it.... Maybe I'm not quite understanding the exact
flow of how things you are suggesting.

> 
> Then you don't need to passphrases in the new key.
> 
>> +	rc = nvdimm_check_key_len(key->datalen, update);
> 
> payload->datalen.
> 
>> +	down_read(&key->sem);
> 
> This needs to be earlier.  The payload attached to the new key can be replaced
> by keyctl_update() at any time whilst you're not holding the lock, so you
> cannot use key->payload[*] without holding the lock or the RCU read lock.
> 
>> +	if (update)
>> +		key_invalidate(key);
> 
> The key doesn't belong to you - should you really be invalidating it?
> 
>> +	else {
>> +		key_link(nvdimm_keyring, key);
>> +		nvdimm->key = key;
>> +		key->perm |= KEY_USR_SEARCH;
>> +	}
> 
> Um - do you really want to be taking the key into your internal keyring?  Why
> aren't you calling nvdimm_replace_key()?  Also, you shouldn't alter the
> permission - it's not your key.
> 
>> +static int __parse_update(const char *buf, size_t len, unsigned int *old_id,
>> +		unsigned int *new_id)
>> +{
> 
> Try using sscanf()?
> 
> David
>
Dan Williams Sept. 22, 2018, 1:26 a.m. UTC | #3
On Fri, Sep 21, 2018 at 5:29 PM Dave Jiang <dave.jiang@intel.com> wrote:
[..]
> > What I meant here was to use, say, nvdimm_lookup_user_key() to get a key from
> > userspace that contains the old password.  You can use the description of the
> > key to search nvdimm_keyring for the private key and then compare the
> > passwords.
>
> Ok I have a bit of confusion here. When the user injects a new key with
> the same description and new passphrase, would that not replace the
> existing user key with the old passphrase? Also, if I'm calling
> lookup_user_key, where would the key_id come from for the old user key?
> I suppose I can cache it.... Maybe I'm not quite understanding the exact
> flow of how things you are suggesting.

If the DIMM is in the in the unlocked state the kernel should already
have the old passphrase cached, right? Could we just have the kernel
fail ->change_key() requests if the kernel does not already have a
valid old passphrase?

It does mean we can't support going from the locked state directly to
changing the password, but I would expect requiring an initial unlock
is fine.
diff mbox series

Patch

diff --git a/drivers/acpi/nfit/intel.c b/drivers/acpi/nfit/intel.c
index 4bfc1c1da339..314eae7e02d7 100644
--- a/drivers/acpi/nfit/intel.c
+++ b/drivers/acpi/nfit/intel.c
@@ -18,6 +18,73 @@ 
 #include "intel.h"
 #include "nfit.h"
 
+/*
+ * The update passphrase takes the old passphrase and the new passphrase
+ * and send those to the nvdimm. The nvdimm will verify the old
+ * passphrase and then update it with the new passphrase if pending
+ * verification. The function will pass in a zeroed passphrase field
+ * if the old passphrase is NULL. This typically happens when we are
+ * enabling security from the disabled state.
+ */
+static int intel_dimm_security_update_passphrase(
+		struct nvdimm_bus *nvdimm_bus, struct nvdimm *nvdimm,
+		const struct nvdimm_key_data *old_data,
+		const struct nvdimm_key_data *new_data)
+{
+	struct nvdimm_bus_descriptor *nd_desc = to_nd_desc(nvdimm_bus);
+	int cmd_rc, rc = 0;
+	struct nfit_mem *nfit_mem = nvdimm_provider_data(nvdimm);
+	struct {
+		struct nd_cmd_pkg pkg;
+		struct nd_intel_set_passphrase cmd;
+	} nd_cmd = {
+		.pkg = {
+			.nd_command = NVDIMM_INTEL_SET_PASSPHRASE,
+			.nd_family = NVDIMM_FAMILY_INTEL,
+			.nd_size_in = ND_INTEL_PASSPHRASE_SIZE * 2,
+			.nd_size_out = ND_INTEL_STATUS_SIZE,
+			.nd_fw_size = ND_INTEL_STATUS_SIZE,
+		},
+		.cmd = {
+			.status = 0,
+		},
+	};
+
+	if (!test_bit(NVDIMM_INTEL_SET_PASSPHRASE, &nfit_mem->dsm_mask))
+		return -ENOTTY;
+
+	if (old_data)
+		memcpy(nd_cmd.cmd.old_pass, old_data->data,
+				sizeof(nd_cmd.cmd.old_pass));
+	else
+		memset(nd_cmd.cmd.old_pass, 0, sizeof(nd_cmd.cmd.old_pass));
+	memcpy(nd_cmd.cmd.new_pass, new_data->data,
+			sizeof(nd_cmd.cmd.new_pass));
+	rc = nd_desc->ndctl(nd_desc, nvdimm, ND_CMD_CALL, &nd_cmd,
+			sizeof(nd_cmd), &cmd_rc);
+	if (rc < 0)
+		goto out;
+	if (cmd_rc < 0) {
+		rc = cmd_rc;
+		goto out;
+	}
+
+	switch (nd_cmd.cmd.status) {
+	case 0:
+		break;
+	case ND_INTEL_STATUS_INVALID_PASS:
+		rc = -EINVAL;
+		goto out;
+	case ND_INTEL_STATUS_INVALID_STATE:
+	default:
+		rc = -ENXIO;
+		goto out;
+	}
+
+ out:
+	return rc;
+}
+
 static int intel_dimm_security_unlock(struct nvdimm_bus *nvdimm_bus,
 		struct nvdimm *nvdimm, const struct nvdimm_key_data *nkey)
 {
@@ -149,4 +216,5 @@  static int intel_dimm_security_state(struct nvdimm_bus *nvdimm_bus,
 const struct nvdimm_security_ops intel_security_ops = {
 	.state = intel_dimm_security_state,
 	.unlock = intel_dimm_security_unlock,
+	.change_key = intel_dimm_security_update_passphrase,
 };
diff --git a/drivers/nvdimm/dimm_devs.c b/drivers/nvdimm/dimm_devs.c
index fe61f2a2ad5d..e7129998a5f3 100644
--- a/drivers/nvdimm/dimm_devs.c
+++ b/drivers/nvdimm/dimm_devs.c
@@ -20,6 +20,7 @@ 
 #include <linux/mm.h>
 #include <linux/key.h>
 #include <linux/init_task.h>
+#include <linux/key-type.h>
 #include <keys/user-type.h>
 #include "nd-core.h"
 #include "label.h"
@@ -51,6 +52,57 @@  static struct key *nvdimm_get_key(struct device *dev)
 	return nvdimm->key;
 }
 
+/*
+ * Replacing the user key with a kernel key
+ */
+static struct key *nvdimm_replace_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, &init_cred, 0,
+			KEY_ALLOC_NOT_IN_QUOTA, NULL);
+	if (IS_ERR(new_key))
+		return NULL;
+
+	down_read(&key->sem);
+	payload = key->payload.data[0];
+	rc = key_instantiate_and_link(new_key, payload->data, key->datalen,
+			nvdimm_keyring, NULL);
+	up_read(&key->sem);
+	if (rc < 0) {
+		key_revoke(new_key);
+		key_put(new_key);
+		return NULL;
+	}
+
+	key_invalidate(key);
+	key_put(key);
+
+	return new_key;
+}
+
+/*
+ * Retrieve user injected key
+ */
+static struct key *nvdimm_lookup_user_key(struct device *dev,
+		key_serial_t id)
+{
+	key_ref_t keyref;
+	struct key *key;
+
+	keyref = lookup_user_key(id, 0, 0);
+	if (IS_ERR(keyref))
+		return NULL;
+
+	key = key_ref_to_ptr(keyref);
+	dev_dbg(dev, "%s: key found: %d\n", __func__, key_serial(key));
+
+	return key;
+}
+
 /*
  * Retrieve kernel key for DIMM and request from user space if necessary.
  */
@@ -152,6 +204,96 @@  int nvdimm_security_unlock_dimm(struct device *dev)
 	return rc;
 }
 
+static int nvdimm_security_change_key(struct device *dev,
+		unsigned int old_keyid, unsigned int new_keyid)
+{
+	struct nvdimm *nvdimm = to_nvdimm(dev);
+	struct nvdimm_bus *nvdimm_bus = walk_to_nvdimm_bus(dev);
+	struct key *key = NULL, *old_key = NULL;
+	int rc;
+	void *old_data, *new_data;
+	bool update = false;
+	struct user_key_payload *payload;
+
+	if (!nvdimm->security_ops)
+		return 0;
+
+	if (nvdimm->state == NVDIMM_SECURITY_FROZEN)
+		return -EBUSY;
+
+	/* look for a key from keyring if exists and remove */
+	if (old_keyid != 0) {
+		old_key = nvdimm_get_key(dev);
+		if (old_key) {
+			if (key_serial(old_key) != old_keyid) {
+				dev_warn(dev, "incorrect key id: %u\n",
+						old_keyid);
+				return -EINVAL;
+			}
+			dev_dbg(dev, "%s: old key: %#x\n",
+					__func__, key_serial(old_key));
+			update = true;
+		}
+	}
+
+	/* request new key from userspace */
+	key = nvdimm_lookup_user_key(dev, new_keyid);
+	if (!key) {
+		dev_dbg(dev, "%s: failed to acquire new key\n", __func__);
+		rc = -ENXIO;
+		goto out;
+	}
+
+	dev_dbg(dev, "%s: new key: %#x\n", __func__, key_serial(key));
+
+	rc = nvdimm_check_key_len(key->datalen, update);
+	if (rc < 0)
+		goto out;
+
+	if (!update)
+		key = nvdimm_replace_key(key);
+
+	if (!key)
+		goto out;
+
+	payload = key->payload.data[0];
+	if (!update) {
+		old_data = NULL;
+		new_data = payload->data;
+	} else {
+		new_data = payload->data;
+		old_data = new_data + NVDIMM_PASSPHRASE_LEN;
+	}
+
+	down_read(&key->sem);
+	rc = nvdimm->security_ops->change_key(nvdimm_bus, nvdimm, old_data,
+			new_data);
+	/* copy new payload to old payload */
+	if (rc == 0) {
+		if (update)
+			key_update(make_key_ref(old_key, 1), new_data,
+					old_key->datalen);
+	} else
+		dev_warn(dev, "key update failed\n");
+	up_read(&key->sem);
+
+	if (update)
+		key_invalidate(key);
+	else {
+		key_link(nvdimm_keyring, key);
+		nvdimm->key = key;
+		key->perm |= KEY_USR_SEARCH;
+	}
+	nvdimm_security_get_state(dev);
+
+ out:
+	if (old_key)
+		key_put(old_key);
+	if (key)
+		key_put(key);
+	return rc;
+}
+
 /*
  * Retrieve bus and dimm handle and return if this bus supports
  * get_config_data commands
@@ -509,11 +651,125 @@  static ssize_t available_slots_show(struct device *dev,
 }
 static DEVICE_ATTR_RO(available_slots);
 
+static ssize_t security_show(struct device *dev,
+		struct device_attribute *attr, char *buf)
+{
+	struct nvdimm *nvdimm = to_nvdimm(dev);
+
+	switch (nvdimm->state) {
+	case NVDIMM_SECURITY_DISABLED:
+		return sprintf(buf, "disabled\n");
+	case NVDIMM_SECURITY_UNLOCKED:
+		return sprintf(buf, "unlocked\n");
+	case NVDIMM_SECURITY_LOCKED:
+		return sprintf(buf, "locked\n");
+	case NVDIMM_SECURITY_FROZEN:
+		return sprintf(buf, "frozen\n");
+	case NVDIMM_SECURITY_UNSUPPORTED:
+	default:
+		return sprintf(buf, "unsupported\n");
+	}
+
+	return -ENOTTY;
+}
+
+/*
+ * The update command:
+ *	update:<old_id>:<new_id>
+ */
+#define STR_UPDATE_LEN		6
+/* ":0:0" */
+#define NVDIMM_UPDATE_MIN_LEN	STR_UPDATE_LEN + 4
+
+static int __parse_update(const char *buf, size_t len, unsigned int *old_id,
+		unsigned int *new_id)
+{
+	int i, rc = 0;
+	const char *tmp, *old_idstr;
+	char *desc;
+	size_t buf_len;
+
+	if (len < NVDIMM_UPDATE_MIN_LEN)
+		return -EINVAL;
+
+	desc = kmemdup_nul(buf, STR_UPDATE_LEN, GFP_KERNEL);
+	if (!desc)
+		return -EINVAL;
+
+	if (strcmp(desc, "update") != 0) {
+		rc = -ENOMEM;
+		goto free_desc;
+	}
+
+	tmp = &buf[STR_UPDATE_LEN];
+	if (*tmp != ':') {
+		rc = -EINVAL;
+		goto free_desc;
+	}
+
+	tmp++;
+
+	buf_len = len - STR_UPDATE_LEN - 1;
+	for (i = 0; i < buf_len; i++) {
+		if (tmp[i] == ':')
+			break;
+	}
+
+	old_idstr = kmemdup_nul(tmp, i, GFP_KERNEL);
+	if (!old_idstr) {
+		rc = -ENOMEM;
+		goto free_desc;
+	}
+
+	rc = kstrtouint(old_idstr, 0, old_id);
+	if (rc < 0)
+		goto free_old;
+
+	tmp += i + 1;
+
+	rc = kstrtouint(tmp, 0, new_id);
+	if (rc < 0)
+		goto free_old;
+
+free_old:
+	kfree(old_idstr);
+free_desc:
+	kfree(desc);
+	return rc;
+}
+
+static ssize_t security_store(struct device *dev,
+		struct device_attribute *attr, const char *buf, size_t len)
+
+{
+	struct nvdimm *nvdimm = to_nvdimm(dev);
+	struct nvdimm_bus *nvdimm_bus = walk_to_nvdimm_bus(dev);
+	ssize_t rc = -EINVAL;
+	unsigned int new_id, old_id;
+
+        wait_nvdimm_bus_probe_idle(&nvdimm_bus->dev);
+        if (atomic_read(&nvdimm->busy))
+                return -EBUSY;
+
+	if (__parse_update(buf, len, &old_id, &new_id) == 0) {
+		dev_dbg(dev, "update: old: %u new: %u\n", old_id, new_id);
+		rc = nvdimm_security_change_key(dev, old_id, new_id);
+	} else
+		return -EINVAL;
+
+	if (rc == 0)
+		rc = len;
+
+	return rc;
+}
+static DEVICE_ATTR_RW(security);
+
 static struct attribute *nvdimm_attributes[] = {
 	&dev_attr_state.attr,
 	&dev_attr_flags.attr,
 	&dev_attr_commands.attr,
 	&dev_attr_available_slots.attr,
+	&dev_attr_security.attr,
 	NULL,
 };
 
diff --git a/include/linux/libnvdimm.h b/include/linux/libnvdimm.h
index 257ff2637ce1..bd6a413164ee 100644
--- a/include/linux/libnvdimm.h
+++ b/include/linux/libnvdimm.h
@@ -160,6 +160,7 @@  enum nvdimm_security_state {
 	NVDIMM_SECURITY_DISABLED,
 	NVDIMM_SECURITY_UNLOCKED,
 	NVDIMM_SECURITY_LOCKED,
+	NVDIMM_SECURITY_FROZEN,
 	NVDIMM_SECURITY_UNSUPPORTED,
 };
 
@@ -177,6 +178,10 @@  struct nvdimm_security_ops {
 	int (*unlock)(struct nvdimm_bus *nvdimm_bus,
 			struct nvdimm *nvdimm,
 			const struct nvdimm_key_data *nkey);
+	int (*change_key)(struct nvdimm_bus *nvdimm_bus,
+			struct nvdimm *nvdimm,
+			const struct nvdimm_key_data *old_data,
+			const struct nvdimm_key_data *new_data);
 };
 
 void badrange_init(struct badrange *badrange);