On Mon, 2018-10-08 at 16:55 -0700, Dave Jiang wrote:
> The following series implements security support for nvdimm. Mostly
> adding
> new security DSM support from the Intel NVDIMM DSM spec v1.7, but
> also
> adding generic support libnvdimm for other vendors. The most
> important
> security features are unlocking locked nvdimms, and updating/setting
> security
> passphrase to nvdimms.
>
> v12:
> - Add a mutex for the cached key and remove key_get/key_put messiness
> (Dan)
> - Move security code to its own C file and wrap under
> CONFIG_NVDIMM_SECURITY
> in order to fix issue reported by 0-day build without CONFIG_KEYS.
Going over this a bit more closely here is some proposed cleanups /
fixes:
* remove nvdimm_get_key() and just rely on nvdimm->key being not NULL
under the key_mutex
* open code nvdimm_check_key_len() checks
* make all the security op function signatures take an nvdimm rather
than a dev
* move the release of nvdimm->key to nvdimm_remove() rather than
nvdimm_release(), i.e. driver unload vs nvdimm_bus de-registration.
* rename nvdimm_replace_key to make_kernel_key
* clean up nvdimm_security_change_key() to a be bit more readable and
fix a missing key_put()
Let me know if these look acceptable and I can fold them into the patch
set.
diff --git a/drivers/nvdimm/dimm.c b/drivers/nvdimm/dimm.c
index b6381ddbd6c1..429c544e7ac5 100644
--- a/drivers/nvdimm/dimm.c
+++ b/drivers/nvdimm/dimm.c
@@ -23,6 +23,7 @@
static int nvdimm_probe(struct device *dev)
{
+ struct nvdimm *nvdimm = to_nvdimm(dev);
struct nvdimm_drvdata *ndd;
int rc;
@@ -51,10 +52,10 @@ static int nvdimm_probe(struct device *dev)
get_device(dev);
kref_init(&ndd->kref);
- nvdimm_security_get_state(dev);
+ nvdimm_security_get_state(nvdimm);
/* unlock DIMM here before touch label */
- rc = nvdimm_security_unlock_dimm(dev);
+ rc = nvdimm_security_unlock_dimm(nvdimm);
if (rc < 0)
dev_warn(dev, "failed to unlock dimm %s\n", dev_name(dev));
@@ -115,6 +116,9 @@ 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 2d9915378bbc..84bec3bb025e 100644
--- a/drivers/nvdimm/dimm_devs.c
+++ b/drivers/nvdimm/dimm_devs.c
@@ -213,13 +213,7 @@ void nvdimm_clear_locked(struct device *dev)
static void nvdimm_release(struct device *dev)
{
struct nvdimm *nvdimm = to_nvdimm(dev);
- struct key *key;
- mutex_lock(&nvdimm->key_mutex);
- key = nvdimm_get_key(dev);
- if (key)
- key_put(key);
- mutex_unlock(&nvdimm->key_mutex);
ida_simple_remove(&dimm_ida, nvdimm->id);
kfree(nvdimm);
}
@@ -436,20 +430,20 @@ static ssize_t security_store(struct device *dev,
if (rc != 3)
return -EINVAL;
dev_dbg(dev, "update %#x %#x\n", old_key, new_key);
- rc = nvdimm_security_change_key(dev, old_key, new_key);
+ rc = nvdimm_security_change_key(nvdimm, old_key, new_key);
} else if (sysfs_streq(cmd, "disable")) {
if (rc != 2)
return -EINVAL;
dev_dbg(dev, "disable %#x\n", old_key);
- rc = nvdimm_security_disable(dev, old_key);
+ rc = nvdimm_security_disable(nvdimm, old_key);
} else if (sysfs_streq(buf, "freeze")) {
dev_dbg(dev, "freeze\n");
- rc = nvdimm_security_freeze_lock(dev);
+ rc = nvdimm_security_freeze_lock(nvdimm);
} else if (sysfs_streq(cmd, "erase")) {
if (rc != 2)
return -EINVAL;
dev_dbg(dev, "erase %#x\n", old_key);
- rc = nvdimm_security_erase(dev, old_key);
+ rc = nvdimm_security_erase(nvdimm, old_key);
} else
return -EINVAL;
diff --git a/drivers/nvdimm/nd.h b/drivers/nvdimm/nd.h
index 08e442632a2d..45fcaf45ba2c 100644
--- a/drivers/nvdimm/nd.h
+++ b/drivers/nvdimm/nd.h
@@ -425,48 +425,47 @@ const u8 *nd_dev_to_uuid(struct device *dev);
bool pmem_should_map_pages(struct device *dev);
#ifdef CONFIG_NVDIMM_SECURITY
-struct key *nvdimm_get_key(struct device *dev);
-int nvdimm_security_unlock_dimm(struct device *dev);
-int nvdimm_security_get_state(struct device *dev);
-int nvdimm_security_change_key(struct device *dev, unsigned int old_keyid,
+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);
-int nvdimm_security_disable(struct device *dev, unsigned int keyid);
-int nvdimm_security_freeze_lock(struct device *dev);
-int nvdimm_security_erase(struct device *dev, unsigned int keyid);
+int nvdimm_security_disable(struct nvdimm *nvdimm, unsigned int keyid);
+int nvdimm_security_freeze_lock(struct nvdimm *nvdimm);
+int nvdimm_security_erase(struct nvdimm *nvdimm, unsigned int keyid);
#else
-static inline struct key *nvdimm_get_key(struct device *dev)
+static inline int nvdimm_security_unlock_dimm(struct nvdimm *nvdimm)
{
- return NULL;
+ return 0;
}
-static inline int nvdimm_security_unlock_dimm(struct device *dev)
+static inline void nvdimm_security_release(struct nvdimm *nvdimm)
{
- return 0;
}
-static inline int nvdimm_security_get_state(struct device *dev)
+static inline int nvdimm_security_get_state(struct nvdimm *nvdimm)
{
return -EOPNOTSUPP;
}
-static inline int nvdimm_security_change_key(struct device *dev,
+static inline int nvdimm_security_change_key(struct nvdimm *nvdimm,
unsigned int old_keyid, unsigned int new_keyid)
{
return -EOPNOTSUPP;
}
-static inline int nvdimm_security_disable(struct device *dev,
+static inline int nvdimm_security_disable(struct nvdimm *nvdimm,
unsigned int keyid)
{
return -EOPNOTSUPP;
}
-static inline int nvdimm_security_freeze_lock(struct device *dev)
+static inline int nvdimm_security_freeze_lock(struct nvdimm *nvdimm)
{
return -EOPNOTSUPP;
}
-static inline int nvdimm_security_erase(struct device *dev,
+static inline int nvdimm_security_erase(struct nvdimm *nvdimm,
unsigned int keyid)
{
return -EOPNOTSUPP;
diff --git a/drivers/nvdimm/security.c b/drivers/nvdimm/security.c
index 9f468f91263d..776c440a02ef 100644
--- a/drivers/nvdimm/security.c
+++ b/drivers/nvdimm/security.c
@@ -13,31 +13,13 @@
#include "nd-core.h"
#include "nd.h"
-/*
- * Find key that's cached with nvdimm.
- */
-struct key *nvdimm_get_key(struct device *dev)
-{
- struct nvdimm *nvdimm = to_nvdimm(dev);
-
- if (!nvdimm->key)
- return NULL;
-
- if (key_validate(nvdimm->key) < 0)
- return NULL;
-
- dev_dbg(dev, "%s: key found: %#x\n", __func__,
- key_serial(nvdimm->key));
- return nvdimm->key;
-}
-
/*
* 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 *nvdimm_replace_key(struct key *key)
+static struct key *make_kernel_key(struct key *key)
{
struct key *new_key;
struct user_key_payload *payload;
@@ -86,9 +68,8 @@ static struct key *nvdimm_lookup_user_key(struct device *dev,
/*
* Retrieve kernel key for DIMM and request from user space if necessary.
*/
-static struct key *nvdimm_request_key(struct device *dev)
+static struct key *nvdimm_request_key(struct nvdimm *nvdimm)
{
- struct nvdimm *nvdimm = to_nvdimm(dev);
struct key *key = NULL;
char desc[NVDIMM_KEY_DESC_LEN + sizeof(NVDIMM_PREFIX)];
@@ -100,34 +81,26 @@ static struct key *nvdimm_request_key(struct device *dev)
return key;
}
-static int nvdimm_check_key_len(unsigned short len)
-{
- if (len == NVDIMM_PASSPHRASE_LEN)
- return 0;
-
- return -EINVAL;
-}
-
-struct key *nvdimm_get_and_verify_key(struct device *dev,
+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;
- int rc = 0;
- key = nvdimm_get_key(dev);
- /* we don't have a cached key */
+ lockdep_assert_held(&nvdimm->key_mutex);
+ key = nvdimm->key;
if (!key) {
dev_dbg(dev, "No cached kernel key\n");
- return NULL;
+ 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");
- rc = -EINVAL;
- goto out;
+ return ERR_PTR(-EINVAL);
}
dev_dbg(dev, "user_key: %#x\n", key_serial(user_key));
@@ -136,45 +109,38 @@ struct key *nvdimm_get_and_verify_key(struct device *dev,
payload = key->payload.data[0];
upayload = user_key->payload.data[0];
- if (memcmp(payload->data, upayload->data,
- NVDIMM_PASSPHRASE_LEN) != 0) {
- rc = -EINVAL;
- dev_warn(dev, "Supplied old user key fails check.\n");
- }
-
+ rc = memcmp(payload->data, upayload->data, NVDIMM_PASSPHRASE_LEN);
up_read(&user_key->sem);
key_put(user_key);
up_read(&key->sem);
-out:
- if (rc < 0)
- key = ERR_PTR(rc);
+ if (rc != 0) {
+ dev_warn(dev, "Supplied old user key fails check.\n");
+ return ERR_PTR(-EINVAL);
+ }
return key;
}
-int nvdimm_security_get_state(struct device *dev)
+int nvdimm_security_get_state(struct nvdimm *nvdimm)
{
- struct nvdimm *nvdimm = to_nvdimm(dev);
-
if (!nvdimm->security_ops)
return 0;
return nvdimm->security_ops->state(nvdimm, &nvdimm->state);
}
-int nvdimm_security_erase(struct device *dev, unsigned int keyid)
+int nvdimm_security_erase(struct nvdimm *nvdimm, unsigned int keyid)
{
- struct nvdimm *nvdimm = to_nvdimm(dev);
- struct nvdimm_bus *nvdimm_bus = walk_to_nvdimm_bus(dev);
+ int rc = 0;
struct key *key;
struct user_key_payload *payload;
- int rc = 0;
+ struct device *dev = &nvdimm->dev;
bool is_userkey = false;
if (!nvdimm->security_ops)
return -EOPNOTSUPP;
- nvdimm_bus_lock(&nvdimm_bus->dev);
+ nvdimm_bus_lock(dev);
mutex_lock(&nvdimm->key_mutex);
if (atomic_read(&nvdimm->busy)) {
dev_warn(dev, "Unable to secure erase while DIMM active.\n");
@@ -195,7 +161,7 @@ int nvdimm_security_erase(struct device *dev, unsigned int
keyid)
}
/* look for a key from cached key if exists */
- key = nvdimm_get_and_verify_key(dev, keyid);
+ key = nvdimm_get_and_verify_key(nvdimm, keyid);
if (IS_ERR(key)) {
dev_dbg(dev, "Unable to get and verify key\n");
rc = PTR_ERR(key);
@@ -229,14 +195,13 @@ int nvdimm_security_erase(struct device *dev, unsigned
int keyid)
out:
mutex_unlock(&nvdimm->key_mutex);
- nvdimm_bus_unlock(&nvdimm_bus->dev);
- nvdimm_security_get_state(dev);
+ nvdimm_bus_unlock(dev);
+ nvdimm_security_get_state(nvdimm);
return rc;
}
-int nvdimm_security_freeze_lock(struct device *dev)
+int nvdimm_security_freeze_lock(struct nvdimm *nvdimm)
{
- struct nvdimm *nvdimm = to_nvdimm(dev);
int rc;
if (!nvdimm->security_ops)
@@ -249,16 +214,16 @@ int nvdimm_security_freeze_lock(struct device *dev)
if (rc < 0)
return rc;
- nvdimm_security_get_state(dev);
+ nvdimm_security_get_state(nvdimm);
return 0;
}
-int nvdimm_security_disable(struct device *dev, unsigned int keyid)
+int nvdimm_security_disable(struct nvdimm *nvdimm, unsigned int keyid)
{
- struct nvdimm *nvdimm = to_nvdimm(dev);
- struct key *key;
int rc;
+ struct key *key;
struct user_key_payload *payload;
+ struct device *dev = &nvdimm->dev;
bool is_userkey = false;
if (!nvdimm->security_ops)
@@ -269,7 +234,7 @@ int nvdimm_security_disable(struct device *dev, unsigned
int keyid)
mutex_lock(&nvdimm->key_mutex);
/* look for a key from cached key */
- key = nvdimm_get_and_verify_key(dev, keyid);
+ key = nvdimm_get_and_verify_key(nvdimm, keyid);
if (IS_ERR(key)) {
mutex_unlock(&nvdimm->key_mutex);
return PTR_ERR(key);
@@ -304,17 +269,16 @@ int nvdimm_security_disable(struct device *dev, unsigned
int keyid)
out:
mutex_unlock(&nvdimm->key_mutex);
- nvdimm_security_get_state(dev);
+ nvdimm_security_get_state(nvdimm);
return rc;
}
-int nvdimm_security_unlock_dimm(struct device *dev)
+int nvdimm_security_unlock_dimm(struct nvdimm *nvdimm)
{
- struct nvdimm *nvdimm = to_nvdimm(dev);
struct key *key;
+ int rc = -ENXIO;
struct user_key_payload *payload;
- int rc;
- bool cached_key = false;
+ struct device *dev = &nvdimm->dev;
if (!nvdimm->security_ops)
return 0;
@@ -325,24 +289,19 @@ int nvdimm_security_unlock_dimm(struct device *dev)
return 0;
mutex_lock(&nvdimm->key_mutex);
- key = nvdimm_get_key(dev);
- if (!key)
- key = nvdimm_request_key(dev);
- else
- cached_key = true;
+ key = nvdimm->key;
if (!key) {
- mutex_unlock(&nvdimm->key_mutex);
- return -ENXIO;
- }
-
- if (!cached_key) {
- rc = nvdimm_check_key_len(key->datalen);
- if (rc < 0) {
+ key = nvdimm_request_key(nvdimm);
+ if (key && key->datalen != NVDIMM_PASSPHRASE_LEN) {
key_put(key);
- mutex_unlock(&nvdimm->key_mutex);
- return rc;
+ key = NULL;
+ rc = -EINVAL;
}
}
+ if (!key) {
+ mutex_unlock(&nvdimm->key_mutex);
+ return rc;
+ }
dev_dbg(dev, "%s: key: %#x\n", __func__, key_serial(key));
down_read(&key->sem);
@@ -352,7 +311,7 @@ int nvdimm_security_unlock_dimm(struct device *dev)
up_read(&key->sem);
if (rc == 0) {
- if (!cached_key)
+ if (!nvdimm->key)
nvdimm->key = key;
nvdimm->state = NVDIMM_SECURITY_UNLOCKED;
dev_dbg(dev, "DIMM %s unlocked\n", dev_name(dev));
@@ -364,18 +323,31 @@ int nvdimm_security_unlock_dimm(struct device *dev)
}
mutex_unlock(&nvdimm->key_mutex);
- nvdimm_security_get_state(dev);
+ nvdimm_security_get_state(nvdimm);
return rc;
}
-int nvdimm_security_change_key(struct device *dev,
+void nvdimm_security_release(struct nvdimm *nvdimm)
+{
+ mutex_lock(&nvdimm->key_mutex);
+ key_put(nvdimm->key);
+ nvdimm->key = NULL;
+ mutex_unlock(&nvdimm->key_mutex);
+}
+
+static void key_destroy(struct key *key)
+{
+ key_invalidate(key);
+ key_put(key);
+}
+
+int nvdimm_security_change_key(struct nvdimm *nvdimm,
unsigned int old_keyid, unsigned int new_keyid)
{
- struct nvdimm *nvdimm = to_nvdimm(dev);
- struct key *key, *old_key;
int rc;
+ struct key *key, *old_key;
void *old_data = NULL, *new_data;
- bool update = false;
+ struct device *dev = &nvdimm->dev;
struct user_key_payload *payload, *old_payload;
if (!nvdimm->security_ops)
@@ -386,16 +358,15 @@ int nvdimm_security_change_key(struct device *dev,
mutex_lock(&nvdimm->key_mutex);
/* look for a key from cached key if exists */
- old_key = nvdimm_get_and_verify_key(dev, old_keyid);
- if (IS_ERR(old_key)) {
+ 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);
- }
- if (old_key) {
- dev_dbg(dev, "%s: old key: %#x\n",
- __func__, key_serial(old_key));
- update = true;
- }
+ } else
+ dev_dbg(dev, "%s: old key: %#x\n", __func__,
+ key_serial(old_key));
/* request new key from userspace */
key = nvdimm_lookup_user_key(dev, new_keyid);
@@ -409,22 +380,29 @@ int nvdimm_security_change_key(struct device *dev,
down_read(&key->sem);
payload = key->payload.data[0];
- rc = nvdimm_check_key_len(payload->datalen);
- if (rc < 0) {
+ if (payload->datalen != NVDIMM_PASSPHRASE_LEN) {
+ rc = -EINVAL;
up_read(&key->sem);
goto out;
}
- if (!update)
- key = nvdimm_replace_key(key);
+ /*
+ * 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
- * nvdimm_replace_key() will.
+ * make_kernel_key() will have upgraded the user key to kernel
+ * and handled the semaphore handoff.
*/
- if (!key)
- goto out;
-
payload = key->payload.data[0];
if (old_key) {
@@ -437,25 +415,26 @@ int nvdimm_security_change_key(struct device *dev,
rc = nvdimm->security_ops->change_key(nvdimm, old_data,
new_data);
- /* copy new payload to old payload */
- if (rc == 0) {
- if (update)
+ if (rc)
+ dev_warn(dev, "key update failed: %d\n", rc);
+
+ if (old_key) {
+ /* copy new payload to old payload */
+ if (rc == 0)
key_update(make_key_ref(old_key, 1), new_data,
old_key->datalen);
- } else
- dev_warn(dev, "key update failed\n");
- if (old_key)
up_read(&old_key->sem);
+ }
up_read(&key->sem);
- if (!update) {
+ if (!old_key) {
if (rc == 0) {
dev_dbg(dev, "key cached: %#x\n", key_serial(key));
nvdimm->key = key;
} else
- key_invalidate(key);
+ key_destroy(key);
}
- nvdimm_security_get_state(dev);
+ nvdimm_security_get_state(nvdimm);
out:
mutex_unlock(&nvdimm->key_mutex);
_______________________________________________
Linux-nvdimm mailing list
[email protected]
https://lists.01.org/mailman/listinfo/linux-nvdimm
