From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga18.intel.com (mga18.intel.com [134.134.136.126]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id A01E321A07A82 for ; Tue, 9 Oct 2018 18:35:29 -0700 (PDT) From: "Williams, Dan J" Subject: Re: [PATCH v12 00/12] Adding security support for nvdimm Date: Wed, 10 Oct 2018 01:35:27 +0000 Message-ID: References: <153904272246.60070.6230977215877367778.stgit@djiang5-desk3.ch.intel.com> In-Reply-To: <153904272246.60070.6230977215877367778.stgit@djiang5-desk3.ch.intel.com> Content-Language: en-US Content-ID: <774DA16502DCB4498ED5E43A1F4AB72F@intel.com> MIME-Version: 1.0 List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Errors-To: linux-nvdimm-bounces@lists.01.org Sender: "Linux-nvdimm" To: "Jiang, Dave" Cc: "Schofield, Alison" , "keescook@chromium.org" , "linux-nvdimm@lists.01.org" , "ebiggers3@gmail.com" , "dhowells@redhat.com" , "keyrings@vger.kernel.org" List-ID: 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 Linux-nvdimm@lists.01.org https://lists.01.org/mailman/listinfo/linux-nvdimm