| Message ID | 153904272246.60070.6230977215877367778.stgit@djiang5-desk3.ch.intel.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | Adding security support for nvdimm | expand |
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);
On 10/09/2018 06:35 PM, Williams, Dan J wrote: > 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. Looks good. Thanks! > > 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); >
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. v11: - Dropped keyring usage. (David) - Fixed up scanf handling. (David) - Removed callout info for request_key(). (David) - Included Dan's patches and folded in some changes from Dan. (Dan) - Made security_show a weak function to allow test override. (Dan) v10: - Change usage of strcmp to sysfs_streq. (Dan) - Lock nvdimm bus when doing secure erase. (Dan) - Change dev_info to dev_dbg for dimm unlocked success output. (Dan) v9: - Addressed various misc comments. (David, Dan) - Removed init_cred and replaced with current_cred(). (David) - Changed NVDIMM_PREFIX to char[] constant (David) - Moved NVDIMM_PREFIX to include/uapi/linux/ndctl.h (Dan) - Reworked security_update to use old user key to verify against kernel key and then update with new user key. (David) - Added requirement of disable and erase to require old user key for verify. (Dan) - Updated documentation. (Dave) v8: - Make the keys retained by the kernel user searchable in order to find the key that needs to be updated for key update. v7: - Add CONFIG_KEYS depenency for libnvdimm. (Alison) - Export lookup_user_key(). (David) - Modified "update" to take two key ids and and use lookup_user_key() in order to improve security. (David) - Use key ptrs and key_validate() for cached keys. (David) v6: - Fix intel DSM data structures to use defined size for passphrase (Robert) - Fix memcpy size to use sizeof data structure member (Robert) - Fix defined dimm id length (Robert) - Making intel_security_ops const (Eric) - Remove unused var in nvdimm_key_search() (Eric) - Added wbinvd before secure erase is issued (Robert) - Removed key_put_sync() usage (David) - Use init_cred instead of creating own cred (David) - Exported init_cred symbol - Move keyring to dedicated (David) - Use logon_key_type and friends instead of creating custom (David) - Use key_lookup() with stored key serial (David) - Exported key_lookup() symbol - Mark passed in key data as const (David) - Added comment for change_pass_phrase to explain how it works (David) - Unlink key when it's being removed from keyring. (David) - Removed request_key() from all security ops except update and unlock. - Update will now update the existing key's payload with the new key's retrieved from userspace when the new payload is accepted by nvdimm. v5: - Moved dimm_id initialization (Dan) - Added a key_put_sync() in order to run key_gc_work and cleanup old key. (Dan) - Added check to block security state changes while DIMM is active. (Dan) v4: - flip payload layout for update passphrase to make it easier on userland. v3: - Set x86 wrappers for x86 only bits. (Dan) - Fixed up some verbiage in commit headers. - Put in usage of sysfs_streq() for sysfs inputs. - 0-day build fixes for non-x86 archs. v2: - Move inclusion of intel.h to relevant source files and not in nfit.h. (Dan) - Moved security ring relevant code to dimm_devs.c. (Dan) - Added dimm_id to nfit_mem to avoid recreate per sysfs show call. (Dan) - Added routine to return security_ops based on family supplied. (Dan) - Added nvdimm_key_data struct to wrap raw passphrase string. (Dan) - Allocate firmware package on stack. (Dan) - Added missing frozen state detection when retrieving security state. --- Dan Williams (2): libnvdimm: Drop nvdimm_bus from security_ops interface acpi, nfit: Move acpi_nfit_get_security_ops() to generic location Dave Jiang (10): nfit: add support for Intel DSM 1.7 commands nfit/libnvdimm: store dimm id as a member to struct nvdimm keys: export lookup_user_key to external users nfit/libnvdimm: add unlock of nvdimm support for Intel DIMMs nfit/libnvdimm: add set passphrase support for Intel nvdimms nfit/libnvdimm: add disable passphrase support to Intel nvdimm. nfit/libnvdimm: add freeze security support to Intel nvdimm nfit/libnvdimm: add support for issue secure erase DSM to Intel nvdimm nfit_test: add test support for Intel nvdimm security DSMs libnvdimm: add documentation for nvdimm security support Documentation/nvdimm/security.txt | 99 +++++++ drivers/acpi/nfit/Makefile | 1 drivers/acpi/nfit/core.c | 68 ++++- drivers/acpi/nfit/intel.c | 369 ++++++++++++++++++++++++++++ drivers/acpi/nfit/intel.h | 70 +++++ drivers/acpi/nfit/nfit.h | 20 +- drivers/nvdimm/Kconfig | 5 drivers/nvdimm/Makefile | 1 drivers/nvdimm/bus.c | 8 + drivers/nvdimm/dimm.c | 7 + drivers/nvdimm/dimm_devs.c | 89 +++++++ drivers/nvdimm/dimm_devs_security.c | 463 +++++++++++++++++++++++++++++++++++ drivers/nvdimm/nd-core.h | 6 drivers/nvdimm/nd.h | 51 ++++ include/linux/key.h | 3 include/linux/libnvdimm.h | 46 +++ include/uapi/linux/ndctl.h | 6 security/keys/internal.h | 2 security/keys/process_keys.c | 1 tools/testing/nvdimm/Kbuild | 3 tools/testing/nvdimm/dimm_devs.c | 39 +++ tools/testing/nvdimm/test/nfit.c | 185 ++++++++++++++ 22 files changed, 1522 insertions(+), 20 deletions(-) create mode 100644 Documentation/nvdimm/security.txt create mode 100644 drivers/acpi/nfit/intel.c create mode 100644 drivers/acpi/nfit/intel.h create mode 100644 drivers/nvdimm/dimm_devs_security.c create mode 100644 tools/testing/nvdimm/dimm_devs.c --