nvdimm.lists.linux.dev archive mirror
 help / color / mirror / Atom feed
* [PATCH v12 00/12] Adding security support for nvdimm
@ 2018-10-08 23:55 Dave Jiang
  2018-10-08 23:55 ` [PATCH v12 01/12] nfit: add support for Intel DSM 1.7 commands Dave Jiang
                   ` (12 more replies)
  0 siblings, 13 replies; 17+ messages in thread
From: Dave Jiang @ 2018-10-08 23:55 UTC (permalink / raw)
  To: dan.j.williams
  Cc: alison.schofield, keescook, linux-nvdimm, ebiggers3, dhowells, keyrings

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

--
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

^ permalink raw reply	[flat|nested] 17+ messages in thread

* [PATCH v12 01/12] nfit: add support for Intel DSM 1.7 commands
  2018-10-08 23:55 [PATCH v12 00/12] Adding security support for nvdimm Dave Jiang
@ 2018-10-08 23:55 ` Dave Jiang
  2018-10-08 23:55 ` [PATCH v12 02/12] nfit/libnvdimm: store dimm id as a member to struct nvdimm Dave Jiang
                   ` (11 subsequent siblings)
  12 siblings, 0 replies; 17+ messages in thread
From: Dave Jiang @ 2018-10-08 23:55 UTC (permalink / raw)
  To: dan.j.williams
  Cc: alison.schofield, keescook, linux-nvdimm, ebiggers3, dhowells, keyrings

Add command definition for security commands defined in Intel DSM
specification v1.7. This includes "get security state", "set passphrase",
"unlock unit", "freeze lock", "secure erase", "ovewrite", and
"overwrite query". Since we are adding a lot of Intel definitions, moving
the relevant bits to its own header. Also, we don't want those commands
to be issued from user space through ioctls. Reason being some of the
security commands require kernel involvement to flush all CPU caches that
can't be done in userspace and the result can cause system crash. So
blocking security commands in the ioctl path.

Signed-off-by: Dave Jiang <dave.jiang@intel.com>
Reviewed-by: Dan Williams <dan.j.williams@intel.com>
Signed-off-by: Dan Williams <dan.j.williams@intel.com>
---
 drivers/acpi/nfit/core.c  |   28 ++++++++++++++++++-
 drivers/acpi/nfit/intel.h |   67 +++++++++++++++++++++++++++++++++++++++++++++
 drivers/acpi/nfit/nfit.h  |   17 +++++++++++
 drivers/nvdimm/bus.c      |    2 +
 include/linux/libnvdimm.h |    2 +
 5 files changed, 112 insertions(+), 4 deletions(-)
 create mode 100644 drivers/acpi/nfit/intel.h

diff --git a/drivers/acpi/nfit/core.c b/drivers/acpi/nfit/core.c
index b072cfc5f20e..4f3b89dc9238 100644
--- a/drivers/acpi/nfit/core.c
+++ b/drivers/acpi/nfit/core.c
@@ -24,6 +24,7 @@
 #include <linux/nd.h>
 #include <asm/cacheflush.h>
 #include <acpi/nfit.h>
+#include "intel.h"
 #include "nfit.h"
 
 /*
@@ -377,6 +378,14 @@ static u8 nfit_dsm_revid(unsigned family, unsigned func)
 			[NVDIMM_INTEL_QUERY_FWUPDATE] = 2,
 			[NVDIMM_INTEL_SET_THRESHOLD] = 2,
 			[NVDIMM_INTEL_INJECT_ERROR] = 2,
+			[NVDIMM_INTEL_GET_SECURITY_STATE] = 2,
+			[NVDIMM_INTEL_SET_PASSPHRASE] = 2,
+			[NVDIMM_INTEL_DISABLE_PASSPHRASE] = 2,
+			[NVDIMM_INTEL_UNLOCK_UNIT] = 2,
+			[NVDIMM_INTEL_FREEZE_LOCK] = 2,
+			[NVDIMM_INTEL_SECURE_ERASE] = 2,
+			[NVDIMM_INTEL_OVERWRITE] = 2,
+			[NVDIMM_INTEL_QUERY_OVERWRITE] = 2,
 		},
 	};
 	u8 id;
@@ -3229,7 +3238,7 @@ static int acpi_nfit_flush_probe(struct nvdimm_bus_descriptor *nd_desc)
 	return 0;
 }
 
-static int acpi_nfit_clear_to_send(struct nvdimm_bus_descriptor *nd_desc,
+static int __acpi_nfit_clear_to_send(struct nvdimm_bus_descriptor *nd_desc,
 		struct nvdimm *nvdimm, unsigned int cmd)
 {
 	struct acpi_nfit_desc *acpi_desc = to_acpi_nfit_desc(nd_desc);
@@ -3251,6 +3260,23 @@ static int acpi_nfit_clear_to_send(struct nvdimm_bus_descriptor *nd_desc,
 	return 0;
 }
 
+/* prevent security commands from being issued via ioctl */
+static int acpi_nfit_clear_to_send(struct nvdimm_bus_descriptor *nd_desc,
+		struct nvdimm *nvdimm, unsigned int cmd, void *buf)
+{
+	struct nd_cmd_pkg *call_pkg = buf;
+	unsigned int func;
+
+	if (nvdimm && cmd == ND_CMD_CALL &&
+			call_pkg->nd_family == NVDIMM_FAMILY_INTEL) {
+		func = call_pkg->nd_command;
+		if ((1 << func) & NVDIMM_INTEL_SECURITY_CMDMASK)
+			return -EOPNOTSUPP;
+	}
+
+	return __acpi_nfit_clear_to_send(nd_desc, nvdimm, cmd);
+}
+
 int acpi_nfit_ars_rescan(struct acpi_nfit_desc *acpi_desc, unsigned long flags)
 {
 	struct device *dev = acpi_desc->dev;
diff --git a/drivers/acpi/nfit/intel.h b/drivers/acpi/nfit/intel.h
new file mode 100644
index 000000000000..a6f8f4bc8ebb
--- /dev/null
+++ b/drivers/acpi/nfit/intel.h
@@ -0,0 +1,67 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/* Copyright(c) 2018 Intel Corporation. All rights reserved. */
+/*
+ * Intel specific definitions for NVDIMM Firmware Interface Table - NFIT
+ */
+#ifndef _NFIT_INTEL_H_
+#define _NFIT_INTEL_H_
+
+#ifdef CONFIG_X86
+
+#define ND_INTEL_STATUS_SIZE		4
+#define ND_INTEL_PASSPHRASE_SIZE	32
+
+#define ND_INTEL_STATUS_RETRY		5
+#define ND_INTEL_STATUS_NOT_READY	9
+#define ND_INTEL_STATUS_INVALID_STATE	10
+#define ND_INTEL_STATUS_INVALID_PASS	11
+
+#define ND_INTEL_SEC_STATE_ENABLED	0x02
+#define ND_INTEL_SEC_STATE_LOCKED	0x04
+#define ND_INTEL_SEC_STATE_FROZEN	0x08
+#define ND_INTEL_SEC_STATE_PLIMIT	0x10
+#define ND_INTEL_SEC_STATE_UNSUPPORTED	0x20
+
+struct nd_intel_get_security_state {
+	u32 status;
+	u32 reserved;
+	u8 state;
+	u8 reserved1[3];
+} __packed;
+
+struct nd_intel_set_passphrase {
+	u8 old_pass[ND_INTEL_PASSPHRASE_SIZE];
+	u8 new_pass[ND_INTEL_PASSPHRASE_SIZE];
+	u32 status;
+} __packed;
+
+struct nd_intel_unlock_unit {
+	u8 passphrase[ND_INTEL_PASSPHRASE_SIZE];
+	u32 status;
+} __packed;
+
+struct nd_intel_disable_passphrase {
+	u8 passphrase[ND_INTEL_PASSPHRASE_SIZE];
+	u32 status;
+} __packed;
+
+struct nd_intel_freeze_lock {
+	u32 status;
+} __packed;
+
+struct nd_intel_secure_erase {
+	u8 passphrase[ND_INTEL_PASSPHRASE_SIZE];
+	u32 status;
+} __packed;
+
+struct nd_intel_overwrite {
+	u8 passphrase[ND_INTEL_PASSPHRASE_SIZE];
+	u32 status;
+} __packed;
+
+struct nd_intel_query_overwrite {
+	u32 status;
+} __packed;
+#endif /* CONFIG_X86 */
+
+#endif
diff --git a/drivers/acpi/nfit/nfit.h b/drivers/acpi/nfit/nfit.h
index d1274ea2d251..1c4e26dd6425 100644
--- a/drivers/acpi/nfit/nfit.h
+++ b/drivers/acpi/nfit/nfit.h
@@ -60,14 +60,29 @@ enum nvdimm_family_cmds {
 	NVDIMM_INTEL_QUERY_FWUPDATE = 16,
 	NVDIMM_INTEL_SET_THRESHOLD = 17,
 	NVDIMM_INTEL_INJECT_ERROR = 18,
+	NVDIMM_INTEL_GET_SECURITY_STATE = 19,
+	NVDIMM_INTEL_SET_PASSPHRASE = 20,
+	NVDIMM_INTEL_DISABLE_PASSPHRASE = 21,
+	NVDIMM_INTEL_UNLOCK_UNIT = 22,
+	NVDIMM_INTEL_FREEZE_LOCK = 23,
+	NVDIMM_INTEL_SECURE_ERASE = 24,
+	NVDIMM_INTEL_OVERWRITE = 25,
+	NVDIMM_INTEL_QUERY_OVERWRITE = 26,
 };
 
+#define NVDIMM_INTEL_SECURITY_CMDMASK \
+(1 << NVDIMM_INTEL_GET_SECURITY_STATE | 1 << NVDIMM_INTEL_SET_PASSPHRASE \
+| 1 << NVDIMM_INTEL_DISABLE_PASSPHRASE | 1 << NVDIMM_INTEL_UNLOCK_UNIT \
+| 1 << NVDIMM_INTEL_FREEZE_LOCK | 1 << NVDIMM_INTEL_SECURE_ERASE \
+| 1 << NVDIMM_INTEL_OVERWRITE | 1 << NVDIMM_INTEL_QUERY_OVERWRITE)
+
 #define NVDIMM_INTEL_CMDMASK \
 (NVDIMM_STANDARD_CMDMASK | 1 << NVDIMM_INTEL_GET_MODES \
  | 1 << NVDIMM_INTEL_GET_FWINFO | 1 << NVDIMM_INTEL_START_FWUPDATE \
  | 1 << NVDIMM_INTEL_SEND_FWUPDATE | 1 << NVDIMM_INTEL_FINISH_FWUPDATE \
  | 1 << NVDIMM_INTEL_QUERY_FWUPDATE | 1 << NVDIMM_INTEL_SET_THRESHOLD \
- | 1 << NVDIMM_INTEL_INJECT_ERROR | 1 << NVDIMM_INTEL_LATCH_SHUTDOWN)
+ | 1 << NVDIMM_INTEL_INJECT_ERROR | 1 << NVDIMM_INTEL_LATCH_SHUTDOWN \
+ | NVDIMM_INTEL_SECURITY_CMDMASK)
 
 enum nfit_uuids {
 	/* for simplicity alias the uuid index with the family id */
diff --git a/drivers/nvdimm/bus.c b/drivers/nvdimm/bus.c
index f1fb39921236..9743d8083538 100644
--- a/drivers/nvdimm/bus.c
+++ b/drivers/nvdimm/bus.c
@@ -902,7 +902,7 @@ static int nd_cmd_clear_to_send(struct nvdimm_bus *nvdimm_bus,
 
 	/* ask the bus provider if it would like to block this request */
 	if (nd_desc->clear_to_send) {
-		int rc = nd_desc->clear_to_send(nd_desc, nvdimm, cmd);
+		int rc = nd_desc->clear_to_send(nd_desc, nvdimm, cmd, data);
 
 		if (rc)
 			return rc;
diff --git a/include/linux/libnvdimm.h b/include/linux/libnvdimm.h
index 097072c5a852..472171af7f60 100644
--- a/include/linux/libnvdimm.h
+++ b/include/linux/libnvdimm.h
@@ -87,7 +87,7 @@ struct nvdimm_bus_descriptor {
 	ndctl_fn ndctl;
 	int (*flush_probe)(struct nvdimm_bus_descriptor *nd_desc);
 	int (*clear_to_send)(struct nvdimm_bus_descriptor *nd_desc,
-			struct nvdimm *nvdimm, unsigned int cmd);
+			struct nvdimm *nvdimm, unsigned int cmd, void *data);
 };
 
 struct nd_cmd_desc {

_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

^ permalink raw reply related	[flat|nested] 17+ messages in thread

* [PATCH v12 02/12] nfit/libnvdimm: store dimm id as a member to struct nvdimm
  2018-10-08 23:55 [PATCH v12 00/12] Adding security support for nvdimm Dave Jiang
  2018-10-08 23:55 ` [PATCH v12 01/12] nfit: add support for Intel DSM 1.7 commands Dave Jiang
@ 2018-10-08 23:55 ` Dave Jiang
  2018-10-08 23:55 ` [PATCH v12 03/12] keys: export lookup_user_key to external users Dave Jiang
                   ` (10 subsequent siblings)
  12 siblings, 0 replies; 17+ messages in thread
From: Dave Jiang @ 2018-10-08 23:55 UTC (permalink / raw)
  To: dan.j.williams
  Cc: alison.schofield, keescook, linux-nvdimm, ebiggers3, dhowells, keyrings

The generated dimm id is needed for the sysfs attribute as well as being
used as the identifier/description for the security key. Since it's
constant and should never change, store it as a member of struct nvdimm.

Signed-off-by: Dave Jiang <dave.jiang@intel.com>
Reviewed-by: Dan Williams <dan.j.williams@intel.com>
Signed-off-by: Dan Williams <dan.j.williams@intel.com>
---
 drivers/acpi/nfit/core.c   |   29 +++++++++++++++++------------
 drivers/acpi/nfit/nfit.h   |    3 +++
 drivers/nvdimm/dimm_devs.c |    4 +++-
 drivers/nvdimm/nd-core.h   |    1 +
 include/linux/libnvdimm.h  |    2 +-
 5 files changed, 25 insertions(+), 14 deletions(-)

diff --git a/drivers/acpi/nfit/core.c b/drivers/acpi/nfit/core.c
index 4f3b89dc9238..94755a4b6327 100644
--- a/drivers/acpi/nfit/core.c
+++ b/drivers/acpi/nfit/core.c
@@ -1572,18 +1572,10 @@ static DEVICE_ATTR_RO(flags);
 static ssize_t id_show(struct device *dev,
 		struct device_attribute *attr, char *buf)
 {
-	struct acpi_nfit_control_region *dcr = to_nfit_dcr(dev);
+	struct nvdimm *nvdimm = to_nvdimm(dev);
+	struct nfit_mem *nfit_mem = nvdimm_provider_data(nvdimm);
 
-	if (dcr->valid_fields & ACPI_NFIT_CONTROL_MFG_INFO_VALID)
-		return sprintf(buf, "%04x-%02x-%04x-%08x\n",
-				be16_to_cpu(dcr->vendor_id),
-				dcr->manufacturing_location,
-				be16_to_cpu(dcr->manufacturing_date),
-				be32_to_cpu(dcr->serial_number));
-	else
-		return sprintf(buf, "%04x-%08x\n",
-				be16_to_cpu(dcr->vendor_id),
-				be32_to_cpu(dcr->serial_number));
+	return sprintf(buf, "%s\n", nfit_mem->id);
 }
 static DEVICE_ATTR_RO(id);
 
@@ -1712,10 +1704,23 @@ static int acpi_nfit_add_dimm(struct acpi_nfit_desc *acpi_desc,
 	const guid_t *guid;
 	int i;
 	int family = -1;
+	struct acpi_nfit_control_region *dcr = nfit_mem->dcr;
 
 	/* nfit test assumes 1:1 relationship between commands and dsms */
 	nfit_mem->dsm_mask = acpi_desc->dimm_cmd_force_en;
 	nfit_mem->family = NVDIMM_FAMILY_INTEL;
+
+	if (dcr->valid_fields & ACPI_NFIT_CONTROL_MFG_INFO_VALID)
+		sprintf(nfit_mem->id, "%04x-%02x-%04x-%08x",
+				be16_to_cpu(dcr->vendor_id),
+				dcr->manufacturing_location,
+				be16_to_cpu(dcr->manufacturing_date),
+				be32_to_cpu(dcr->serial_number));
+	else
+		sprintf(nfit_mem->id, "%04x-%08x",
+				be16_to_cpu(dcr->vendor_id),
+				be32_to_cpu(dcr->serial_number));
+
 	adev = to_acpi_dev(acpi_desc);
 	if (!adev)
 		return 0;
@@ -1899,7 +1904,7 @@ static int acpi_nfit_register_dimms(struct acpi_nfit_desc *acpi_desc)
 		nvdimm = nvdimm_create(acpi_desc->nvdimm_bus, nfit_mem,
 				acpi_nfit_dimm_attribute_groups,
 				flags, cmd_mask, flush ? flush->hint_count : 0,
-				nfit_mem->flush_wpq);
+				nfit_mem->flush_wpq, &nfit_mem->id[0]);
 		if (!nvdimm)
 			return -ENOMEM;
 
diff --git a/drivers/acpi/nfit/nfit.h b/drivers/acpi/nfit/nfit.h
index 1c4e26dd6425..36c8695a3d27 100644
--- a/drivers/acpi/nfit/nfit.h
+++ b/drivers/acpi/nfit/nfit.h
@@ -174,6 +174,8 @@ struct nfit_memdev {
 	struct acpi_nfit_memory_map memdev[0];
 };
 
+#define NFIT_DIMM_ID_LEN	22
+
 /* assembled tables for a given dimm/memory-device */
 struct nfit_mem {
 	struct nvdimm *nvdimm;
@@ -196,6 +198,7 @@ struct nfit_mem {
 	int family;
 	bool has_lsr;
 	bool has_lsw;
+	char id[NFIT_DIMM_ID_LEN+1];
 };
 
 struct acpi_nfit_desc {
diff --git a/drivers/nvdimm/dimm_devs.c b/drivers/nvdimm/dimm_devs.c
index 863cabc35215..3f02008d75e7 100644
--- a/drivers/nvdimm/dimm_devs.c
+++ b/drivers/nvdimm/dimm_devs.c
@@ -398,7 +398,7 @@ EXPORT_SYMBOL_GPL(nvdimm_attribute_group);
 struct nvdimm *nvdimm_create(struct nvdimm_bus *nvdimm_bus, void *provider_data,
 		const struct attribute_group **groups, unsigned long flags,
 		unsigned long cmd_mask, int num_flush,
-		struct resource *flush_wpq)
+		struct resource *flush_wpq, const char *dimm_id)
 {
 	struct nvdimm *nvdimm = kzalloc(sizeof(*nvdimm), GFP_KERNEL);
 	struct device *dev;
@@ -411,6 +411,8 @@ struct nvdimm *nvdimm_create(struct nvdimm_bus *nvdimm_bus, void *provider_data,
 		kfree(nvdimm);
 		return NULL;
 	}
+
+	nvdimm->dimm_id = dimm_id;
 	nvdimm->provider_data = provider_data;
 	nvdimm->flags = flags;
 	nvdimm->cmd_mask = cmd_mask;
diff --git a/drivers/nvdimm/nd-core.h b/drivers/nvdimm/nd-core.h
index 182258f64417..ff26876e6ea3 100644
--- a/drivers/nvdimm/nd-core.h
+++ b/drivers/nvdimm/nd-core.h
@@ -41,6 +41,7 @@ struct nvdimm {
 	atomic_t busy;
 	int id, num_flush;
 	struct resource *flush_wpq;
+	const char *dimm_id;
 };
 
 /**
diff --git a/include/linux/libnvdimm.h b/include/linux/libnvdimm.h
index 472171af7f60..ac9de5ba3536 100644
--- a/include/linux/libnvdimm.h
+++ b/include/linux/libnvdimm.h
@@ -178,7 +178,7 @@ void *nvdimm_provider_data(struct nvdimm *nvdimm);
 struct nvdimm *nvdimm_create(struct nvdimm_bus *nvdimm_bus, void *provider_data,
 		const struct attribute_group **groups, unsigned long flags,
 		unsigned long cmd_mask, int num_flush,
-		struct resource *flush_wpq);
+		struct resource *flush_wpq, const char *dimm_id);
 const struct nd_cmd_desc *nd_cmd_dimm_desc(int cmd);
 const struct nd_cmd_desc *nd_cmd_bus_desc(int cmd);
 u32 nd_cmd_in_size(struct nvdimm *nvdimm, int cmd,

_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

^ permalink raw reply related	[flat|nested] 17+ messages in thread

* [PATCH v12 03/12] keys: export lookup_user_key to external users
  2018-10-08 23:55 [PATCH v12 00/12] Adding security support for nvdimm Dave Jiang
  2018-10-08 23:55 ` [PATCH v12 01/12] nfit: add support for Intel DSM 1.7 commands Dave Jiang
  2018-10-08 23:55 ` [PATCH v12 02/12] nfit/libnvdimm: store dimm id as a member to struct nvdimm Dave Jiang
@ 2018-10-08 23:55 ` Dave Jiang
  2018-10-08 23:55 ` [PATCH v12 04/12] nfit/libnvdimm: add unlock of nvdimm support for Intel DIMMs Dave Jiang
                   ` (9 subsequent siblings)
  12 siblings, 0 replies; 17+ messages in thread
From: Dave Jiang @ 2018-10-08 23:55 UTC (permalink / raw)
  To: dan.j.williams
  Cc: alison.schofield, keescook, linux-nvdimm, ebiggers3, dhowells, keyrings

Export lookup_user_key() symbol in order to allow nvdimm passphrase
update to retrieve user injected keys.

Signed-off-by: Dave Jiang <dave.jiang@intel.com>
Acked-by: David Howells <dhowells at redhat.com>
Signed-off-by: Dan Williams <dan.j.williams@intel.com>
---
 include/linux/key.h          |    3 +++
 security/keys/internal.h     |    2 --
 security/keys/process_keys.c |    1 +
 3 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/include/linux/key.h b/include/linux/key.h
index e58ee10f6e58..7099985e35a9 100644
--- a/include/linux/key.h
+++ b/include/linux/key.h
@@ -346,6 +346,9 @@ static inline key_serial_t key_serial(const struct key *key)
 
 extern void key_set_timeout(struct key *, unsigned);
 
+extern key_ref_t lookup_user_key(key_serial_t id, unsigned long flags,
+				 key_perm_t perm);
+
 /*
  * The permissions required on a key that we're looking up.
  */
diff --git a/security/keys/internal.h b/security/keys/internal.h
index 9f8208dc0e55..9968b21a76dd 100644
--- a/security/keys/internal.h
+++ b/security/keys/internal.h
@@ -158,8 +158,6 @@ extern struct key *request_key_and_link(struct key_type *type,
 
 extern bool lookup_user_key_possessed(const struct key *key,
 				      const struct key_match_data *match_data);
-extern key_ref_t lookup_user_key(key_serial_t id, unsigned long flags,
-				 key_perm_t perm);
 #define KEY_LOOKUP_CREATE	0x01
 #define KEY_LOOKUP_PARTIAL	0x02
 #define KEY_LOOKUP_FOR_UNLINK	0x04
diff --git a/security/keys/process_keys.c b/security/keys/process_keys.c
index d5b25e535d3a..ec4fd4531224 100644
--- a/security/keys/process_keys.c
+++ b/security/keys/process_keys.c
@@ -755,6 +755,7 @@ key_ref_t lookup_user_key(key_serial_t id, unsigned long lflags,
 	put_cred(ctx.cred);
 	goto try_again;
 }
+EXPORT_SYMBOL(lookup_user_key);
 
 /*
  * Join the named keyring as the session keyring if possible else attempt to

_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

^ permalink raw reply related	[flat|nested] 17+ messages in thread

* [PATCH v12 04/12] nfit/libnvdimm: add unlock of nvdimm support for Intel DIMMs
  2018-10-08 23:55 [PATCH v12 00/12] Adding security support for nvdimm Dave Jiang
                   ` (2 preceding siblings ...)
  2018-10-08 23:55 ` [PATCH v12 03/12] keys: export lookup_user_key to external users Dave Jiang
@ 2018-10-08 23:55 ` Dave Jiang
  2018-10-09 19:07   ` Dan Williams
  2018-10-08 23:55 ` [PATCH v12 05/12] nfit/libnvdimm: add set passphrase support for Intel nvdimms Dave Jiang
                   ` (8 subsequent siblings)
  12 siblings, 1 reply; 17+ messages in thread
From: Dave Jiang @ 2018-10-08 23:55 UTC (permalink / raw)
  To: dan.j.williams
  Cc: alison.schofield, keescook, linux-nvdimm, ebiggers3, dhowells, keyrings

Add support to allow query the security status of the Intel nvdimms and
also unlock the dimm via the kernel key management APIs. The passphrase is
expected to be pulled from userspace through keyutils. Moving the Intel
related bits to its own source file as well.

Signed-off-by: Dave Jiang <dave.jiang@intel.com>
Reviewed-by: Dan Williams <dan.j.williams@intel.com>
Signed-off-by: Dan Williams <dan.j.williams@intel.com>
---
 drivers/acpi/nfit/Makefile          |    1 
 drivers/acpi/nfit/core.c            |    3 -
 drivers/acpi/nfit/intel.c           |  152 +++++++++++++++++++++++++++++++++++
 drivers/acpi/nfit/intel.h           |   15 +++
 drivers/nvdimm/Kconfig              |    5 +
 drivers/nvdimm/Makefile             |    1 
 drivers/nvdimm/dimm.c               |    7 ++
 drivers/nvdimm/dimm_devs.c          |   12 +++
 drivers/nvdimm/dimm_devs_security.c |  129 ++++++++++++++++++++++++++++++
 drivers/nvdimm/nd-core.h            |    5 +
 drivers/nvdimm/nd.h                 |   21 +++++
 include/linux/libnvdimm.h           |   27 ++++++
 include/uapi/linux/ndctl.h          |    6 +
 tools/testing/nvdimm/Kbuild         |    2 
 14 files changed, 383 insertions(+), 3 deletions(-)
 create mode 100644 drivers/acpi/nfit/intel.c
 create mode 100644 drivers/nvdimm/dimm_devs_security.c

diff --git a/drivers/acpi/nfit/Makefile b/drivers/acpi/nfit/Makefile
index a407e769f103..443c7ef4e6a6 100644
--- a/drivers/acpi/nfit/Makefile
+++ b/drivers/acpi/nfit/Makefile
@@ -1,3 +1,4 @@
 obj-$(CONFIG_ACPI_NFIT) := nfit.o
 nfit-y := core.o
+nfit-$(CONFIG_X86) += intel.o
 nfit-$(CONFIG_X86_MCE) += mce.o
diff --git a/drivers/acpi/nfit/core.c b/drivers/acpi/nfit/core.c
index 94755a4b6327..e5f7c664a7c8 100644
--- a/drivers/acpi/nfit/core.c
+++ b/drivers/acpi/nfit/core.c
@@ -1904,7 +1904,8 @@ static int acpi_nfit_register_dimms(struct acpi_nfit_desc *acpi_desc)
 		nvdimm = nvdimm_create(acpi_desc->nvdimm_bus, nfit_mem,
 				acpi_nfit_dimm_attribute_groups,
 				flags, cmd_mask, flush ? flush->hint_count : 0,
-				nfit_mem->flush_wpq, &nfit_mem->id[0]);
+				nfit_mem->flush_wpq, &nfit_mem->id[0],
+				acpi_nfit_get_security_ops(nfit_mem->family));
 		if (!nvdimm)
 			return -ENOMEM;
 
diff --git a/drivers/acpi/nfit/intel.c b/drivers/acpi/nfit/intel.c
new file mode 100644
index 000000000000..4bfc1c1da339
--- /dev/null
+++ b/drivers/acpi/nfit/intel.c
@@ -0,0 +1,152 @@
+// SPDX-License-Identifier: GPL-2.0
+/* Copyright(c) 2018 Intel Corporation. All rights reserved. */
+/*
+ * Intel specific NFIT ops
+ */
+#include <linux/libnvdimm.h>
+#include <linux/module.h>
+#include <linux/mutex.h>
+#include <linux/ndctl.h>
+#include <linux/sysfs.h>
+#include <linux/delay.h>
+#include <linux/acpi.h>
+#include <linux/io.h>
+#include <linux/nd.h>
+#include <asm/cacheflush.h>
+#include <asm/smp.h>
+#include <acpi/nfit.h>
+#include "intel.h"
+#include "nfit.h"
+
+static int intel_dimm_security_unlock(struct nvdimm_bus *nvdimm_bus,
+		struct nvdimm *nvdimm, const struct nvdimm_key_data *nkey)
+{
+	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_unlock_unit cmd;
+	} nd_cmd = {
+		.pkg = {
+			.nd_command = NVDIMM_INTEL_UNLOCK_UNIT,
+			.nd_family = NVDIMM_FAMILY_INTEL,
+			.nd_size_in = ND_INTEL_PASSPHRASE_SIZE,
+			.nd_size_out = ND_INTEL_STATUS_SIZE,
+			.nd_fw_size = ND_INTEL_STATUS_SIZE,
+		},
+		.cmd = {
+			.status = 0,
+		},
+	};
+
+	if (!test_bit(NVDIMM_INTEL_UNLOCK_UNIT, &nfit_mem->dsm_mask))
+		return -ENOTTY;
+
+	memcpy(nd_cmd.cmd.passphrase, nkey->data,
+			sizeof(nd_cmd.cmd.passphrase));
+	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;
+	}
+
+	/*
+	 * TODO: define a cross arch wbinvd when/if NVDIMM_FAMILY_INTEL
+	 * support arrives on another arch.
+	 */
+	/* DIMM unlocked, invalidate all CPU caches before we read it */
+	wbinvd_on_all_cpus();
+
+ out:
+	return rc;
+}
+
+static int intel_dimm_security_state(struct nvdimm_bus *nvdimm_bus,
+		struct nvdimm *nvdimm, enum nvdimm_security_state *state)
+{
+	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_get_security_state cmd;
+	} nd_cmd = {
+		.pkg = {
+			.nd_command = NVDIMM_INTEL_GET_SECURITY_STATE,
+			.nd_family = NVDIMM_FAMILY_INTEL,
+			.nd_size_in = 0,
+			.nd_size_out =
+				sizeof(struct nd_intel_get_security_state),
+			.nd_fw_size =
+				sizeof(struct nd_intel_get_security_state),
+		},
+		.cmd = {
+			.status = 0,
+			.state = 0,
+		},
+	};
+
+	if (!test_bit(NVDIMM_INTEL_GET_SECURITY_STATE, &nfit_mem->dsm_mask)) {
+		*state = NVDIMM_SECURITY_UNSUPPORTED;
+		return 0;
+	}
+
+	*state = NVDIMM_SECURITY_DISABLED;
+	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_RETRY:
+		rc = -EAGAIN;
+		goto out;
+	case ND_INTEL_STATUS_NOT_READY:
+	default:
+		rc = -ENXIO;
+		goto out;
+	}
+
+	/* check and see if security is enabled and locked */
+	if (nd_cmd.cmd.state & ND_INTEL_SEC_STATE_UNSUPPORTED)
+		*state = NVDIMM_SECURITY_UNSUPPORTED;
+	else if (nd_cmd.cmd.state & ND_INTEL_SEC_STATE_ENABLED) {
+		if (nd_cmd.cmd.state & ND_INTEL_SEC_STATE_LOCKED)
+			*state = NVDIMM_SECURITY_LOCKED;
+		else
+			*state = NVDIMM_SECURITY_UNLOCKED;
+	} else
+		*state = NVDIMM_SECURITY_DISABLED;
+
+ out:
+	if (rc < 0)
+		*state = NVDIMM_SECURITY_INVALID;
+	return rc;
+}
+
+const struct nvdimm_security_ops intel_security_ops = {
+	.state = intel_dimm_security_state,
+	.unlock = intel_dimm_security_unlock,
+};
diff --git a/drivers/acpi/nfit/intel.h b/drivers/acpi/nfit/intel.h
index a6f8f4bc8ebb..f9ac718776ca 100644
--- a/drivers/acpi/nfit/intel.h
+++ b/drivers/acpi/nfit/intel.h
@@ -8,6 +8,8 @@
 
 #ifdef CONFIG_X86
 
+extern const struct nvdimm_security_ops intel_security_ops;
+
 #define ND_INTEL_STATUS_SIZE		4
 #define ND_INTEL_PASSPHRASE_SIZE	32
 
@@ -64,4 +66,17 @@ struct nd_intel_query_overwrite {
 } __packed;
 #endif /* CONFIG_X86 */
 
+static inline const struct nvdimm_security_ops *
+acpi_nfit_get_security_ops(int family)
+{
+	switch (family) {
+#ifdef CONFIG_X86
+	case NVDIMM_FAMILY_INTEL:
+		return &intel_security_ops;
+#endif
+	default:
+		return NULL;
+	}
+}
+
 #endif
diff --git a/drivers/nvdimm/Kconfig b/drivers/nvdimm/Kconfig
index 9d36473dc2a2..09fa6bba975d 100644
--- a/drivers/nvdimm/Kconfig
+++ b/drivers/nvdimm/Kconfig
@@ -112,4 +112,9 @@ config OF_PMEM
 
 	  Select Y if unsure.
 
+config NVDIMM_SECURITY
+	bool
+	depends on KEYS
+	default LIBNVDIMM
+
 endif
diff --git a/drivers/nvdimm/Makefile b/drivers/nvdimm/Makefile
index e8847045dac0..c476547348b1 100644
--- a/drivers/nvdimm/Makefile
+++ b/drivers/nvdimm/Makefile
@@ -27,3 +27,4 @@ libnvdimm-$(CONFIG_ND_CLAIM) += claim.o
 libnvdimm-$(CONFIG_BTT) += btt_devs.o
 libnvdimm-$(CONFIG_NVDIMM_PFN) += pfn_devs.o
 libnvdimm-$(CONFIG_NVDIMM_DAX) += dax_devs.o
+libnvdimm-$(CONFIG_NVDIMM_SECURITY) += dimm_devs_security.o
diff --git a/drivers/nvdimm/dimm.c b/drivers/nvdimm/dimm.c
index 6c8fb7590838..b6381ddbd6c1 100644
--- a/drivers/nvdimm/dimm.c
+++ b/drivers/nvdimm/dimm.c
@@ -51,6 +51,13 @@ static int nvdimm_probe(struct device *dev)
 	get_device(dev);
 	kref_init(&ndd->kref);
 
+	nvdimm_security_get_state(dev);
+
+	/* unlock DIMM here before touch label */
+	rc = nvdimm_security_unlock_dimm(dev);
+	if (rc < 0)
+		dev_warn(dev, "failed to unlock dimm %s\n", dev_name(dev));
+
 	/*
 	 * EACCES failures reading the namespace label-area-properties
 	 * are interpreted as the DIMM capacity being locked but the
diff --git a/drivers/nvdimm/dimm_devs.c b/drivers/nvdimm/dimm_devs.c
index 3f02008d75e7..024b1c4dfe88 100644
--- a/drivers/nvdimm/dimm_devs.c
+++ b/drivers/nvdimm/dimm_devs.c
@@ -18,6 +18,7 @@
 #include <linux/io.h>
 #include <linux/fs.h>
 #include <linux/mm.h>
+#include <linux/key.h>
 #include "nd-core.h"
 #include "label.h"
 #include "pmem.h"
@@ -212,7 +213,13 @@ 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);
 }
@@ -398,7 +405,8 @@ EXPORT_SYMBOL_GPL(nvdimm_attribute_group);
 struct nvdimm *nvdimm_create(struct nvdimm_bus *nvdimm_bus, void *provider_data,
 		const struct attribute_group **groups, unsigned long flags,
 		unsigned long cmd_mask, int num_flush,
-		struct resource *flush_wpq, const char *dimm_id)
+		struct resource *flush_wpq, const char *dimm_id,
+		const struct nvdimm_security_ops *sec_ops)
 {
 	struct nvdimm *nvdimm = kzalloc(sizeof(*nvdimm), GFP_KERNEL);
 	struct device *dev;
@@ -413,12 +421,14 @@ struct nvdimm *nvdimm_create(struct nvdimm_bus *nvdimm_bus, void *provider_data,
 	}
 
 	nvdimm->dimm_id = dimm_id;
+	nvdimm->security_ops = sec_ops;
 	nvdimm->provider_data = provider_data;
 	nvdimm->flags = flags;
 	nvdimm->cmd_mask = cmd_mask;
 	nvdimm->num_flush = num_flush;
 	nvdimm->flush_wpq = flush_wpq;
 	atomic_set(&nvdimm->busy, 0);
+	mutex_init(&nvdimm->key_mutex);
 	dev = &nvdimm->dev;
 	dev_set_name(dev, "nmem%d", nvdimm->id);
 	dev->parent = &nvdimm_bus->dev;
diff --git a/drivers/nvdimm/dimm_devs_security.c b/drivers/nvdimm/dimm_devs_security.c
new file mode 100644
index 000000000000..9ebf0b4f4739
--- /dev/null
+++ b/drivers/nvdimm/dimm_devs_security.c
@@ -0,0 +1,129 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/* Copyright(c) 2018 Intel Corporation. All rights reserved. */
+
+#include <linux/device.h>
+#include <linux/ndctl.h>
+#include <linux/slab.h>
+#include <linux/io.h>
+#include <linux/mm.h>
+#include <linux/cred.h>
+#include <linux/key.h>
+#include <keys/user-type.h>
+#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;
+}
+
+/*
+ * Retrieve kernel key for DIMM and request from user space if necessary.
+ */
+static struct key *nvdimm_request_key(struct device *dev)
+{
+	struct nvdimm *nvdimm = to_nvdimm(dev);
+	struct key *key = NULL;
+	char desc[NVDIMM_KEY_DESC_LEN + sizeof(NVDIMM_PREFIX)];
+
+	sprintf(desc, "%s%s", NVDIMM_PREFIX, nvdimm->dimm_id);
+	key = request_key(&key_type_logon, desc, "");
+	if (IS_ERR(key))
+		key = NULL;
+
+	return key;
+}
+
+static int nvdimm_check_key_len(unsigned short len)
+{
+	if (len == NVDIMM_PASSPHRASE_LEN)
+		return 0;
+
+	return -EINVAL;
+}
+
+int nvdimm_security_get_state(struct device *dev)
+{
+	struct nvdimm *nvdimm = to_nvdimm(dev);
+	struct nvdimm_bus *nvdimm_bus = walk_to_nvdimm_bus(dev);
+
+	if (!nvdimm->security_ops)
+		return 0;
+
+	return nvdimm->security_ops->state(nvdimm_bus, nvdimm,
+			&nvdimm->state);
+}
+
+int nvdimm_security_unlock_dimm(struct device *dev)
+{
+	struct nvdimm *nvdimm = to_nvdimm(dev);
+	struct nvdimm_bus *nvdimm_bus = walk_to_nvdimm_bus(dev);
+	struct key *key;
+	struct user_key_payload *payload;
+	int rc;
+	bool cached_key = false;
+
+	if (!nvdimm->security_ops)
+		return 0;
+
+	if (nvdimm->state == NVDIMM_SECURITY_UNLOCKED ||
+			nvdimm->state == NVDIMM_SECURITY_UNSUPPORTED ||
+			nvdimm->state == NVDIMM_SECURITY_DISABLED)
+		return 0;
+
+	mutex_lock(&nvdimm->key_mutex);
+	key = nvdimm_get_key(dev);
+	if (!key)
+		key = nvdimm_request_key(dev);
+	else
+		cached_key = true;
+	if (!key) {
+		mutex_unlock(&nvdimm->key_mutex);
+		return -ENXIO;
+	}
+
+	if (!cached_key) {
+		rc = nvdimm_check_key_len(key->datalen);
+		if (rc < 0) {
+			key_put(key);
+			mutex_unlock(&nvdimm->key_mutex);
+			return rc;
+		}
+	}
+
+	dev_dbg(dev, "%s: key: %#x\n", __func__, key_serial(key));
+	down_read(&key->sem);
+	payload = key->payload.data[0];
+	rc = nvdimm->security_ops->unlock(nvdimm_bus, nvdimm,
+			(const void *)payload->data);
+	up_read(&key->sem);
+
+	if (rc == 0) {
+		if (!cached_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;
+		dev_warn(dev, "Failed to unlock dimm: %s\n", dev_name(dev));
+	}
+
+	mutex_unlock(&nvdimm->key_mutex);
+	nvdimm_security_get_state(dev);
+	return rc;
+}
diff --git a/drivers/nvdimm/nd-core.h b/drivers/nvdimm/nd-core.h
index ff26876e6ea3..6e2877996a9b 100644
--- a/drivers/nvdimm/nd-core.h
+++ b/drivers/nvdimm/nd-core.h
@@ -17,6 +17,7 @@
 #include <linux/sizes.h>
 #include <linux/mutex.h>
 #include <linux/nd.h>
+#include <linux/key.h>
 
 extern struct list_head nvdimm_bus_list;
 extern struct mutex nvdimm_bus_list_mutex;
@@ -42,6 +43,10 @@ struct nvdimm {
 	int id, num_flush;
 	struct resource *flush_wpq;
 	const char *dimm_id;
+	const struct nvdimm_security_ops *security_ops;
+	enum nvdimm_security_state state;
+	struct key *key;
+	struct mutex key_mutex;
 };
 
 /**
diff --git a/drivers/nvdimm/nd.h b/drivers/nvdimm/nd.h
index 98317e7ce5b5..934219232177 100644
--- a/drivers/nvdimm/nd.h
+++ b/drivers/nvdimm/nd.h
@@ -423,4 +423,25 @@ static inline bool is_bad_pmem(struct badblocks *bb, sector_t sector,
 resource_size_t nd_namespace_blk_validate(struct nd_namespace_blk *nsblk);
 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);
+#else
+static inline struct key *nvdimm_get_key(struct device *dev)
+{
+	return NULL;
+}
+
+static inline int nvdimm_security_unlock_dimm(struct device *dev)
+{
+	return 0;
+}
+
+static inline int nvdimm_security_get_state(struct device *dev)
+{
+	return -EOPNOTSUPP;
+}
+#endif
 #endif /* __ND_H__ */
diff --git a/include/linux/libnvdimm.h b/include/linux/libnvdimm.h
index ac9de5ba3536..257ff2637ce1 100644
--- a/include/linux/libnvdimm.h
+++ b/include/linux/libnvdimm.h
@@ -155,6 +155,30 @@ static inline struct nd_blk_region_desc *to_blk_region_desc(
 
 }
 
+enum nvdimm_security_state {
+	NVDIMM_SECURITY_INVALID = 0,
+	NVDIMM_SECURITY_DISABLED,
+	NVDIMM_SECURITY_UNLOCKED,
+	NVDIMM_SECURITY_LOCKED,
+	NVDIMM_SECURITY_UNSUPPORTED,
+};
+
+#define NVDIMM_PASSPHRASE_LEN		32
+#define NVDIMM_KEY_DESC_LEN		22
+
+struct nvdimm_key_data {
+	u8 data[NVDIMM_PASSPHRASE_LEN];
+};
+
+struct nvdimm_security_ops {
+	int (*state)(struct nvdimm_bus *nvdimm_bus,
+			struct nvdimm *nvdimm,
+			enum nvdimm_security_state *state);
+	int (*unlock)(struct nvdimm_bus *nvdimm_bus,
+			struct nvdimm *nvdimm,
+			const struct nvdimm_key_data *nkey);
+};
+
 void badrange_init(struct badrange *badrange);
 int badrange_add(struct badrange *badrange, u64 addr, u64 length);
 void badrange_forget(struct badrange *badrange, phys_addr_t start,
@@ -178,7 +202,8 @@ void *nvdimm_provider_data(struct nvdimm *nvdimm);
 struct nvdimm *nvdimm_create(struct nvdimm_bus *nvdimm_bus, void *provider_data,
 		const struct attribute_group **groups, unsigned long flags,
 		unsigned long cmd_mask, int num_flush,
-		struct resource *flush_wpq, const char *dimm_id);
+		struct resource *flush_wpq, const char *dimm_id,
+		const struct nvdimm_security_ops *sec_ops);
 const struct nd_cmd_desc *nd_cmd_dimm_desc(int cmd);
 const struct nd_cmd_desc *nd_cmd_bus_desc(int cmd);
 u32 nd_cmd_in_size(struct nvdimm *nvdimm, int cmd,
diff --git a/include/uapi/linux/ndctl.h b/include/uapi/linux/ndctl.h
index 7e27070b9440..dd48a2b5aa3d 100644
--- a/include/uapi/linux/ndctl.h
+++ b/include/uapi/linux/ndctl.h
@@ -257,4 +257,10 @@ struct nd_cmd_pkg {
 #define ND_IOCTL_CALL			_IOWR(ND_IOCTL, ND_CMD_CALL,\
 					struct nd_cmd_pkg)
 
+/*
+ * The key payload description for nvdimm logon key is:
+ * <prefix><bus-provider-specific-unique-id>
+ */
+static const char NVDIMM_PREFIX[] = "nvdimm:";
+
 #endif /* __NDCTL_H__ */
diff --git a/tools/testing/nvdimm/Kbuild b/tools/testing/nvdimm/Kbuild
index 0392153a0009..80cd0b0399ec 100644
--- a/tools/testing/nvdimm/Kbuild
+++ b/tools/testing/nvdimm/Kbuild
@@ -37,6 +37,7 @@ obj-$(CONFIG_DEV_DAX_PMEM) += dax_pmem.o
 
 nfit-y := $(ACPI_SRC)/core.o
 nfit-$(CONFIG_X86_MCE) += $(ACPI_SRC)/mce.o
+nfit-$(CONFIG_X86) += $(ACPI_SRC)/intel.o
 nfit-y += acpi_nfit_test.o
 nfit-y += config_check.o
 
@@ -80,5 +81,6 @@ libnvdimm-$(CONFIG_NVDIMM_PFN) += $(NVDIMM_SRC)/pfn_devs.o
 libnvdimm-$(CONFIG_NVDIMM_DAX) += $(NVDIMM_SRC)/dax_devs.o
 libnvdimm-y += libnvdimm_test.o
 libnvdimm-y += config_check.o
+libnvdimm-$(CONFIG_NVDIMM_SECURITY) += $(NVDIMM_SRC)/dimm_devs_security.o
 
 obj-m += test/

_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

^ permalink raw reply related	[flat|nested] 17+ messages in thread

* [PATCH v12 05/12] nfit/libnvdimm: add set passphrase support for Intel nvdimms
  2018-10-08 23:55 [PATCH v12 00/12] Adding security support for nvdimm Dave Jiang
                   ` (3 preceding siblings ...)
  2018-10-08 23:55 ` [PATCH v12 04/12] nfit/libnvdimm: add unlock of nvdimm support for Intel DIMMs Dave Jiang
@ 2018-10-08 23:55 ` Dave Jiang
  2018-10-08 23:56 ` [PATCH v12 06/12] nfit/libnvdimm: add disable passphrase support to Intel nvdimm Dave Jiang
                   ` (7 subsequent siblings)
  12 siblings, 0 replies; 17+ messages in thread
From: Dave Jiang @ 2018-10-08 23:55 UTC (permalink / raw)
  To: dan.j.williams
  Cc: alison.schofield, keescook, linux-nvdimm, ebiggers3, dhowells, keyrings

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 <old_keyid> <new_keyid>" to the
sysfs attribute "security". If no <old_keyid> exists (for enabling security)
then a 0 should be used. 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>
Signed-off-by: Dan Williams <dan.j.williams@intel.com>
---
 drivers/acpi/nfit/intel.c           |   68 ++++++++++++
 drivers/nvdimm/dimm_devs.c          |   62 +++++++++++
 drivers/nvdimm/dimm_devs_security.c |  192 +++++++++++++++++++++++++++++++++++
 drivers/nvdimm/nd.h                 |    8 +
 include/linux/libnvdimm.h           |    5 +
 5 files changed, 335 insertions(+)

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 024b1c4dfe88..fe1a90636749 100644
--- a/drivers/nvdimm/dimm_devs.c
+++ b/drivers/nvdimm/dimm_devs.c
@@ -389,11 +389,73 @@ static ssize_t available_slots_show(struct device *dev,
 }
 static DEVICE_ATTR_RO(available_slots);
 
+#ifdef CONFIG_NVDIMM_SECURITY
+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;
+}
+
+#define SEC_CMD_SIZE 128
+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_key = 0, old_key = 0;
+	char cmd[SEC_CMD_SIZE+1];
+
+	if (len > SEC_CMD_SIZE)
+		return -EINVAL;
+
+        wait_nvdimm_bus_probe_idle(&nvdimm_bus->dev);
+        if (atomic_read(&nvdimm->busy))
+                return -EBUSY;
+
+	rc = sscanf(buf, "%"__stringify(SEC_CMD_SIZE)"s %u %u",
+			cmd, &old_key, &new_key);
+	if (sysfs_streq(cmd, "update"))	{
+		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);
+	} else
+		return -EINVAL;
+
+	if (rc == 0)
+		rc = len;
+
+	return rc;
+}
+static DEVICE_ATTR_RW(security);
+#endif
+
 static struct attribute *nvdimm_attributes[] = {
 	&dev_attr_state.attr,
 	&dev_attr_flags.attr,
 	&dev_attr_commands.attr,
 	&dev_attr_available_slots.attr,
+#ifdef CONFIG_NVDIMM_SECURITY
+	&dev_attr_security.attr,
+#endif
 	NULL,
 };
 
diff --git a/drivers/nvdimm/dimm_devs_security.c b/drivers/nvdimm/dimm_devs_security.c
index 9ebf0b4f4739..d9d0355e1844 100644
--- a/drivers/nvdimm/dimm_devs_security.c
+++ b/drivers/nvdimm/dimm_devs_security.c
@@ -8,6 +8,7 @@
 #include <linux/mm.h>
 #include <linux/cred.h>
 #include <linux/key.h>
+#include <linux/key-type.h>
 #include <keys/user-type.h>
 #include "nd-core.h"
 #include "nd.h"
@@ -30,6 +31,58 @@ struct key *nvdimm_get_key(struct device *dev)
 	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)
+{
+	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_SEARCH, 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
+ */
+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: %#x\n", __func__, key_serial(key));
+
+	return key;
+}
+
 /*
  * Retrieve kernel key for DIMM and request from user space if necessary.
  */
@@ -55,6 +108,50 @@ static int nvdimm_check_key_len(unsigned short len)
 	return -EINVAL;
 }
 
+struct key *nvdimm_get_and_verify_key(struct device *dev,
+		unsigned int user_key_id)
+{
+	struct key *user_key, *key;
+	struct user_key_payload *upayload, *payload;
+	int rc = 0;
+
+	key = nvdimm_get_key(dev);
+	/* we don't have a cached key */
+	if (!key) {
+		dev_dbg(dev, "No cached kernel key\n");
+		return NULL;
+	}
+	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;
+	}
+	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];
+
+	if (memcmp(payload->data, upayload->data,
+				NVDIMM_PASSPHRASE_LEN) != 0) {
+		rc = -EINVAL;
+		dev_warn(dev, "Supplied old user key fails check.\n");
+	}
+
+	up_read(&user_key->sem);
+	key_put(user_key);
+	up_read(&key->sem);
+
+out:
+	if (rc < 0)
+		key = ERR_PTR(rc);
+	return key;
+}
+
 int nvdimm_security_get_state(struct device *dev)
 {
 	struct nvdimm *nvdimm = to_nvdimm(dev);
@@ -127,3 +224,98 @@ int nvdimm_security_unlock_dimm(struct device *dev)
 	nvdimm_security_get_state(dev);
 	return rc;
 }
+
+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, *old_key;
+	int rc;
+	void *old_data = NULL, *new_data;
+	bool update = false;
+	struct user_key_payload *payload, *old_payload;
+
+	if (!nvdimm->security_ops)
+		return -EOPNOTSUPP;
+
+	if (nvdimm->state == NVDIMM_SECURITY_FROZEN)
+		return -EBUSY;
+
+	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)) {
+		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;
+	}
+
+	/* 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));
+
+	down_read(&key->sem);
+	payload = key->payload.data[0];
+	rc = nvdimm_check_key_len(payload->datalen);
+	if (rc < 0) {
+		up_read(&key->sem);
+		goto out;
+	}
+
+	if (!update)
+		key = nvdimm_replace_key(key);
+
+	/*
+	 * We don't need to release key->sem here because
+	 * nvdimm_replace_key() will.
+	 */
+	if (!key)
+		goto out;
+
+	payload = key->payload.data[0];
+
+	if (old_key) {
+		down_read(&old_key->sem);
+		old_payload = old_key->payload.data[0];
+		old_data = old_payload->data;
+	}
+
+	new_data = payload->data;
+
+	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");
+	if (old_key)
+		up_read(&old_key->sem);
+	up_read(&key->sem);
+
+	if (!update) {
+		if (rc == 0) {
+			dev_dbg(dev, "key cached: %#x\n", key_serial(key));
+			nvdimm->key = key;
+		} else
+			key_invalidate(key);
+	}
+	nvdimm_security_get_state(dev);
+
+ out:
+	mutex_unlock(&nvdimm->key_mutex);
+	return rc;
+}
diff --git a/drivers/nvdimm/nd.h b/drivers/nvdimm/nd.h
index 934219232177..61bdc0c45c11 100644
--- a/drivers/nvdimm/nd.h
+++ b/drivers/nvdimm/nd.h
@@ -428,6 +428,8 @@ bool pmem_should_map_pages(struct device *dev);
 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,
+		unsigned int new_keyid);
 #else
 static inline struct key *nvdimm_get_key(struct device *dev)
 {
@@ -443,5 +445,11 @@ static inline int nvdimm_security_get_state(struct device *dev)
 {
 	return -EOPNOTSUPP;
 }
+
+static inline int nvdimm_security_change_key(struct device *dev,
+		unsigned int old_keyid, unsigned int new_keyid)
+{
+	return -EOPNOTSUPP;
+}
 #endif
 #endif /* __ND_H__ */
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);

_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

^ permalink raw reply related	[flat|nested] 17+ messages in thread

* [PATCH v12 06/12] nfit/libnvdimm: add disable passphrase support to Intel nvdimm.
  2018-10-08 23:55 [PATCH v12 00/12] Adding security support for nvdimm Dave Jiang
                   ` (4 preceding siblings ...)
  2018-10-08 23:55 ` [PATCH v12 05/12] nfit/libnvdimm: add set passphrase support for Intel nvdimms Dave Jiang
@ 2018-10-08 23:56 ` Dave Jiang
  2018-10-08 23:56 ` [PATCH v12 07/12] nfit/libnvdimm: add freeze security " Dave Jiang
                   ` (6 subsequent siblings)
  12 siblings, 0 replies; 17+ messages in thread
From: Dave Jiang @ 2018-10-08 23:56 UTC (permalink / raw)
  To: dan.j.williams
  Cc: alison.schofield, keescook, linux-nvdimm, ebiggers3, dhowells, keyrings

Add support to disable passphrase (security) for the Intel nvdimm. The
passphrase used for disabling is pulled from userspace via the kernel
key management. The action is triggered by writing "disable <old_keyid>" to
the sysfs attribute "security". libnvdimm will support the generic disable
API call. The kernel will verify the passphrase of the user key against
the cached kernel key. If no kernel key exists, then the user key will be
tried for the op.

Signed-off-by: Dave Jiang <dave.jiang@intel.com>
Reviewed-by: Dan Williams <dan.j.williams@intel.com>
Signed-off-by: Dan Williams <dan.j.williams@intel.com>
---
 drivers/acpi/nfit/intel.c           |   53 +++++++++++++++++++++++++++++++++
 drivers/nvdimm/dimm_devs.c          |    5 +++
 drivers/nvdimm/dimm_devs_security.c |   56 +++++++++++++++++++++++++++++++++++
 drivers/nvdimm/nd.h                 |    7 ++++
 include/linux/libnvdimm.h           |    3 ++
 5 files changed, 124 insertions(+)

diff --git a/drivers/acpi/nfit/intel.c b/drivers/acpi/nfit/intel.c
index 314eae7e02d7..21d30222371f 100644
--- a/drivers/acpi/nfit/intel.c
+++ b/drivers/acpi/nfit/intel.c
@@ -18,6 +18,58 @@
 #include "intel.h"
 #include "nfit.h"
 
+static int intel_dimm_security_disable(struct nvdimm_bus *nvdimm_bus,
+		struct nvdimm *nvdimm, const struct nvdimm_key_data *nkey)
+{
+	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_disable_passphrase cmd;
+	} nd_cmd = {
+		.pkg = {
+			.nd_command = NVDIMM_INTEL_DISABLE_PASSPHRASE,
+			.nd_family = NVDIMM_FAMILY_INTEL,
+			.nd_size_in = ND_INTEL_PASSPHRASE_SIZE,
+			.nd_size_out = ND_INTEL_STATUS_SIZE,
+			.nd_fw_size = ND_INTEL_STATUS_SIZE,
+		},
+		.cmd = {
+			.status = 0,
+		},
+	};
+
+	if (!test_bit(NVDIMM_INTEL_DISABLE_PASSPHRASE, &nfit_mem->dsm_mask))
+		return -ENOTTY;
+
+	memcpy(nd_cmd.cmd.passphrase, nkey->data,
+			sizeof(nd_cmd.cmd.passphrase));
+	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;
+}
+
 /*
  * The update passphrase takes the old passphrase and the new passphrase
  * and send those to the nvdimm. The nvdimm will verify the old
@@ -217,4 +269,5 @@ 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,
+	.disable = intel_dimm_security_disable,
 };
diff --git a/drivers/nvdimm/dimm_devs.c b/drivers/nvdimm/dimm_devs.c
index fe1a90636749..ae0c3e02e4f5 100644
--- a/drivers/nvdimm/dimm_devs.c
+++ b/drivers/nvdimm/dimm_devs.c
@@ -437,6 +437,11 @@ static ssize_t security_store(struct device *dev,
 			return -EINVAL;
 		dev_dbg(dev, "update %#x %#x\n", old_key, new_key);
 		rc = nvdimm_security_change_key(dev, 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);
 	} else
 		return -EINVAL;
 
diff --git a/drivers/nvdimm/dimm_devs_security.c b/drivers/nvdimm/dimm_devs_security.c
index d9d0355e1844..6f8b9021292c 100644
--- a/drivers/nvdimm/dimm_devs_security.c
+++ b/drivers/nvdimm/dimm_devs_security.c
@@ -164,6 +164,62 @@ int nvdimm_security_get_state(struct device *dev)
 			&nvdimm->state);
 }
 
+int nvdimm_security_disable(struct device *dev, unsigned int keyid)
+{
+	struct nvdimm *nvdimm = to_nvdimm(dev);
+	struct nvdimm_bus *nvdimm_bus = walk_to_nvdimm_bus(dev);
+	struct key *key;
+	int rc;
+	struct user_key_payload *payload;
+	bool is_userkey = false;
+
+	if (!nvdimm->security_ops)
+		return -EOPNOTSUPP;
+
+	if (nvdimm->state == NVDIMM_SECURITY_UNSUPPORTED)
+		return -EOPNOTSUPP;
+
+	mutex_lock(&nvdimm->key_mutex);
+	/* look for a key from cached key */
+	key = nvdimm_get_and_verify_key(dev, keyid);
+	if (IS_ERR(key)) {
+		mutex_unlock(&nvdimm->key_mutex);
+		return PTR_ERR(key);
+	}
+	if (!key) {
+		/* get old user key */
+		key = nvdimm_lookup_user_key(dev, keyid);
+		if (!key) {
+			mutex_unlock(&nvdimm->key_mutex);
+			return -ENOKEY;
+		}
+		is_userkey = true;
+	}
+
+	down_read(&key->sem);
+	payload = key->payload.data[0];
+
+	rc = nvdimm->security_ops->disable(nvdimm_bus, nvdimm,
+			(void *)payload->data);
+	up_read(&key->sem);
+	if (rc < 0) {
+		dev_warn(dev, "unlock failed\n");
+		goto out;
+	}
+
+	/* If we succeed then remove the key */
+	if (!is_userkey) {
+		key_invalidate(key);
+		nvdimm->key = NULL;
+	}
+	key_put(key);
+
+ out:
+	mutex_unlock(&nvdimm->key_mutex);
+	nvdimm_security_get_state(dev);
+	return rc;
+}
+
 int nvdimm_security_unlock_dimm(struct device *dev)
 {
 	struct nvdimm *nvdimm = to_nvdimm(dev);
diff --git a/drivers/nvdimm/nd.h b/drivers/nvdimm/nd.h
index 61bdc0c45c11..b80cab3a12df 100644
--- a/drivers/nvdimm/nd.h
+++ b/drivers/nvdimm/nd.h
@@ -430,6 +430,7 @@ 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,
 		unsigned int new_keyid);
+int nvdimm_security_disable(struct device *dev, unsigned int keyid);
 #else
 static inline struct key *nvdimm_get_key(struct device *dev)
 {
@@ -451,5 +452,11 @@ static inline int nvdimm_security_change_key(struct device *dev,
 {
 	return -EOPNOTSUPP;
 }
+
+static inline int nvdimm_security_disable(struct device *dev,
+		unsigned int keyid)
+{
+	return -EOPNOTSUPP;
+}
 #endif
 #endif /* __ND_H__ */
diff --git a/include/linux/libnvdimm.h b/include/linux/libnvdimm.h
index bd6a413164ee..c60ab4b238f3 100644
--- a/include/linux/libnvdimm.h
+++ b/include/linux/libnvdimm.h
@@ -182,6 +182,9 @@ struct nvdimm_security_ops {
 			struct nvdimm *nvdimm,
 			const struct nvdimm_key_data *old_data,
 			const struct nvdimm_key_data *new_data);
+	int (*disable)(struct nvdimm_bus *nvdimm_bus,
+			struct nvdimm *nvdimm,
+			const struct nvdimm_key_data *nkey);
 };
 
 void badrange_init(struct badrange *badrange);

_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

^ permalink raw reply related	[flat|nested] 17+ messages in thread

* [PATCH v12 07/12] nfit/libnvdimm: add freeze security support to Intel nvdimm
  2018-10-08 23:55 [PATCH v12 00/12] Adding security support for nvdimm Dave Jiang
                   ` (5 preceding siblings ...)
  2018-10-08 23:56 ` [PATCH v12 06/12] nfit/libnvdimm: add disable passphrase support to Intel nvdimm Dave Jiang
@ 2018-10-08 23:56 ` Dave Jiang
  2018-10-08 23:56 ` [PATCH v12 08/12] nfit/libnvdimm: add support for issue secure erase DSM " Dave Jiang
                   ` (5 subsequent siblings)
  12 siblings, 0 replies; 17+ messages in thread
From: Dave Jiang @ 2018-10-08 23:56 UTC (permalink / raw)
  To: dan.j.williams
  Cc: alison.schofield, keescook, linux-nvdimm, ebiggers3, dhowells, keyrings

Add support for freeze security on Intel nvdimm. This locks out any
changes to security for the DIMM unless a reboot is done. This is triggered
by writing "freeze" to the "security" sysfs attribute. libnvdimm will
support the generic freeze_lock API call.

Signed-off-by: Dave Jiang <dave.jiang@intel.com>
Reviewed-by: Dan Williams <dan.j.williams@intel.com>
Signed-off-by: Dan Williams <dan.j.williams@intel.com>
---
 drivers/acpi/nfit/intel.c           |   51 +++++++++++++++++++++++++++++++++++
 drivers/nvdimm/dimm_devs.c          |    3 ++
 drivers/nvdimm/dimm_devs_security.c |   20 ++++++++++++++
 drivers/nvdimm/nd.h                 |    6 ++++
 include/linux/libnvdimm.h           |    2 +
 5 files changed, 82 insertions(+)

diff --git a/drivers/acpi/nfit/intel.c b/drivers/acpi/nfit/intel.c
index 21d30222371f..ba886f1f5399 100644
--- a/drivers/acpi/nfit/intel.c
+++ b/drivers/acpi/nfit/intel.c
@@ -18,6 +18,53 @@
 #include "intel.h"
 #include "nfit.h"
 
+static int intel_dimm_security_freeze_lock(struct nvdimm_bus *nvdimm_bus,
+		struct nvdimm *nvdimm)
+{
+	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_freeze_lock cmd;
+	} nd_cmd = {
+		.pkg = {
+			.nd_command = NVDIMM_INTEL_FREEZE_LOCK,
+			.nd_family = NVDIMM_FAMILY_INTEL,
+			.nd_size_in = 0,
+			.nd_size_out = ND_INTEL_STATUS_SIZE,
+			.nd_fw_size = ND_INTEL_STATUS_SIZE,
+		},
+		.cmd = {
+			.status = 0,
+		},
+	};
+
+	if (!test_bit(NVDIMM_INTEL_FREEZE_LOCK, &nfit_mem->dsm_mask))
+		return -ENOTTY;
+
+	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_STATE:
+	default:
+		rc = -ENXIO;
+		goto out;
+	}
+
+ out:
+	return rc;
+}
+
 static int intel_dimm_security_disable(struct nvdimm_bus *nvdimm_bus,
 		struct nvdimm *nvdimm, const struct nvdimm_key_data *nkey)
 {
@@ -254,6 +301,9 @@ static int intel_dimm_security_state(struct nvdimm_bus *nvdimm_bus,
 	else if (nd_cmd.cmd.state & ND_INTEL_SEC_STATE_ENABLED) {
 		if (nd_cmd.cmd.state & ND_INTEL_SEC_STATE_LOCKED)
 			*state = NVDIMM_SECURITY_LOCKED;
+		else if (nd_cmd.cmd.state & ND_INTEL_SEC_STATE_FROZEN ||
+				nd_cmd.cmd.state & ND_INTEL_SEC_STATE_PLIMIT)
+			*state = NVDIMM_SECURITY_FROZEN;
 		else
 			*state = NVDIMM_SECURITY_UNLOCKED;
 	} else
@@ -270,4 +320,5 @@ const struct nvdimm_security_ops intel_security_ops = {
 	.unlock = intel_dimm_security_unlock,
 	.change_key = intel_dimm_security_update_passphrase,
 	.disable = intel_dimm_security_disable,
+	.freeze_lock = intel_dimm_security_freeze_lock,
 };
diff --git a/drivers/nvdimm/dimm_devs.c b/drivers/nvdimm/dimm_devs.c
index ae0c3e02e4f5..a1c4c28d951f 100644
--- a/drivers/nvdimm/dimm_devs.c
+++ b/drivers/nvdimm/dimm_devs.c
@@ -442,6 +442,9 @@ static ssize_t security_store(struct device *dev,
 			return -EINVAL;
 		dev_dbg(dev, "disable %#x\n", old_key);
 		rc = nvdimm_security_disable(dev, old_key);
+	} else if (sysfs_streq(buf, "freeze")) {
+		dev_dbg(dev, "freeze\n");
+		rc = nvdimm_security_freeze_lock(dev);
 	} else
 		return -EINVAL;
 
diff --git a/drivers/nvdimm/dimm_devs_security.c b/drivers/nvdimm/dimm_devs_security.c
index 6f8b9021292c..767c5ae06cb5 100644
--- a/drivers/nvdimm/dimm_devs_security.c
+++ b/drivers/nvdimm/dimm_devs_security.c
@@ -164,6 +164,26 @@ int nvdimm_security_get_state(struct device *dev)
 			&nvdimm->state);
 }
 
+int nvdimm_security_freeze_lock(struct device *dev)
+{
+	struct nvdimm *nvdimm = to_nvdimm(dev);
+	struct nvdimm_bus *nvdimm_bus = walk_to_nvdimm_bus(dev);
+	int rc;
+
+	if (!nvdimm->security_ops)
+		return -EOPNOTSUPP;
+
+	if (nvdimm->state == NVDIMM_SECURITY_UNSUPPORTED)
+		return -EOPNOTSUPP;
+
+	rc = nvdimm->security_ops->freeze_lock(nvdimm_bus, nvdimm);
+	if (rc < 0)
+		return rc;
+
+	nvdimm_security_get_state(dev);
+	return 0;
+}
+
 int nvdimm_security_disable(struct device *dev, unsigned int keyid)
 {
 	struct nvdimm *nvdimm = to_nvdimm(dev);
diff --git a/drivers/nvdimm/nd.h b/drivers/nvdimm/nd.h
index b80cab3a12df..d89fe1423f9a 100644
--- a/drivers/nvdimm/nd.h
+++ b/drivers/nvdimm/nd.h
@@ -431,6 +431,7 @@ int nvdimm_security_get_state(struct device *dev);
 int nvdimm_security_change_key(struct device *dev, 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);
 #else
 static inline struct key *nvdimm_get_key(struct device *dev)
 {
@@ -458,5 +459,10 @@ static inline int nvdimm_security_disable(struct device *dev,
 {
 	return -EOPNOTSUPP;
 }
+
+static inline int nvdimm_security_freeze_lock(struct device *dev)
+{
+	return -EOPNOTSUPP;
+}
 #endif
 #endif /* __ND_H__ */
diff --git a/include/linux/libnvdimm.h b/include/linux/libnvdimm.h
index c60ab4b238f3..bcab42caa948 100644
--- a/include/linux/libnvdimm.h
+++ b/include/linux/libnvdimm.h
@@ -185,6 +185,8 @@ struct nvdimm_security_ops {
 	int (*disable)(struct nvdimm_bus *nvdimm_bus,
 			struct nvdimm *nvdimm,
 			const struct nvdimm_key_data *nkey);
+	int (*freeze_lock)(struct nvdimm_bus *nvdimm_bus,
+			struct nvdimm *nvdimm);
 };
 
 void badrange_init(struct badrange *badrange);

_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

^ permalink raw reply related	[flat|nested] 17+ messages in thread

* [PATCH v12 08/12] nfit/libnvdimm: add support for issue secure erase DSM to Intel nvdimm
  2018-10-08 23:55 [PATCH v12 00/12] Adding security support for nvdimm Dave Jiang
                   ` (6 preceding siblings ...)
  2018-10-08 23:56 ` [PATCH v12 07/12] nfit/libnvdimm: add freeze security " Dave Jiang
@ 2018-10-08 23:56 ` Dave Jiang
  2018-10-08 23:56 ` [PATCH v12 09/12] nfit_test: add test support for Intel nvdimm security DSMs Dave Jiang
                   ` (4 subsequent siblings)
  12 siblings, 0 replies; 17+ messages in thread
From: Dave Jiang @ 2018-10-08 23:56 UTC (permalink / raw)
  To: dan.j.williams
  Cc: alison.schofield, keescook, linux-nvdimm, ebiggers3, dhowells, keyrings

Add support to issue a secure erase DSM to the Intel nvdimm. The
required passphrase is acquired from userspace through the kernel key
management. To trigger the action, "erase <old_keyid>" is written to the
"security" sysfs attribute. libnvdimm will support the erase generic API
call. The user key provided will be verified against the cached kernel
key. If no kernel key exists, then the user key will be attempted for the
operation.

Signed-off-by: Dave Jiang <dave.jiang@intel.com>
Reviewed-by: Dan Williams <dan.j.williams@intel.com>
Signed-off-by: Dan Williams <dan.j.williams@intel.com>
---
 drivers/acpi/nfit/intel.c           |   58 ++++++++++++++++++++++++++++
 drivers/nvdimm/dimm_devs.c          |    5 ++
 drivers/nvdimm/dimm_devs_security.c |   72 +++++++++++++++++++++++++++++++++++
 drivers/nvdimm/nd.h                 |    7 +++
 include/linux/libnvdimm.h           |    3 +
 5 files changed, 145 insertions(+)

diff --git a/drivers/acpi/nfit/intel.c b/drivers/acpi/nfit/intel.c
index ba886f1f5399..419a7d54d4e8 100644
--- a/drivers/acpi/nfit/intel.c
+++ b/drivers/acpi/nfit/intel.c
@@ -18,6 +18,63 @@
 #include "intel.h"
 #include "nfit.h"
 
+static int intel_dimm_security_erase(struct nvdimm_bus *nvdimm_bus,
+		struct nvdimm *nvdimm, const struct nvdimm_key_data *nkey)
+{
+	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_secure_erase cmd;
+	} nd_cmd = {
+		.pkg = {
+			.nd_command = NVDIMM_INTEL_SECURE_ERASE,
+			.nd_family = NVDIMM_FAMILY_INTEL,
+			.nd_size_in = ND_INTEL_PASSPHRASE_SIZE,
+			.nd_size_out = ND_INTEL_STATUS_SIZE,
+			.nd_fw_size = ND_INTEL_STATUS_SIZE,
+		},
+		.cmd = {
+			.status = 0,
+		},
+	};
+
+	if (!test_bit(NVDIMM_INTEL_SECURE_ERASE, &nfit_mem->dsm_mask))
+		return -ENOTTY;
+
+	/* flush all cache before we erase DIMM */
+	wbinvd_on_all_cpus();
+	memcpy(nd_cmd.cmd.passphrase, nkey->data,
+			sizeof(nd_cmd.cmd.passphrase));
+	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;
+	}
+
+	/* DIMM erased, invalidate all CPU caches before we read it */
+	wbinvd_on_all_cpus();
+
+ out:
+	return rc;
+}
+
 static int intel_dimm_security_freeze_lock(struct nvdimm_bus *nvdimm_bus,
 		struct nvdimm *nvdimm)
 {
@@ -321,4 +378,5 @@ const struct nvdimm_security_ops intel_security_ops = {
 	.change_key = intel_dimm_security_update_passphrase,
 	.disable = intel_dimm_security_disable,
 	.freeze_lock = intel_dimm_security_freeze_lock,
+	.erase = intel_dimm_security_erase,
 };
diff --git a/drivers/nvdimm/dimm_devs.c b/drivers/nvdimm/dimm_devs.c
index a1c4c28d951f..1421404a687f 100644
--- a/drivers/nvdimm/dimm_devs.c
+++ b/drivers/nvdimm/dimm_devs.c
@@ -445,6 +445,11 @@ static ssize_t security_store(struct device *dev,
 	} else if (sysfs_streq(buf, "freeze")) {
 		dev_dbg(dev, "freeze\n");
 		rc = nvdimm_security_freeze_lock(dev);
+	} 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);
 	} else
 		return -EINVAL;
 
diff --git a/drivers/nvdimm/dimm_devs_security.c b/drivers/nvdimm/dimm_devs_security.c
index 767c5ae06cb5..4a8bd4d94515 100644
--- a/drivers/nvdimm/dimm_devs_security.c
+++ b/drivers/nvdimm/dimm_devs_security.c
@@ -164,6 +164,78 @@ int nvdimm_security_get_state(struct device *dev)
 			&nvdimm->state);
 }
 
+int nvdimm_security_erase(struct device *dev, unsigned int keyid)
+{
+	struct nvdimm *nvdimm = to_nvdimm(dev);
+	struct nvdimm_bus *nvdimm_bus = walk_to_nvdimm_bus(dev);
+	struct key *key;
+	struct user_key_payload *payload;
+	int rc = 0;
+	bool is_userkey = false;
+
+	if (!nvdimm->security_ops)
+		return -EOPNOTSUPP;
+
+	nvdimm_bus_lock(&nvdimm_bus->dev);
+	mutex_lock(&nvdimm->key_mutex);
+	if (atomic_read(&nvdimm->busy)) {
+		dev_warn(dev, "Unable to secure erase while DIMM active.\n");
+		rc = -EBUSY;
+		goto out;
+	}
+
+	if (dev_get_drvdata(dev)) {
+		dev_warn(dev, "Unable to secure erase while DIMM enabled.\n");
+		rc = -EBUSY;
+		goto out;
+	}
+
+	if (nvdimm->state == NVDIMM_SECURITY_UNSUPPORTED) {
+		dev_warn(dev, "Attempt to secure erase in wrong state.\n");
+		rc = -EOPNOTSUPP;
+		goto out;
+	}
+
+	/* look for a key from cached key if exists */
+	key = nvdimm_get_and_verify_key(dev, keyid);
+	if (IS_ERR(key)) {
+		dev_dbg(dev, "Unable to get and verify key\n");
+		rc = PTR_ERR(key);
+		goto out;
+	}
+	if (!key) {
+		dev_dbg(dev, "No cached key found\n");
+		/* get old user key */
+		key = nvdimm_lookup_user_key(dev, keyid);
+		if (!key) {
+			dev_dbg(dev, "Unable to retrieve user key: %#x\n",
+					keyid);
+			rc = -ENOKEY;
+			goto out;
+		}
+		is_userkey = true;
+	}
+
+	down_read(&key->sem);
+	payload = key->payload.data[0];
+	rc = nvdimm->security_ops->erase(nvdimm_bus, nvdimm,
+			(void *)payload->data);
+	up_read(&key->sem);
+
+	/* remove key since secure erase kills the passphrase */
+	if (!is_userkey) {
+		key_invalidate(key);
+		nvdimm->key = NULL;
+	}
+	key_put(key);
+
+ out:
+	mutex_unlock(&nvdimm->key_mutex);
+	nvdimm_bus_unlock(&nvdimm_bus->dev);
+	nvdimm_security_get_state(dev);
+	return rc;
+}
+
 int nvdimm_security_freeze_lock(struct device *dev)
 {
 	struct nvdimm *nvdimm = to_nvdimm(dev);
diff --git a/drivers/nvdimm/nd.h b/drivers/nvdimm/nd.h
index d89fe1423f9a..08e442632a2d 100644
--- a/drivers/nvdimm/nd.h
+++ b/drivers/nvdimm/nd.h
@@ -432,6 +432,7 @@ int nvdimm_security_change_key(struct device *dev, 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);
 #else
 static inline struct key *nvdimm_get_key(struct device *dev)
 {
@@ -464,5 +465,11 @@ static inline int nvdimm_security_freeze_lock(struct device *dev)
 {
 	return -EOPNOTSUPP;
 }
+
+static inline int nvdimm_security_erase(struct device *dev,
+		unsigned int keyid)
+{
+	return -EOPNOTSUPP;
+}
 #endif
 #endif /* __ND_H__ */
diff --git a/include/linux/libnvdimm.h b/include/linux/libnvdimm.h
index bcab42caa948..0d85e092a6dd 100644
--- a/include/linux/libnvdimm.h
+++ b/include/linux/libnvdimm.h
@@ -187,6 +187,9 @@ struct nvdimm_security_ops {
 			const struct nvdimm_key_data *nkey);
 	int (*freeze_lock)(struct nvdimm_bus *nvdimm_bus,
 			struct nvdimm *nvdimm);
+	int (*erase)(struct nvdimm_bus *nvdimm_bus,
+			struct nvdimm *nvdimm,
+			const struct nvdimm_key_data *nkey);
 };
 
 void badrange_init(struct badrange *badrange);

_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

^ permalink raw reply related	[flat|nested] 17+ messages in thread

* [PATCH v12 09/12] nfit_test: add test support for Intel nvdimm security DSMs
  2018-10-08 23:55 [PATCH v12 00/12] Adding security support for nvdimm Dave Jiang
                   ` (7 preceding siblings ...)
  2018-10-08 23:56 ` [PATCH v12 08/12] nfit/libnvdimm: add support for issue secure erase DSM " Dave Jiang
@ 2018-10-08 23:56 ` Dave Jiang
  2018-10-08 23:56 ` [PATCH v12 10/12] libnvdimm: add documentation for nvdimm security support Dave Jiang
                   ` (3 subsequent siblings)
  12 siblings, 0 replies; 17+ messages in thread
From: Dave Jiang @ 2018-10-08 23:56 UTC (permalink / raw)
  To: dan.j.williams
  Cc: alison.schofield, keescook, linux-nvdimm, ebiggers3, dhowells, keyrings

Add nfit_test support for DSM functions "Get Security State",
"Set Passphrase", "Disable Passphrase", "Unlock Unit", "Freeze Lock",
and "Secure Erase" for the fake DIMMs.

Also adding a sysfs knob in order to put the DIMMs in "locked" state. The
order of testing DIMM unlocking would be.
1a. Disable DIMM X.
1b. Set Passphrase to DIMM X.
2. Write to
/sys/devices/platform/nfit_test.0/nfit_test_dimm/test_dimmX/lock_dimm
3. Renable DIMM X
4. Check DIMM X state via sysfs "security" attribute for nmemX.

Signed-off-by: Dave Jiang <dave.jiang@intel.com>
Reviewed-by: Dan Williams <dan.j.williams@intel.com>
Signed-off-by: Dan Williams <dan.j.williams@intel.com>
---
 drivers/nvdimm/dimm_devs.c       |    2 
 drivers/nvdimm/nd.h              |    2 
 tools/testing/nvdimm/Kbuild      |    1 
 tools/testing/nvdimm/dimm_devs.c |   39 ++++++++
 tools/testing/nvdimm/test/nfit.c |  185 ++++++++++++++++++++++++++++++++++++++
 5 files changed, 228 insertions(+), 1 deletion(-)
 create mode 100644 tools/testing/nvdimm/dimm_devs.c

diff --git a/drivers/nvdimm/dimm_devs.c b/drivers/nvdimm/dimm_devs.c
index 1421404a687f..9e71e9497536 100644
--- a/drivers/nvdimm/dimm_devs.c
+++ b/drivers/nvdimm/dimm_devs.c
@@ -390,7 +390,7 @@ static ssize_t available_slots_show(struct device *dev,
 static DEVICE_ATTR_RO(available_slots);
 
 #ifdef CONFIG_NVDIMM_SECURITY
-static ssize_t security_show(struct device *dev,
+__weak ssize_t security_show(struct device *dev,
 		struct device_attribute *attr, char *buf)
 {
 	struct nvdimm *nvdimm = to_nvdimm(dev);
diff --git a/drivers/nvdimm/nd.h b/drivers/nvdimm/nd.h
index 08e442632a2d..75ff9b327543 100644
--- a/drivers/nvdimm/nd.h
+++ b/drivers/nvdimm/nd.h
@@ -433,6 +433,8 @@ int nvdimm_security_change_key(struct device *dev, unsigned int old_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);
+ssize_t security_show(struct device *dev, struct device_attribute *attr,
+		char *buf);
 #else
 static inline struct key *nvdimm_get_key(struct device *dev)
 {
diff --git a/tools/testing/nvdimm/Kbuild b/tools/testing/nvdimm/Kbuild
index 80cd0b0399ec..379da8312dc2 100644
--- a/tools/testing/nvdimm/Kbuild
+++ b/tools/testing/nvdimm/Kbuild
@@ -79,6 +79,7 @@ libnvdimm-$(CONFIG_ND_CLAIM) += $(NVDIMM_SRC)/claim.o
 libnvdimm-$(CONFIG_BTT) += $(NVDIMM_SRC)/btt_devs.o
 libnvdimm-$(CONFIG_NVDIMM_PFN) += $(NVDIMM_SRC)/pfn_devs.o
 libnvdimm-$(CONFIG_NVDIMM_DAX) += $(NVDIMM_SRC)/dax_devs.o
+libnvdimm-y += dimm_devs.o
 libnvdimm-y += libnvdimm_test.o
 libnvdimm-y += config_check.o
 libnvdimm-$(CONFIG_NVDIMM_SECURITY) += $(NVDIMM_SRC)/dimm_devs_security.o
diff --git a/tools/testing/nvdimm/dimm_devs.c b/tools/testing/nvdimm/dimm_devs.c
new file mode 100644
index 000000000000..f6717312dbb2
--- /dev/null
+++ b/tools/testing/nvdimm/dimm_devs.c
@@ -0,0 +1,39 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/* Copyright Intel Corp. 2018 */
+#include <linux/init.h>
+#include <linux/module.h>
+#include <linux/moduleparam.h>
+#include <linux/nd.h>
+#include "pmem.h"
+#include "pfn.h"
+#include "nd.h"
+#include "nd-core.h"
+
+ssize_t security_show(struct device *dev,
+		struct device_attribute *attr, char *buf)
+{
+	struct nvdimm *nvdimm = to_nvdimm(dev);
+
+	/*
+	 * For the test version we need to poll the "hardware" in order
+	 * to get the updated status for unlock testing.
+	 */
+	nvdimm_security_get_state(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;
+}
+
diff --git a/tools/testing/nvdimm/test/nfit.c b/tools/testing/nvdimm/test/nfit.c
index cffc2c5a778d..1d7ae7aecb46 100644
--- a/tools/testing/nvdimm/test/nfit.c
+++ b/tools/testing/nvdimm/test/nfit.c
@@ -24,6 +24,7 @@
 #include <linux/list.h>
 #include <linux/slab.h>
 #include <nd-core.h>
+#include <intel.h>
 #include <nfit.h>
 #include <nd.h>
 #include "nfit_test.h"
@@ -139,8 +140,16 @@ static u32 handle[] = {
 	[6] = NFIT_DIMM_HANDLE(1, 0, 0, 0, 1),
 };
 
+/*
+ * State that needs to be maintained for sysfs where we can't reliably
+ * look up our nfit_test relative dimm index.
+ */
 static unsigned long dimm_fail_cmd_flags[NUM_DCR];
 static int dimm_fail_cmd_code[NUM_DCR];
+struct nfit_test_sec {
+	u8 state;
+	u8 passphrase[32];
+} dimm_sec_info[NUM_DCR];
 
 static const struct nd_intel_smart smart_def = {
 	.flags = ND_INTEL_SMART_HEALTH_VALID
@@ -931,6 +940,138 @@ static int override_return_code(int dimm, unsigned int func, int rc)
 	return rc;
 }
 
+static int nd_intel_test_cmd_security_status(struct nfit_test *t,
+		struct nd_intel_get_security_state *nd_cmd,
+		unsigned int buf_len, int dimm)
+{
+	struct device *dev = &t->pdev.dev;
+	struct nfit_test_sec *sec = &dimm_sec_info[dimm];
+
+	nd_cmd->status = 0;
+	nd_cmd->state = sec->state;
+	dev_dbg(dev, "security state (%#x) returned\n", nd_cmd->state);
+
+	return 0;
+}
+
+static int nd_intel_test_cmd_unlock_unit(struct nfit_test *t,
+		struct nd_intel_unlock_unit *nd_cmd,
+		unsigned int buf_len, int dimm)
+{
+	struct device *dev = &t->pdev.dev;
+	struct nfit_test_sec *sec = &dimm_sec_info[dimm];
+
+	if (!(sec->state & ND_INTEL_SEC_STATE_LOCKED) ||
+			(sec->state & ND_INTEL_SEC_STATE_FROZEN)) {
+		nd_cmd->status = ND_INTEL_STATUS_INVALID_STATE;
+		dev_dbg(dev, "unlock unit: invalid state: %#x\n",
+				sec->state);
+	} else if (memcmp(nd_cmd->passphrase, sec->passphrase,
+				ND_INTEL_PASSPHRASE_SIZE) != 0) {
+		nd_cmd->status = ND_INTEL_STATUS_INVALID_PASS;
+		dev_dbg(dev, "unlock unit: invalid passphrase\n");
+	} else {
+		nd_cmd->status = 0;
+		sec->state = ND_INTEL_SEC_STATE_ENABLED;
+		dev_dbg(dev, "Unit unlocked\n");
+	}
+
+	dev_dbg(dev, "unlocking status returned: %#x\n", nd_cmd->status);
+	return 0;
+}
+
+static int nd_intel_test_cmd_set_pass(struct nfit_test *t,
+		struct nd_intel_set_passphrase *nd_cmd,
+		unsigned int buf_len, int dimm)
+{
+	struct device *dev = &t->pdev.dev;
+	struct nfit_test_sec *sec = &dimm_sec_info[dimm];
+
+	if (sec->state & ND_INTEL_SEC_STATE_FROZEN) {
+		nd_cmd->status = ND_INTEL_STATUS_INVALID_STATE;
+		dev_dbg(dev, "set passphrase: wrong security state\n");
+	} else if (memcmp(nd_cmd->old_pass, sec->passphrase,
+				ND_INTEL_PASSPHRASE_SIZE) != 0) {
+		nd_cmd->status = ND_INTEL_STATUS_INVALID_PASS;
+		dev_dbg(dev, "set passphrase: wrong passphrase\n");
+	} else {
+		memcpy(sec->passphrase, nd_cmd->new_pass,
+				ND_INTEL_PASSPHRASE_SIZE);
+		sec->state |= ND_INTEL_SEC_STATE_ENABLED;
+		nd_cmd->status = 0;
+		dev_dbg(dev, "passphrase updated\n");
+	}
+
+	return 0;
+}
+
+static int nd_intel_test_cmd_freeze_lock(struct nfit_test *t,
+		struct nd_intel_freeze_lock *nd_cmd,
+		unsigned int buf_len, int dimm)
+{
+	struct device *dev = &t->pdev.dev;
+	struct nfit_test_sec *sec = &dimm_sec_info[dimm];
+
+	if (!(sec->state & ND_INTEL_SEC_STATE_ENABLED)) {
+		nd_cmd->status = ND_INTEL_STATUS_INVALID_STATE;
+		dev_dbg(dev, "freeze lock: wrong security state\n");
+	} else {
+		sec->state |= ND_INTEL_SEC_STATE_FROZEN;
+		nd_cmd->status = 0;
+		dev_dbg(dev, "security frozen\n");
+	}
+
+	return 0;
+}
+
+static int nd_intel_test_cmd_disable_pass(struct nfit_test *t,
+		struct nd_intel_disable_passphrase *nd_cmd,
+		unsigned int buf_len, int dimm)
+{
+	struct device *dev = &t->pdev.dev;
+	struct nfit_test_sec *sec = &dimm_sec_info[dimm];
+
+	if (!(sec->state & ND_INTEL_SEC_STATE_ENABLED) ||
+			(sec->state & ND_INTEL_SEC_STATE_FROZEN)) {
+		nd_cmd->status = ND_INTEL_STATUS_INVALID_STATE;
+		dev_dbg(dev, "disable passphrase: wrong security state\n");
+	} else if (memcmp(nd_cmd->passphrase, sec->passphrase,
+				ND_INTEL_PASSPHRASE_SIZE) != 0) {
+		nd_cmd->status = ND_INTEL_STATUS_INVALID_PASS;
+		dev_dbg(dev, "disable passphrase: wrong passphrase\n");
+	} else {
+		memset(sec->passphrase, 0, ND_INTEL_PASSPHRASE_SIZE);
+		sec->state = 0;
+		dev_dbg(dev, "disable passphrase: done\n");
+	}
+
+	return 0;
+}
+
+static int nd_intel_test_cmd_secure_erase(struct nfit_test *t,
+		struct nd_intel_secure_erase *nd_cmd,
+		unsigned int buf_len, int dimm)
+{
+	struct device *dev = &t->pdev.dev;
+	struct nfit_test_sec *sec = &dimm_sec_info[dimm];
+
+	if (!(sec->state & ND_INTEL_SEC_STATE_ENABLED) ||
+			(sec->state & ND_INTEL_SEC_STATE_FROZEN)) {
+		nd_cmd->status = ND_INTEL_STATUS_INVALID_STATE;
+		dev_dbg(dev, "secure erase: wrong security state\n");
+	} else if (memcmp(nd_cmd->passphrase, sec->passphrase,
+				ND_INTEL_PASSPHRASE_SIZE) != 0) {
+		nd_cmd->status = ND_INTEL_STATUS_INVALID_PASS;
+		dev_dbg(dev, "secure erase: wrong passphrase\n");
+	} else {
+		memset(sec->passphrase, 0, ND_INTEL_PASSPHRASE_SIZE);
+		sec->state = 0;
+		dev_dbg(dev, "secure erase: done\n");
+	}
+
+	return 0;
+}
+
 static int get_dimm(struct nfit_mem *nfit_mem, unsigned int func)
 {
 	int i;
@@ -978,6 +1119,30 @@ static int nfit_test_ctl(struct nvdimm_bus_descriptor *nd_desc,
 				return i;
 
 			switch (func) {
+			case NVDIMM_INTEL_GET_SECURITY_STATE:
+				rc = nd_intel_test_cmd_security_status(t,
+						buf, buf_len, i);
+				break;
+			case NVDIMM_INTEL_UNLOCK_UNIT:
+				rc = nd_intel_test_cmd_unlock_unit(t,
+						buf, buf_len, i);
+				break;
+			case NVDIMM_INTEL_SET_PASSPHRASE:
+				rc = nd_intel_test_cmd_set_pass(t,
+						buf, buf_len, i);
+				break;
+			case NVDIMM_INTEL_DISABLE_PASSPHRASE:
+				rc = nd_intel_test_cmd_disable_pass(t,
+						buf, buf_len, i);
+				break;
+			case NVDIMM_INTEL_FREEZE_LOCK:
+				rc = nd_intel_test_cmd_freeze_lock(t,
+						buf, buf_len, i);
+				break;
+			case NVDIMM_INTEL_SECURE_ERASE:
+				rc = nd_intel_test_cmd_secure_erase(t,
+						buf, buf_len, i);
+				break;
 			case ND_INTEL_ENABLE_LSS_STATUS:
 				rc = nd_intel_test_cmd_set_lss_status(t,
 						buf, buf_len);
@@ -1311,10 +1476,22 @@ static ssize_t fail_cmd_code_store(struct device *dev, struct device_attribute *
 }
 static DEVICE_ATTR_RW(fail_cmd_code);
 
+static ssize_t lock_dimm_store(struct device *dev,
+		struct device_attribute *attr, const char *buf, size_t size)
+{
+	int dimm = dimm_name_to_id(dev);
+	struct nfit_test_sec *sec = &dimm_sec_info[dimm];
+
+	sec->state = ND_INTEL_SEC_STATE_ENABLED | ND_INTEL_SEC_STATE_LOCKED;
+	return size;
+}
+static DEVICE_ATTR_WO(lock_dimm);
+
 static struct attribute *nfit_test_dimm_attributes[] = {
 	&dev_attr_fail_cmd.attr,
 	&dev_attr_fail_cmd_code.attr,
 	&dev_attr_handle.attr,
+	&dev_attr_lock_dimm.attr,
 	NULL,
 };
 
@@ -2193,6 +2370,14 @@ static void nfit_test0_setup(struct nfit_test *t)
 	set_bit(ND_INTEL_FW_FINISH_UPDATE, &acpi_desc->dimm_cmd_force_en);
 	set_bit(ND_INTEL_FW_FINISH_QUERY, &acpi_desc->dimm_cmd_force_en);
 	set_bit(ND_INTEL_ENABLE_LSS_STATUS, &acpi_desc->dimm_cmd_force_en);
+	set_bit(NVDIMM_INTEL_GET_SECURITY_STATE,
+			&acpi_desc->dimm_cmd_force_en);
+	set_bit(NVDIMM_INTEL_SET_PASSPHRASE, &acpi_desc->dimm_cmd_force_en);
+	set_bit(NVDIMM_INTEL_DISABLE_PASSPHRASE,
+			&acpi_desc->dimm_cmd_force_en);
+	set_bit(NVDIMM_INTEL_UNLOCK_UNIT, &acpi_desc->dimm_cmd_force_en);
+	set_bit(NVDIMM_INTEL_FREEZE_LOCK, &acpi_desc->dimm_cmd_force_en);
+	set_bit(NVDIMM_INTEL_SECURE_ERASE, &acpi_desc->dimm_cmd_force_en);
 }
 
 static void nfit_test1_setup(struct nfit_test *t)

_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

^ permalink raw reply related	[flat|nested] 17+ messages in thread

* [PATCH v12 10/12] libnvdimm: add documentation for nvdimm security support
  2018-10-08 23:55 [PATCH v12 00/12] Adding security support for nvdimm Dave Jiang
                   ` (8 preceding siblings ...)
  2018-10-08 23:56 ` [PATCH v12 09/12] nfit_test: add test support for Intel nvdimm security DSMs Dave Jiang
@ 2018-10-08 23:56 ` Dave Jiang
  2018-10-08 23:56 ` [PATCH v12 11/12] libnvdimm: Drop nvdimm_bus from security_ops interface Dave Jiang
                   ` (2 subsequent siblings)
  12 siblings, 0 replies; 17+ messages in thread
From: Dave Jiang @ 2018-10-08 23:56 UTC (permalink / raw)
  To: dan.j.williams
  Cc: alison.schofield, keescook, linux-nvdimm, ebiggers3, dhowells, keyrings

Add theory of operation for the security support that's going into
libnvdimm.

Signed-off-by: Dave Jiang <dave.jiang@intel.com>
Signed-off-by: Dan Williams <dan.j.williams@intel.com>
---
 Documentation/nvdimm/security.txt |   99 +++++++++++++++++++++++++++++++++++++
 1 file changed, 99 insertions(+)
 create mode 100644 Documentation/nvdimm/security.txt

diff --git a/Documentation/nvdimm/security.txt b/Documentation/nvdimm/security.txt
new file mode 100644
index 000000000000..50cbb6cb96a1
--- /dev/null
+++ b/Documentation/nvdimm/security.txt
@@ -0,0 +1,99 @@
+NVDIMM SECURITY
+===============
+
+1. Introduction
+---------------
+
+With the introduction of Intel DSM v1.7 specification [1], security DSMs are
+introduced. The spec added the following security DSMs: "get security state",
+"set passphrase", "disable passphrase", "unlock unit", "freeze lock",
+"secure erase", and "overwrite". A security_ops data structure has been
+added to struct dimm in order to support the security operations and generic
+APIs are exposed to allow vendor neutral operations.
+
+2. Sysfs Interface
+------------------
+The "security" sysfs attribute is provided in the nvdimm sysfs directory. For
+example:
+/sys/devices/LNXSYSTM:00/LNXSYBUS:00/ACPI0012:00/ndbus0/nmem0/security
+
+The "show" function of that attribute will display the security state for
+that DIMM. The following states are available: disabled, unlocked, locked,
+frozen, and unsupported.
+
+The "store" function takes several commands when the attribute is written to
+in order to support some of the security functionalities:
+update <old_keyid> <new_keyid> - enable security. Add or update current key.
+disable <old_keyid> - disable enabled security and remove key.
+freeze - freeze changing of security states.
+erase <old_keyid> - generate new ecryption key for DIMM and crypto-scrambles
+	all existing user data.
+
+3. Key Management
+-----------------
+
+The key is associted to the payload by the DIMM id. For example:
+# cat /sys/devices/LNXSYSTM:00/LNXSYBUS:00/ACPI0012:00/ndbus0/nmem0/nfit/id
+8089-a2-1740-00000133
+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
+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:
+nvdimm:<bus-provider-specific-unique-id>
+
+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:
+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.
+
+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
+be injected with a key description that does not have the "nvdimm:" prefix.
+
+6. Freeze
+---------
+The freeze operation does not require any keys. The security config can be
+frozen by a user with root privelege.
+
+7. Disable
+----------
+The security disable command format is:
+disable <old 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
+its keyid should be passed in via sysfs.
+
+8. Secure Erase
+---------------
+The command format for doing a secure erase is:
+erase <old 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
+its keyid should be passed in via sysfs.
+
+[1]: http://pmem.io/documents/NVDIMM_DSM_Interface-V1.7.pdf
+[2]: http://www.t13.org/documents/UploadedDocuments/docs2006/e05179r4-ACS-SecurityClarifications.pdf

_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

^ permalink raw reply related	[flat|nested] 17+ messages in thread

* [PATCH v12 11/12] libnvdimm: Drop nvdimm_bus from security_ops interface
  2018-10-08 23:55 [PATCH v12 00/12] Adding security support for nvdimm Dave Jiang
                   ` (9 preceding siblings ...)
  2018-10-08 23:56 ` [PATCH v12 10/12] libnvdimm: add documentation for nvdimm security support Dave Jiang
@ 2018-10-08 23:56 ` Dave Jiang
  2018-10-08 23:56 ` [PATCH v12 12/12] acpi, nfit: Move acpi_nfit_get_security_ops() to generic location Dave Jiang
  2018-10-10  1:35 ` [PATCH v12 00/12] Adding security support for nvdimm Williams, Dan J
  12 siblings, 0 replies; 17+ messages in thread
From: Dave Jiang @ 2018-10-08 23:56 UTC (permalink / raw)
  To: dan.j.williams
  Cc: alison.schofield, keescook, linux-nvdimm, ebiggers3, dhowells, keyrings

From: Dan Williams <dan.j.williams@intel.com>

All that is missing is a helper to go from an nvdimm to its bus. With
the new nvdimm_to_bus() and nvdimm_ctl() helpers some boilerplate can be
consolidated.

Reviewed-by: Dave Jiang <dave.jiang@intel.com>
Signed-off-by: Dan Williams <dan.j.williams@intel.com>
---
 drivers/acpi/nfit/intel.c           |   46 ++++++++++++-----------------------
 drivers/nvdimm/bus.c                |    6 +++++
 drivers/nvdimm/dimm_devs_security.c |   24 +++++++-----------
 include/linux/libnvdimm.h           |   28 ++++++++++++---------
 4 files changed, 47 insertions(+), 57 deletions(-)

diff --git a/drivers/acpi/nfit/intel.c b/drivers/acpi/nfit/intel.c
index 419a7d54d4e8..70bccb0c57e2 100644
--- a/drivers/acpi/nfit/intel.c
+++ b/drivers/acpi/nfit/intel.c
@@ -18,10 +18,9 @@
 #include "intel.h"
 #include "nfit.h"
 
-static int intel_dimm_security_erase(struct nvdimm_bus *nvdimm_bus,
-		struct nvdimm *nvdimm, const struct nvdimm_key_data *nkey)
+static int intel_dimm_security_erase(struct nvdimm *nvdimm,
+		const struct nvdimm_key_data *nkey)
 {
-	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 {
@@ -47,8 +46,7 @@ static int intel_dimm_security_erase(struct nvdimm_bus *nvdimm_bus,
 	wbinvd_on_all_cpus();
 	memcpy(nd_cmd.cmd.passphrase, nkey->data,
 			sizeof(nd_cmd.cmd.passphrase));
-	rc = nd_desc->ndctl(nd_desc, nvdimm, ND_CMD_CALL, &nd_cmd,
-			sizeof(nd_cmd), &cmd_rc);
+	rc = nvdimm_ctl(nvdimm, ND_CMD_CALL, &nd_cmd, sizeof(nd_cmd), &cmd_rc);
 	if (rc < 0)
 		goto out;
 	if (cmd_rc < 0) {
@@ -75,10 +73,8 @@ static int intel_dimm_security_erase(struct nvdimm_bus *nvdimm_bus,
 	return rc;
 }
 
-static int intel_dimm_security_freeze_lock(struct nvdimm_bus *nvdimm_bus,
-		struct nvdimm *nvdimm)
+static int intel_dimm_security_freeze_lock(struct nvdimm *nvdimm)
 {
-	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 {
@@ -100,8 +96,7 @@ static int intel_dimm_security_freeze_lock(struct nvdimm_bus *nvdimm_bus,
 	if (!test_bit(NVDIMM_INTEL_FREEZE_LOCK, &nfit_mem->dsm_mask))
 		return -ENOTTY;
 
-	rc = nd_desc->ndctl(nd_desc, nvdimm, ND_CMD_CALL, &nd_cmd,
-			sizeof(nd_cmd), &cmd_rc);
+	rc = nvdimm_ctl(nvdimm, ND_CMD_CALL, &nd_cmd, sizeof(nd_cmd), &cmd_rc);
 	if (rc < 0)
 		goto out;
 	if (cmd_rc < 0) {
@@ -122,10 +117,9 @@ static int intel_dimm_security_freeze_lock(struct nvdimm_bus *nvdimm_bus,
 	return rc;
 }
 
-static int intel_dimm_security_disable(struct nvdimm_bus *nvdimm_bus,
-		struct nvdimm *nvdimm, const struct nvdimm_key_data *nkey)
+static int intel_dimm_security_disable(struct nvdimm *nvdimm,
+		const struct nvdimm_key_data *nkey)
 {
-	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 {
@@ -149,8 +143,7 @@ static int intel_dimm_security_disable(struct nvdimm_bus *nvdimm_bus,
 
 	memcpy(nd_cmd.cmd.passphrase, nkey->data,
 			sizeof(nd_cmd.cmd.passphrase));
-	rc = nd_desc->ndctl(nd_desc, nvdimm, ND_CMD_CALL, &nd_cmd,
-			sizeof(nd_cmd), &cmd_rc);
+	rc = nvdimm_ctl(nvdimm, ND_CMD_CALL, &nd_cmd, sizeof(nd_cmd), &cmd_rc);
 	if (rc < 0)
 		goto out;
 	if (cmd_rc < 0) {
@@ -182,12 +175,10 @@ static int intel_dimm_security_disable(struct nvdimm_bus *nvdimm_bus,
  * 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,
+static int intel_dimm_security_update_passphrase(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 {
@@ -216,8 +207,7 @@ static int intel_dimm_security_update_passphrase(
 		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);
+	rc = nvdimm_ctl(nvdimm, ND_CMD_CALL, &nd_cmd, sizeof(nd_cmd), &cmd_rc);
 	if (rc < 0)
 		goto out;
 	if (cmd_rc < 0) {
@@ -241,10 +231,9 @@ static int intel_dimm_security_update_passphrase(
 	return rc;
 }
 
-static int intel_dimm_security_unlock(struct nvdimm_bus *nvdimm_bus,
-		struct nvdimm *nvdimm, const struct nvdimm_key_data *nkey)
+static int intel_dimm_security_unlock(struct nvdimm *nvdimm,
+		const struct nvdimm_key_data *nkey)
 {
-	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 {
@@ -268,8 +257,7 @@ static int intel_dimm_security_unlock(struct nvdimm_bus *nvdimm_bus,
 
 	memcpy(nd_cmd.cmd.passphrase, nkey->data,
 			sizeof(nd_cmd.cmd.passphrase));
-	rc = nd_desc->ndctl(nd_desc, nvdimm, ND_CMD_CALL, &nd_cmd,
-			sizeof(nd_cmd), &cmd_rc);
+	rc = nvdimm_ctl(nvdimm, ND_CMD_CALL, &nd_cmd, sizeof(nd_cmd), &cmd_rc);
 	if (rc < 0)
 		goto out;
 	if (cmd_rc < 0) {
@@ -300,10 +288,9 @@ static int intel_dimm_security_unlock(struct nvdimm_bus *nvdimm_bus,
 	return rc;
 }
 
-static int intel_dimm_security_state(struct nvdimm_bus *nvdimm_bus,
-		struct nvdimm *nvdimm, enum nvdimm_security_state *state)
+static int intel_dimm_security_state(struct nvdimm *nvdimm,
+		enum nvdimm_security_state *state)
 {
-	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 {
@@ -331,8 +318,7 @@ static int intel_dimm_security_state(struct nvdimm_bus *nvdimm_bus,
 	}
 
 	*state = NVDIMM_SECURITY_DISABLED;
-	rc = nd_desc->ndctl(nd_desc, nvdimm, ND_CMD_CALL, &nd_cmd,
-			sizeof(nd_cmd), &cmd_rc);
+	rc = nvdimm_ctl(nvdimm, ND_CMD_CALL, &nd_cmd, sizeof(nd_cmd), &cmd_rc);
 	if (rc < 0)
 		goto out;
 	if (cmd_rc < 0) {
diff --git a/drivers/nvdimm/bus.c b/drivers/nvdimm/bus.c
index 9743d8083538..eae17d8ee539 100644
--- a/drivers/nvdimm/bus.c
+++ b/drivers/nvdimm/bus.c
@@ -331,6 +331,12 @@ struct nvdimm_bus *to_nvdimm_bus(struct device *dev)
 }
 EXPORT_SYMBOL_GPL(to_nvdimm_bus);
 
+struct nvdimm_bus *nvdimm_to_bus(struct nvdimm *nvdimm)
+{
+	return to_nvdimm_bus(nvdimm->dev.parent);
+}
+EXPORT_SYMBOL_GPL(nvdimm_to_bus);
+
 struct nvdimm_bus *nvdimm_bus_register(struct device *parent,
 		struct nvdimm_bus_descriptor *nd_desc)
 {
diff --git a/drivers/nvdimm/dimm_devs_security.c b/drivers/nvdimm/dimm_devs_security.c
index 4a8bd4d94515..e3be3a238765 100644
--- a/drivers/nvdimm/dimm_devs_security.c
+++ b/drivers/nvdimm/dimm_devs_security.c
@@ -155,13 +155,11 @@ struct key *nvdimm_get_and_verify_key(struct device *dev,
 int nvdimm_security_get_state(struct device *dev)
 {
 	struct nvdimm *nvdimm = to_nvdimm(dev);
-	struct nvdimm_bus *nvdimm_bus = walk_to_nvdimm_bus(dev);
 
 	if (!nvdimm->security_ops)
 		return 0;
 
-	return nvdimm->security_ops->state(nvdimm_bus, nvdimm,
-			&nvdimm->state);
+	return nvdimm->security_ops->state(nvdimm, &nvdimm->state);
 }
 
 int nvdimm_security_erase(struct device *dev, unsigned int keyid)
@@ -218,8 +216,8 @@ int nvdimm_security_erase(struct device *dev, unsigned int keyid)
 
 	down_read(&key->sem);
 	payload = key->payload.data[0];
-	rc = nvdimm->security_ops->erase(nvdimm_bus, nvdimm,
-			(void *)payload->data);
+	rc = nvdimm->security_ops->erase(nvdimm,
+			(const struct nvdimm_key_data *)payload->data);
 	up_read(&key->sem);
 
 	/* remove key since secure erase kills the passphrase */
@@ -239,7 +237,6 @@ int nvdimm_security_erase(struct device *dev, unsigned int keyid)
 int nvdimm_security_freeze_lock(struct device *dev)
 {
 	struct nvdimm *nvdimm = to_nvdimm(dev);
-	struct nvdimm_bus *nvdimm_bus = walk_to_nvdimm_bus(dev);
 	int rc;
 
 	if (!nvdimm->security_ops)
@@ -248,7 +245,7 @@ int nvdimm_security_freeze_lock(struct device *dev)
 	if (nvdimm->state == NVDIMM_SECURITY_UNSUPPORTED)
 		return -EOPNOTSUPP;
 
-	rc = nvdimm->security_ops->freeze_lock(nvdimm_bus, nvdimm);
+	rc = nvdimm->security_ops->freeze_lock(nvdimm);
 	if (rc < 0)
 		return rc;
 
@@ -259,7 +256,6 @@ int nvdimm_security_freeze_lock(struct device *dev)
 int nvdimm_security_disable(struct device *dev, unsigned int keyid)
 {
 	struct nvdimm *nvdimm = to_nvdimm(dev);
-	struct nvdimm_bus *nvdimm_bus = walk_to_nvdimm_bus(dev);
 	struct key *key;
 	int rc;
 	struct user_key_payload *payload;
@@ -291,8 +287,8 @@ int nvdimm_security_disable(struct device *dev, unsigned int keyid)
 	down_read(&key->sem);
 	payload = key->payload.data[0];
 
-	rc = nvdimm->security_ops->disable(nvdimm_bus, nvdimm,
-			(void *)payload->data);
+	rc = nvdimm->security_ops->disable(nvdimm,
+			(const struct nvdimm_key_data *)payload->data);
 	up_read(&key->sem);
 	if (rc < 0) {
 		dev_warn(dev, "unlock failed\n");
@@ -315,7 +311,6 @@ int nvdimm_security_disable(struct device *dev, unsigned int keyid)
 int nvdimm_security_unlock_dimm(struct device *dev)
 {
 	struct nvdimm *nvdimm = to_nvdimm(dev);
-	struct nvdimm_bus *nvdimm_bus = walk_to_nvdimm_bus(dev);
 	struct key *key;
 	struct user_key_payload *payload;
 	int rc;
@@ -352,8 +347,8 @@ int nvdimm_security_unlock_dimm(struct device *dev)
 	dev_dbg(dev, "%s: key: %#x\n", __func__, key_serial(key));
 	down_read(&key->sem);
 	payload = key->payload.data[0];
-	rc = nvdimm->security_ops->unlock(nvdimm_bus, nvdimm,
-			(const void *)payload->data);
+	rc = nvdimm->security_ops->unlock(nvdimm,
+			(const struct nvdimm_key_data *)payload->data);
 	up_read(&key->sem);
 
 	if (rc == 0) {
@@ -377,7 +372,6 @@ 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, *old_key;
 	int rc;
 	void *old_data = NULL, *new_data;
@@ -441,7 +435,7 @@ int nvdimm_security_change_key(struct device *dev,
 
 	new_data = payload->data;
 
-	rc = nvdimm->security_ops->change_key(nvdimm_bus, nvdimm, old_data,
+	rc = nvdimm->security_ops->change_key(nvdimm, old_data,
 			new_data);
 	/* copy new payload to old payload */
 	if (rc == 0) {
diff --git a/include/linux/libnvdimm.h b/include/linux/libnvdimm.h
index 0d85e092a6dd..c352b8195675 100644
--- a/include/linux/libnvdimm.h
+++ b/include/linux/libnvdimm.h
@@ -172,23 +172,17 @@ struct nvdimm_key_data {
 };
 
 struct nvdimm_security_ops {
-	int (*state)(struct nvdimm_bus *nvdimm_bus,
-			struct nvdimm *nvdimm,
+	int (*state)(struct nvdimm *nvdimm,
 			enum nvdimm_security_state *state);
-	int (*unlock)(struct nvdimm_bus *nvdimm_bus,
-			struct nvdimm *nvdimm,
+	int (*unlock)(struct nvdimm *nvdimm,
 			const struct nvdimm_key_data *nkey);
-	int (*change_key)(struct nvdimm_bus *nvdimm_bus,
-			struct nvdimm *nvdimm,
+	int (*change_key)(struct nvdimm *nvdimm,
 			const struct nvdimm_key_data *old_data,
 			const struct nvdimm_key_data *new_data);
-	int (*disable)(struct nvdimm_bus *nvdimm_bus,
-			struct nvdimm *nvdimm,
+	int (*disable)(struct nvdimm *nvdimm,
 			const struct nvdimm_key_data *nkey);
-	int (*freeze_lock)(struct nvdimm_bus *nvdimm_bus,
-			struct nvdimm *nvdimm);
-	int (*erase)(struct nvdimm_bus *nvdimm_bus,
-			struct nvdimm *nvdimm,
+	int (*freeze_lock)(struct nvdimm *nvdimm);
+	int (*erase)(struct nvdimm *nvdimm,
 			const struct nvdimm_key_data *nkey);
 };
 
@@ -202,6 +196,7 @@ struct nvdimm_bus *nvdimm_bus_register(struct device *parent,
 		struct nvdimm_bus_descriptor *nfit_desc);
 void nvdimm_bus_unregister(struct nvdimm_bus *nvdimm_bus);
 struct nvdimm_bus *to_nvdimm_bus(struct device *dev);
+struct nvdimm_bus *nvdimm_to_bus(struct nvdimm *nvdimm);
 struct nvdimm *to_nvdimm(struct device *dev);
 struct nd_region *to_nd_region(struct device *dev);
 struct device *nd_region_dev(struct nd_region *nd_region);
@@ -243,6 +238,15 @@ void nvdimm_flush(struct nd_region *nd_region);
 int nvdimm_has_flush(struct nd_region *nd_region);
 int nvdimm_has_cache(struct nd_region *nd_region);
 
+static inline int nvdimm_ctl(struct nvdimm *nvdimm, unsigned int cmd, void *buf,
+		unsigned int buf_len, int *cmd_rc)
+{
+	struct nvdimm_bus *nvdimm_bus = nvdimm_to_bus(nvdimm);
+	struct nvdimm_bus_descriptor *nd_desc = to_nd_desc(nvdimm_bus);
+
+	return nd_desc->ndctl(nd_desc, nvdimm, cmd, buf, buf_len, cmd_rc);
+}
+
 #ifdef CONFIG_ARCH_HAS_PMEM_API
 #define ARCH_MEMREMAP_PMEM MEMREMAP_WB
 void arch_wb_cache_pmem(void *addr, size_t size);

_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

^ permalink raw reply related	[flat|nested] 17+ messages in thread

* [PATCH v12 12/12] acpi, nfit: Move acpi_nfit_get_security_ops() to generic location
  2018-10-08 23:55 [PATCH v12 00/12] Adding security support for nvdimm Dave Jiang
                   ` (10 preceding siblings ...)
  2018-10-08 23:56 ` [PATCH v12 11/12] libnvdimm: Drop nvdimm_bus from security_ops interface Dave Jiang
@ 2018-10-08 23:56 ` Dave Jiang
  2018-10-10  1:35 ` [PATCH v12 00/12] Adding security support for nvdimm Williams, Dan J
  12 siblings, 0 replies; 17+ messages in thread
From: Dave Jiang @ 2018-10-08 23:56 UTC (permalink / raw)
  To: dan.j.williams
  Cc: alison.schofield, keescook, linux-nvdimm, ebiggers3, dhowells, keyrings

From: Dan Williams <dan.j.williams@intel.com>

In anticipation of other DIMMs adding security operations support, don't
require them to edit intel.h. Instead have intel.h optionally arrange
for intel_security_ops to be NULL or not based on build parameters.

Reviewed-by: Dave Jiang <dave.jiang@intel.com>
Signed-off-by: Dan Williams <dan.j.williams@intel.com>
---
 drivers/acpi/nfit/core.c  |   10 ++++++++++
 drivers/acpi/nfit/intel.c |    3 ++-
 drivers/acpi/nfit/intel.h |   18 +++---------------
 3 files changed, 15 insertions(+), 16 deletions(-)

diff --git a/drivers/acpi/nfit/core.c b/drivers/acpi/nfit/core.c
index e5f7c664a7c8..b7773c70ee81 100644
--- a/drivers/acpi/nfit/core.c
+++ b/drivers/acpi/nfit/core.c
@@ -1835,6 +1835,16 @@ static void shutdown_dimm_notify(void *data)
 	mutex_unlock(&acpi_desc->init_mutex);
 }
 
+static const struct nvdimm_security_ops *acpi_nfit_get_security_ops(int family)
+{
+        switch (family) {
+        case NVDIMM_FAMILY_INTEL:
+                return intel_security_ops;
+        default:
+                return NULL;
+        }
+}
+
 static int acpi_nfit_register_dimms(struct acpi_nfit_desc *acpi_desc)
 {
 	struct nfit_mem *nfit_mem;
diff --git a/drivers/acpi/nfit/intel.c b/drivers/acpi/nfit/intel.c
index 70bccb0c57e2..8d2413810a5f 100644
--- a/drivers/acpi/nfit/intel.c
+++ b/drivers/acpi/nfit/intel.c
@@ -358,7 +358,7 @@ static int intel_dimm_security_state(struct nvdimm *nvdimm,
 	return rc;
 }
 
-const struct nvdimm_security_ops intel_security_ops = {
+static 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,
@@ -366,3 +366,4 @@ const struct nvdimm_security_ops intel_security_ops = {
 	.freeze_lock = intel_dimm_security_freeze_lock,
 	.erase = intel_dimm_security_erase,
 };
+const struct nvdimm_security_ops *intel_security_ops = &__intel_security_ops;
diff --git a/drivers/acpi/nfit/intel.h b/drivers/acpi/nfit/intel.h
index f9ac718776ca..f831084f95fa 100644
--- a/drivers/acpi/nfit/intel.h
+++ b/drivers/acpi/nfit/intel.h
@@ -8,7 +8,7 @@
 
 #ifdef CONFIG_X86
 
-extern const struct nvdimm_security_ops intel_security_ops;
+extern const struct nvdimm_security_ops *intel_security_ops;
 
 #define ND_INTEL_STATUS_SIZE		4
 #define ND_INTEL_PASSPHRASE_SIZE	32
@@ -64,19 +64,7 @@ struct nd_intel_overwrite {
 struct nd_intel_query_overwrite {
 	u32 status;
 } __packed;
+#else /* CONFIG_X86 */
+#define intel_security_ops (NULL)
 #endif /* CONFIG_X86 */
-
-static inline const struct nvdimm_security_ops *
-acpi_nfit_get_security_ops(int family)
-{
-	switch (family) {
-#ifdef CONFIG_X86
-	case NVDIMM_FAMILY_INTEL:
-		return &intel_security_ops;
-#endif
-	default:
-		return NULL;
-	}
-}
-
 #endif

_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

^ permalink raw reply related	[flat|nested] 17+ messages in thread

* Re: [PATCH v12 04/12] nfit/libnvdimm: add unlock of nvdimm support for Intel DIMMs
  2018-10-08 23:55 ` [PATCH v12 04/12] nfit/libnvdimm: add unlock of nvdimm support for Intel DIMMs Dave Jiang
@ 2018-10-09 19:07   ` Dan Williams
  2018-10-09 19:45     ` Dave Jiang
  0 siblings, 1 reply; 17+ messages in thread
From: Dan Williams @ 2018-10-09 19:07 UTC (permalink / raw)
  To: Dave Jiang
  Cc: Schofield, Alison, Kees Cook, linux-nvdimm, Eric Biggers,
	David Howells, keyrings

On Mon, Oct 8, 2018 at 5:01 PM Dave Jiang <dave.jiang@intel.com> wrote:
>
> Add support to allow query the security status of the Intel nvdimms and
> also unlock the dimm via the kernel key management APIs. The passphrase is
> expected to be pulled from userspace through keyutils. Moving the Intel
> related bits to its own source file as well.
>
> Signed-off-by: Dave Jiang <dave.jiang@intel.com>
> Reviewed-by: Dan Williams <dan.j.williams@intel.com>
> Signed-off-by: Dan Williams <dan.j.williams@intel.com>
> ---
>  drivers/acpi/nfit/Makefile          |    1
>  drivers/acpi/nfit/core.c            |    3 -
>  drivers/acpi/nfit/intel.c           |  152 +++++++++++++++++++++++++++++++++++
>  drivers/acpi/nfit/intel.h           |   15 +++
>  drivers/nvdimm/Kconfig              |    5 +
>  drivers/nvdimm/Makefile             |    1
>  drivers/nvdimm/dimm.c               |    7 ++
>  drivers/nvdimm/dimm_devs.c          |   12 +++
>  drivers/nvdimm/dimm_devs_security.c |  129 ++++++++++++++++++++++++++++++
>  drivers/nvdimm/nd-core.h            |    5 +
>  drivers/nvdimm/nd.h                 |   21 +++++
>  include/linux/libnvdimm.h           |   27 ++++++
>  include/uapi/linux/ndctl.h          |    6 +
>  tools/testing/nvdimm/Kbuild         |    2
>  14 files changed, 383 insertions(+), 3 deletions(-)
>  create mode 100644 drivers/acpi/nfit/intel.c
>  create mode 100644 drivers/nvdimm/dimm_devs_security.c
>
> diff --git a/drivers/acpi/nfit/Makefile b/drivers/acpi/nfit/Makefile
> index a407e769f103..443c7ef4e6a6 100644
> --- a/drivers/acpi/nfit/Makefile
> +++ b/drivers/acpi/nfit/Makefile
> @@ -1,3 +1,4 @@
>  obj-$(CONFIG_ACPI_NFIT) := nfit.o
>  nfit-y := core.o
> +nfit-$(CONFIG_X86) += intel.o
>  nfit-$(CONFIG_X86_MCE) += mce.o
> diff --git a/drivers/acpi/nfit/core.c b/drivers/acpi/nfit/core.c
> index 94755a4b6327..e5f7c664a7c8 100644
> --- a/drivers/acpi/nfit/core.c
> +++ b/drivers/acpi/nfit/core.c
> @@ -1904,7 +1904,8 @@ static int acpi_nfit_register_dimms(struct acpi_nfit_desc *acpi_desc)
>                 nvdimm = nvdimm_create(acpi_desc->nvdimm_bus, nfit_mem,
>                                 acpi_nfit_dimm_attribute_groups,
>                                 flags, cmd_mask, flush ? flush->hint_count : 0,
> -                               nfit_mem->flush_wpq, &nfit_mem->id[0]);
> +                               nfit_mem->flush_wpq, &nfit_mem->id[0],
> +                               acpi_nfit_get_security_ops(nfit_mem->family));
>                 if (!nvdimm)
>                         return -ENOMEM;
>
> diff --git a/drivers/acpi/nfit/intel.c b/drivers/acpi/nfit/intel.c
> new file mode 100644
> index 000000000000..4bfc1c1da339
> --- /dev/null
> +++ b/drivers/acpi/nfit/intel.c
> @@ -0,0 +1,152 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/* Copyright(c) 2018 Intel Corporation. All rights reserved. */
> +/*
> + * Intel specific NFIT ops
> + */
> +#include <linux/libnvdimm.h>
> +#include <linux/module.h>
> +#include <linux/mutex.h>
> +#include <linux/ndctl.h>
> +#include <linux/sysfs.h>
> +#include <linux/delay.h>
> +#include <linux/acpi.h>
> +#include <linux/io.h>
> +#include <linux/nd.h>
> +#include <asm/cacheflush.h>
> +#include <asm/smp.h>
> +#include <acpi/nfit.h>
> +#include "intel.h"
> +#include "nfit.h"
> +
> +static int intel_dimm_security_unlock(struct nvdimm_bus *nvdimm_bus,
> +               struct nvdimm *nvdimm, const struct nvdimm_key_data *nkey)
> +{
> +       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_unlock_unit cmd;
> +       } nd_cmd = {
> +               .pkg = {
> +                       .nd_command = NVDIMM_INTEL_UNLOCK_UNIT,
> +                       .nd_family = NVDIMM_FAMILY_INTEL,
> +                       .nd_size_in = ND_INTEL_PASSPHRASE_SIZE,
> +                       .nd_size_out = ND_INTEL_STATUS_SIZE,
> +                       .nd_fw_size = ND_INTEL_STATUS_SIZE,
> +               },
> +               .cmd = {
> +                       .status = 0,
> +               },
> +       };
> +
> +       if (!test_bit(NVDIMM_INTEL_UNLOCK_UNIT, &nfit_mem->dsm_mask))
> +               return -ENOTTY;
> +
> +       memcpy(nd_cmd.cmd.passphrase, nkey->data,
> +                       sizeof(nd_cmd.cmd.passphrase));
> +       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;
> +       }
> +
> +       /*
> +        * TODO: define a cross arch wbinvd when/if NVDIMM_FAMILY_INTEL
> +        * support arrives on another arch.
> +        */
> +       /* DIMM unlocked, invalidate all CPU caches before we read it */
> +       wbinvd_on_all_cpus();
> +
> + out:
> +       return rc;
> +}
> +
> +static int intel_dimm_security_state(struct nvdimm_bus *nvdimm_bus,
> +               struct nvdimm *nvdimm, enum nvdimm_security_state *state)
> +{
> +       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_get_security_state cmd;
> +       } nd_cmd = {
> +               .pkg = {
> +                       .nd_command = NVDIMM_INTEL_GET_SECURITY_STATE,
> +                       .nd_family = NVDIMM_FAMILY_INTEL,
> +                       .nd_size_in = 0,
> +                       .nd_size_out =
> +                               sizeof(struct nd_intel_get_security_state),
> +                       .nd_fw_size =
> +                               sizeof(struct nd_intel_get_security_state),
> +               },
> +               .cmd = {
> +                       .status = 0,
> +                       .state = 0,
> +               },
> +       };
> +
> +       if (!test_bit(NVDIMM_INTEL_GET_SECURITY_STATE, &nfit_mem->dsm_mask)) {
> +               *state = NVDIMM_SECURITY_UNSUPPORTED;
> +               return 0;
> +       }
> +
> +       *state = NVDIMM_SECURITY_DISABLED;
> +       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_RETRY:
> +               rc = -EAGAIN;
> +               goto out;
> +       case ND_INTEL_STATUS_NOT_READY:
> +       default:
> +               rc = -ENXIO;
> +               goto out;
> +       }
> +
> +       /* check and see if security is enabled and locked */
> +       if (nd_cmd.cmd.state & ND_INTEL_SEC_STATE_UNSUPPORTED)
> +               *state = NVDIMM_SECURITY_UNSUPPORTED;
> +       else if (nd_cmd.cmd.state & ND_INTEL_SEC_STATE_ENABLED) {
> +               if (nd_cmd.cmd.state & ND_INTEL_SEC_STATE_LOCKED)
> +                       *state = NVDIMM_SECURITY_LOCKED;
> +               else
> +                       *state = NVDIMM_SECURITY_UNLOCKED;
> +       } else
> +               *state = NVDIMM_SECURITY_DISABLED;
> +
> + out:
> +       if (rc < 0)
> +               *state = NVDIMM_SECURITY_INVALID;
> +       return rc;
> +}
> +
> +const struct nvdimm_security_ops intel_security_ops = {
> +       .state = intel_dimm_security_state,
> +       .unlock = intel_dimm_security_unlock,
> +};
> diff --git a/drivers/acpi/nfit/intel.h b/drivers/acpi/nfit/intel.h
> index a6f8f4bc8ebb..f9ac718776ca 100644
> --- a/drivers/acpi/nfit/intel.h
> +++ b/drivers/acpi/nfit/intel.h
> @@ -8,6 +8,8 @@
>
>  #ifdef CONFIG_X86
>
> +extern const struct nvdimm_security_ops intel_security_ops;
> +
>  #define ND_INTEL_STATUS_SIZE           4
>  #define ND_INTEL_PASSPHRASE_SIZE       32
>
> @@ -64,4 +66,17 @@ struct nd_intel_query_overwrite {
>  } __packed;
>  #endif /* CONFIG_X86 */
>
> +static inline const struct nvdimm_security_ops *
> +acpi_nfit_get_security_ops(int family)
> +{
> +       switch (family) {
> +#ifdef CONFIG_X86
> +       case NVDIMM_FAMILY_INTEL:
> +               return &intel_security_ops;
> +#endif
> +       default:
> +               return NULL;
> +       }
> +}
> +
>  #endif
> diff --git a/drivers/nvdimm/Kconfig b/drivers/nvdimm/Kconfig
> index 9d36473dc2a2..09fa6bba975d 100644
> --- a/drivers/nvdimm/Kconfig
> +++ b/drivers/nvdimm/Kconfig
> @@ -112,4 +112,9 @@ config OF_PMEM
>
>           Select Y if unsure.
>
> +config NVDIMM_SECURITY
> +       bool
> +       depends on KEYS
> +       default LIBNVDIMM
> +
>  endif
> diff --git a/drivers/nvdimm/Makefile b/drivers/nvdimm/Makefile
> index e8847045dac0..c476547348b1 100644
> --- a/drivers/nvdimm/Makefile
> +++ b/drivers/nvdimm/Makefile
> @@ -27,3 +27,4 @@ libnvdimm-$(CONFIG_ND_CLAIM) += claim.o
>  libnvdimm-$(CONFIG_BTT) += btt_devs.o
>  libnvdimm-$(CONFIG_NVDIMM_PFN) += pfn_devs.o
>  libnvdimm-$(CONFIG_NVDIMM_DAX) += dax_devs.o
> +libnvdimm-$(CONFIG_NVDIMM_SECURITY) += dimm_devs_security.o
> diff --git a/drivers/nvdimm/dimm.c b/drivers/nvdimm/dimm.c
> index 6c8fb7590838..b6381ddbd6c1 100644
> --- a/drivers/nvdimm/dimm.c
> +++ b/drivers/nvdimm/dimm.c
> @@ -51,6 +51,13 @@ static int nvdimm_probe(struct device *dev)
>         get_device(dev);
>         kref_init(&ndd->kref);
>
> +       nvdimm_security_get_state(dev);
> +
> +       /* unlock DIMM here before touch label */
> +       rc = nvdimm_security_unlock_dimm(dev);
> +       if (rc < 0)
> +               dev_warn(dev, "failed to unlock dimm %s\n", dev_name(dev));
> +
>         /*
>          * EACCES failures reading the namespace label-area-properties
>          * are interpreted as the DIMM capacity being locked but the
> diff --git a/drivers/nvdimm/dimm_devs.c b/drivers/nvdimm/dimm_devs.c
> index 3f02008d75e7..024b1c4dfe88 100644
> --- a/drivers/nvdimm/dimm_devs.c
> +++ b/drivers/nvdimm/dimm_devs.c
> @@ -18,6 +18,7 @@
>  #include <linux/io.h>
>  #include <linux/fs.h>
>  #include <linux/mm.h>
> +#include <linux/key.h>
>  #include "nd-core.h"
>  #include "label.h"
>  #include "pmem.h"
> @@ -212,7 +213,13 @@ 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);

If the assumption is that the key is valid as long as it is attached
to the nvdimm, can't we do this instead:

    mutex_lock(&nvdimm->key_mutex);
    key_put(nvdimm->key);
    nvdimm->key = NULL;
    mutex_unlock(&nvdimm->key_mutex);

key_put() already checks for key being NULL, and don't we need to
disconnect the key from anyone else that might be doing a nvdimm->key
usage right after we drop the lock?
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH v12 04/12] nfit/libnvdimm: add unlock of nvdimm support for Intel DIMMs
  2018-10-09 19:07   ` Dan Williams
@ 2018-10-09 19:45     ` Dave Jiang
  0 siblings, 0 replies; 17+ messages in thread
From: Dave Jiang @ 2018-10-09 19:45 UTC (permalink / raw)
  To: Dan Williams
  Cc: Schofield, Alison, Kees Cook, linux-nvdimm, Eric Biggers,
	David Howells, keyrings



On 10/09/2018 12:07 PM, Dan Williams wrote:
> On Mon, Oct 8, 2018 at 5:01 PM Dave Jiang <dave.jiang@intel.com> wrote:
>>
>> Add support to allow query the security status of the Intel nvdimms and
>> also unlock the dimm via the kernel key management APIs. The passphrase is
>> expected to be pulled from userspace through keyutils. Moving the Intel
>> related bits to its own source file as well.
>>
>> Signed-off-by: Dave Jiang <dave.jiang@intel.com>
>> Reviewed-by: Dan Williams <dan.j.williams@intel.com>
>> Signed-off-by: Dan Williams <dan.j.williams@intel.com>
>> ---
>>  drivers/acpi/nfit/Makefile          |    1
>>  drivers/acpi/nfit/core.c            |    3 -
>>  drivers/acpi/nfit/intel.c           |  152 +++++++++++++++++++++++++++++++++++
>>  drivers/acpi/nfit/intel.h           |   15 +++
>>  drivers/nvdimm/Kconfig              |    5 +
>>  drivers/nvdimm/Makefile             |    1
>>  drivers/nvdimm/dimm.c               |    7 ++
>>  drivers/nvdimm/dimm_devs.c          |   12 +++
>>  drivers/nvdimm/dimm_devs_security.c |  129 ++++++++++++++++++++++++++++++
>>  drivers/nvdimm/nd-core.h            |    5 +
>>  drivers/nvdimm/nd.h                 |   21 +++++
>>  include/linux/libnvdimm.h           |   27 ++++++
>>  include/uapi/linux/ndctl.h          |    6 +
>>  tools/testing/nvdimm/Kbuild         |    2
>>  14 files changed, 383 insertions(+), 3 deletions(-)
>>  create mode 100644 drivers/acpi/nfit/intel.c
>>  create mode 100644 drivers/nvdimm/dimm_devs_security.c
>>
>> diff --git a/drivers/acpi/nfit/Makefile b/drivers/acpi/nfit/Makefile
>> index a407e769f103..443c7ef4e6a6 100644
>> --- a/drivers/acpi/nfit/Makefile
>> +++ b/drivers/acpi/nfit/Makefile
>> @@ -1,3 +1,4 @@
>>  obj-$(CONFIG_ACPI_NFIT) := nfit.o
>>  nfit-y := core.o
>> +nfit-$(CONFIG_X86) += intel.o
>>  nfit-$(CONFIG_X86_MCE) += mce.o
>> diff --git a/drivers/acpi/nfit/core.c b/drivers/acpi/nfit/core.c
>> index 94755a4b6327..e5f7c664a7c8 100644
>> --- a/drivers/acpi/nfit/core.c
>> +++ b/drivers/acpi/nfit/core.c
>> @@ -1904,7 +1904,8 @@ static int acpi_nfit_register_dimms(struct acpi_nfit_desc *acpi_desc)
>>                 nvdimm = nvdimm_create(acpi_desc->nvdimm_bus, nfit_mem,
>>                                 acpi_nfit_dimm_attribute_groups,
>>                                 flags, cmd_mask, flush ? flush->hint_count : 0,
>> -                               nfit_mem->flush_wpq, &nfit_mem->id[0]);
>> +                               nfit_mem->flush_wpq, &nfit_mem->id[0],
>> +                               acpi_nfit_get_security_ops(nfit_mem->family));
>>                 if (!nvdimm)
>>                         return -ENOMEM;
>>
>> diff --git a/drivers/acpi/nfit/intel.c b/drivers/acpi/nfit/intel.c
>> new file mode 100644
>> index 000000000000..4bfc1c1da339
>> --- /dev/null
>> +++ b/drivers/acpi/nfit/intel.c
>> @@ -0,0 +1,152 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/* Copyright(c) 2018 Intel Corporation. All rights reserved. */
>> +/*
>> + * Intel specific NFIT ops
>> + */
>> +#include <linux/libnvdimm.h>
>> +#include <linux/module.h>
>> +#include <linux/mutex.h>
>> +#include <linux/ndctl.h>
>> +#include <linux/sysfs.h>
>> +#include <linux/delay.h>
>> +#include <linux/acpi.h>
>> +#include <linux/io.h>
>> +#include <linux/nd.h>
>> +#include <asm/cacheflush.h>
>> +#include <asm/smp.h>
>> +#include <acpi/nfit.h>
>> +#include "intel.h"
>> +#include "nfit.h"
>> +
>> +static int intel_dimm_security_unlock(struct nvdimm_bus *nvdimm_bus,
>> +               struct nvdimm *nvdimm, const struct nvdimm_key_data *nkey)
>> +{
>> +       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_unlock_unit cmd;
>> +       } nd_cmd = {
>> +               .pkg = {
>> +                       .nd_command = NVDIMM_INTEL_UNLOCK_UNIT,
>> +                       .nd_family = NVDIMM_FAMILY_INTEL,
>> +                       .nd_size_in = ND_INTEL_PASSPHRASE_SIZE,
>> +                       .nd_size_out = ND_INTEL_STATUS_SIZE,
>> +                       .nd_fw_size = ND_INTEL_STATUS_SIZE,
>> +               },
>> +               .cmd = {
>> +                       .status = 0,
>> +               },
>> +       };
>> +
>> +       if (!test_bit(NVDIMM_INTEL_UNLOCK_UNIT, &nfit_mem->dsm_mask))
>> +               return -ENOTTY;
>> +
>> +       memcpy(nd_cmd.cmd.passphrase, nkey->data,
>> +                       sizeof(nd_cmd.cmd.passphrase));
>> +       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;
>> +       }
>> +
>> +       /*
>> +        * TODO: define a cross arch wbinvd when/if NVDIMM_FAMILY_INTEL
>> +        * support arrives on another arch.
>> +        */
>> +       /* DIMM unlocked, invalidate all CPU caches before we read it */
>> +       wbinvd_on_all_cpus();
>> +
>> + out:
>> +       return rc;
>> +}
>> +
>> +static int intel_dimm_security_state(struct nvdimm_bus *nvdimm_bus,
>> +               struct nvdimm *nvdimm, enum nvdimm_security_state *state)
>> +{
>> +       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_get_security_state cmd;
>> +       } nd_cmd = {
>> +               .pkg = {
>> +                       .nd_command = NVDIMM_INTEL_GET_SECURITY_STATE,
>> +                       .nd_family = NVDIMM_FAMILY_INTEL,
>> +                       .nd_size_in = 0,
>> +                       .nd_size_out =
>> +                               sizeof(struct nd_intel_get_security_state),
>> +                       .nd_fw_size =
>> +                               sizeof(struct nd_intel_get_security_state),
>> +               },
>> +               .cmd = {
>> +                       .status = 0,
>> +                       .state = 0,
>> +               },
>> +       };
>> +
>> +       if (!test_bit(NVDIMM_INTEL_GET_SECURITY_STATE, &nfit_mem->dsm_mask)) {
>> +               *state = NVDIMM_SECURITY_UNSUPPORTED;
>> +               return 0;
>> +       }
>> +
>> +       *state = NVDIMM_SECURITY_DISABLED;
>> +       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_RETRY:
>> +               rc = -EAGAIN;
>> +               goto out;
>> +       case ND_INTEL_STATUS_NOT_READY:
>> +       default:
>> +               rc = -ENXIO;
>> +               goto out;
>> +       }
>> +
>> +       /* check and see if security is enabled and locked */
>> +       if (nd_cmd.cmd.state & ND_INTEL_SEC_STATE_UNSUPPORTED)
>> +               *state = NVDIMM_SECURITY_UNSUPPORTED;
>> +       else if (nd_cmd.cmd.state & ND_INTEL_SEC_STATE_ENABLED) {
>> +               if (nd_cmd.cmd.state & ND_INTEL_SEC_STATE_LOCKED)
>> +                       *state = NVDIMM_SECURITY_LOCKED;
>> +               else
>> +                       *state = NVDIMM_SECURITY_UNLOCKED;
>> +       } else
>> +               *state = NVDIMM_SECURITY_DISABLED;
>> +
>> + out:
>> +       if (rc < 0)
>> +               *state = NVDIMM_SECURITY_INVALID;
>> +       return rc;
>> +}
>> +
>> +const struct nvdimm_security_ops intel_security_ops = {
>> +       .state = intel_dimm_security_state,
>> +       .unlock = intel_dimm_security_unlock,
>> +};
>> diff --git a/drivers/acpi/nfit/intel.h b/drivers/acpi/nfit/intel.h
>> index a6f8f4bc8ebb..f9ac718776ca 100644
>> --- a/drivers/acpi/nfit/intel.h
>> +++ b/drivers/acpi/nfit/intel.h
>> @@ -8,6 +8,8 @@
>>
>>  #ifdef CONFIG_X86
>>
>> +extern const struct nvdimm_security_ops intel_security_ops;
>> +
>>  #define ND_INTEL_STATUS_SIZE           4
>>  #define ND_INTEL_PASSPHRASE_SIZE       32
>>
>> @@ -64,4 +66,17 @@ struct nd_intel_query_overwrite {
>>  } __packed;
>>  #endif /* CONFIG_X86 */
>>
>> +static inline const struct nvdimm_security_ops *
>> +acpi_nfit_get_security_ops(int family)
>> +{
>> +       switch (family) {
>> +#ifdef CONFIG_X86
>> +       case NVDIMM_FAMILY_INTEL:
>> +               return &intel_security_ops;
>> +#endif
>> +       default:
>> +               return NULL;
>> +       }
>> +}
>> +
>>  #endif
>> diff --git a/drivers/nvdimm/Kconfig b/drivers/nvdimm/Kconfig
>> index 9d36473dc2a2..09fa6bba975d 100644
>> --- a/drivers/nvdimm/Kconfig
>> +++ b/drivers/nvdimm/Kconfig
>> @@ -112,4 +112,9 @@ config OF_PMEM
>>
>>           Select Y if unsure.
>>
>> +config NVDIMM_SECURITY
>> +       bool
>> +       depends on KEYS
>> +       default LIBNVDIMM
>> +
>>  endif
>> diff --git a/drivers/nvdimm/Makefile b/drivers/nvdimm/Makefile
>> index e8847045dac0..c476547348b1 100644
>> --- a/drivers/nvdimm/Makefile
>> +++ b/drivers/nvdimm/Makefile
>> @@ -27,3 +27,4 @@ libnvdimm-$(CONFIG_ND_CLAIM) += claim.o
>>  libnvdimm-$(CONFIG_BTT) += btt_devs.o
>>  libnvdimm-$(CONFIG_NVDIMM_PFN) += pfn_devs.o
>>  libnvdimm-$(CONFIG_NVDIMM_DAX) += dax_devs.o
>> +libnvdimm-$(CONFIG_NVDIMM_SECURITY) += dimm_devs_security.o
>> diff --git a/drivers/nvdimm/dimm.c b/drivers/nvdimm/dimm.c
>> index 6c8fb7590838..b6381ddbd6c1 100644
>> --- a/drivers/nvdimm/dimm.c
>> +++ b/drivers/nvdimm/dimm.c
>> @@ -51,6 +51,13 @@ static int nvdimm_probe(struct device *dev)
>>         get_device(dev);
>>         kref_init(&ndd->kref);
>>
>> +       nvdimm_security_get_state(dev);
>> +
>> +       /* unlock DIMM here before touch label */
>> +       rc = nvdimm_security_unlock_dimm(dev);
>> +       if (rc < 0)
>> +               dev_warn(dev, "failed to unlock dimm %s\n", dev_name(dev));
>> +
>>         /*
>>          * EACCES failures reading the namespace label-area-properties
>>          * are interpreted as the DIMM capacity being locked but the
>> diff --git a/drivers/nvdimm/dimm_devs.c b/drivers/nvdimm/dimm_devs.c
>> index 3f02008d75e7..024b1c4dfe88 100644
>> --- a/drivers/nvdimm/dimm_devs.c
>> +++ b/drivers/nvdimm/dimm_devs.c
>> @@ -18,6 +18,7 @@
>>  #include <linux/io.h>
>>  #include <linux/fs.h>
>>  #include <linux/mm.h>
>> +#include <linux/key.h>
>>  #include "nd-core.h"
>>  #include "label.h"
>>  #include "pmem.h"
>> @@ -212,7 +213,13 @@ 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);
> 
> If the assumption is that the key is valid as long as it is attached
> to the nvdimm, can't we do this instead:
> 
>     mutex_lock(&nvdimm->key_mutex);
>     key_put(nvdimm->key);
>     nvdimm->key = NULL;
>     mutex_unlock(&nvdimm->key_mutex);

Yes, since key_put() already checks. We could have an invalid key if
security is not on.

> 
> key_put() already checks for key being NULL, and don't we need to
> disconnect the key from anyone else that might be doing a nvdimm->key
> usage right after we drop the lock?
> 

I don't think so. Whomever is using has to hold the mutex. So either
it's being in use and the release block until we can set the key to NULL
or we have set the key NULL before the user tries to acquire the key.
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH v12 00/12] Adding security support for nvdimm
  2018-10-08 23:55 [PATCH v12 00/12] Adding security support for nvdimm Dave Jiang
                   ` (11 preceding siblings ...)
  2018-10-08 23:56 ` [PATCH v12 12/12] acpi, nfit: Move acpi_nfit_get_security_ops() to generic location Dave Jiang
@ 2018-10-10  1:35 ` Williams, Dan J
  2018-10-10 16:13   ` Dave Jiang
  12 siblings, 1 reply; 17+ messages in thread
From: Williams, Dan J @ 2018-10-10  1:35 UTC (permalink / raw)
  To: Jiang, Dave
  Cc: Schofield, Alison, keescook, linux-nvdimm, ebiggers3, dhowells, keyrings

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

^ permalink raw reply related	[flat|nested] 17+ messages in thread

* Re: [PATCH v12 00/12] Adding security support for nvdimm
  2018-10-10  1:35 ` [PATCH v12 00/12] Adding security support for nvdimm Williams, Dan J
@ 2018-10-10 16:13   ` Dave Jiang
  0 siblings, 0 replies; 17+ messages in thread
From: Dave Jiang @ 2018-10-10 16:13 UTC (permalink / raw)
  To: Williams, Dan J
  Cc: Schofield, Alison, keescook, linux-nvdimm, ebiggers3, dhowells, keyrings



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);
> 
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

^ permalink raw reply	[flat|nested] 17+ messages in thread

end of thread, other threads:[~2018-10-10 16:13 UTC | newest]

Thread overview: 17+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-10-08 23:55 [PATCH v12 00/12] Adding security support for nvdimm Dave Jiang
2018-10-08 23:55 ` [PATCH v12 01/12] nfit: add support for Intel DSM 1.7 commands Dave Jiang
2018-10-08 23:55 ` [PATCH v12 02/12] nfit/libnvdimm: store dimm id as a member to struct nvdimm Dave Jiang
2018-10-08 23:55 ` [PATCH v12 03/12] keys: export lookup_user_key to external users Dave Jiang
2018-10-08 23:55 ` [PATCH v12 04/12] nfit/libnvdimm: add unlock of nvdimm support for Intel DIMMs Dave Jiang
2018-10-09 19:07   ` Dan Williams
2018-10-09 19:45     ` Dave Jiang
2018-10-08 23:55 ` [PATCH v12 05/12] nfit/libnvdimm: add set passphrase support for Intel nvdimms Dave Jiang
2018-10-08 23:56 ` [PATCH v12 06/12] nfit/libnvdimm: add disable passphrase support to Intel nvdimm Dave Jiang
2018-10-08 23:56 ` [PATCH v12 07/12] nfit/libnvdimm: add freeze security " Dave Jiang
2018-10-08 23:56 ` [PATCH v12 08/12] nfit/libnvdimm: add support for issue secure erase DSM " Dave Jiang
2018-10-08 23:56 ` [PATCH v12 09/12] nfit_test: add test support for Intel nvdimm security DSMs Dave Jiang
2018-10-08 23:56 ` [PATCH v12 10/12] libnvdimm: add documentation for nvdimm security support Dave Jiang
2018-10-08 23:56 ` [PATCH v12 11/12] libnvdimm: Drop nvdimm_bus from security_ops interface Dave Jiang
2018-10-08 23:56 ` [PATCH v12 12/12] acpi, nfit: Move acpi_nfit_get_security_ops() to generic location Dave Jiang
2018-10-10  1:35 ` [PATCH v12 00/12] Adding security support for nvdimm Williams, Dan J
2018-10-10 16:13   ` Dave Jiang

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).