[PATCH v5 00/12] Adding security support for nvdimm

[PATCH v5 00/12] Adding security support for nvdimm

[Dave Jiang]

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.

Security folks, thanks in advance for taking a look at my key management
implementation and making sure that I'm doing something sane. Mainly you'll
want to review patches 2, 4, 5, and 6 as most relevant ones that need scrutiny.

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.

---

Dave Jiang (12):
      nfit: add support for Intel DSM 1.7 commands
      libnvdimm: create keyring to store security keys
      nfit/libnvdimm: store dimm id as a member to struct nvdimm
      nfit/libnvdimm: add unlock of nvdimm support for Intel DIMMs
      keys: add call key_put_sync() to flush key_gc_work when doing a key_put().
      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 context to dimm_dev for nfit_test
      nfit_test: add test support for Intel nvdimm security DSMs
      libnvdimm: add documentation for nvdimm security support


 Documentation/nvdimm/security    |   70 ++++++
 drivers/acpi/nfit/Makefile       |    1 
 drivers/acpi/nfit/core.c         |   58 ++++-
 drivers/acpi/nfit/intel.c        |  366 ++++++++++++++++++++++++++++++++
 drivers/acpi/nfit/intel.h        |   83 +++++++
 drivers/acpi/nfit/nfit.h         |   20 ++
 drivers/nvdimm/bus.c             |    2 
 drivers/nvdimm/core.c            |    7 +
 drivers/nvdimm/dimm.c            |    7 +
 drivers/nvdimm/dimm_devs.c       |  430 ++++++++++++++++++++++++++++++++++++++
 drivers/nvdimm/nd-core.h         |    4 
 drivers/nvdimm/nd.h              |    2 
 include/linux/key.h              |    1 
 include/linux/libnvdimm.h        |   41 +++-
 security/keys/key.c              |   35 +++
 tools/testing/nvdimm/Kbuild      |    1 
 tools/testing/nvdimm/test/nfit.c |  227 +++++++++++++++++++-
 17 files changed, 1315 insertions(+), 40 deletions(-)
 create mode 100644 Documentation/nvdimm/security
 create mode 100644 drivers/acpi/nfit/intel.c
 create mode 100644 drivers/acpi/nfit/intel.h

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

[PATCH v5 01/12] nfit: add support for Intel DSM 1.7 commands

[Dave Jiang]

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>
---
 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 0f47917b8ef1..a3a944751e6a 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;
@@ -3202,7 +3211,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);
@@ -3224,6 +3233,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..f60dbcbc5f33
--- /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[32];
+	u8 new_pass[32];
+	u32 status;
+} __packed;
+
+struct nd_intel_unlock_unit {
+	u8 passphrase[32];
+	u32 status;
+} __packed;
+
+struct nd_intel_disable_passphrase {
+	u8 passphrase[32];
+	u32 status;
+} __packed;
+
+struct nd_intel_freeze_lock {
+	u32 status;
+} __packed;
+
+struct nd_intel_secure_erase {
+	u8 passphrase[32];
+	u32 status;
+} __packed;
+
+struct nd_intel_overwrite {
+	u8 passphrase[32];
+	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 7d15856a739f..40b0003b1805 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 27902a8799b1..de2af49bbf08 100644
--- a/drivers/nvdimm/bus.c
+++ b/drivers/nvdimm/bus.c
@@ -894,7 +894,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

[PATCH v5 02/12] libnvdimm: create keyring to store security keys

[Dave Jiang]

Prepping the libnvdimm to support security management by adding a keyring
in order to provide passphrase management through the kernel key management
APIs.

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

diff --git a/drivers/nvdimm/core.c b/drivers/nvdimm/core.c
index acce050856a8..3cd33d5c7cf0 100644
--- a/drivers/nvdimm/core.c
+++ b/drivers/nvdimm/core.c
@@ -437,9 +437,12 @@ static __init int libnvdimm_init(void)
 {
 	int rc;
 
-	rc = nvdimm_bus_init();
+	rc = nvdimm_devs_init();
 	if (rc)
 		return rc;
+	rc = nvdimm_bus_init();
+	if (rc)
+		goto err_bus;
 	rc = nvdimm_init();
 	if (rc)
 		goto err_dimm;
@@ -454,6 +457,8 @@ static __init int libnvdimm_init(void)
 	nvdimm_exit();
  err_dimm:
 	nvdimm_bus_exit();
+ err_bus:
+	nvdimm_devs_exit();
 	return rc;
 }
 
diff --git a/drivers/nvdimm/dimm_devs.c b/drivers/nvdimm/dimm_devs.c
index 8d348b22ba45..1dcbb653455b 100644
--- a/drivers/nvdimm/dimm_devs.c
+++ b/drivers/nvdimm/dimm_devs.c
@@ -18,12 +18,54 @@
 #include <linux/io.h>
 #include <linux/fs.h>
 #include <linux/mm.h>
+#include <linux/cred.h>
+#include <keys/user-type.h>
+#include <linux/key-type.h>
+#include <linux/keyctl.h>
 #include "nd-core.h"
 #include "label.h"
 #include "pmem.h"
 #include "nd.h"
 
 static DEFINE_IDA(dimm_ida);
+static const struct cred *nvdimm_cred;
+
+static int nvdimm_key_instantiate(struct key *key,
+		struct key_preparsed_payload *prep);
+static void nvdimm_key_destroy(struct key *key);
+
+struct key_type nvdimm_key_type = {
+	.name = "nvdimm",
+	.instantiate = nvdimm_key_instantiate,
+	.destroy = nvdimm_key_destroy,
+	.describe = user_describe,
+	/*
+	 * The reason to have default data length to be len*2 is to
+	 * accomodate the payload when we are doing a key change where
+	 * the we stuff the old key and new key in the same payload.
+	 */
+	.def_datalen = NVDIMM_PASSPHRASE_LEN * 2,
+};
+
+static int nvdimm_key_instantiate(struct key *key,
+		struct key_preparsed_payload *prep)
+{
+	char *payload;
+
+	payload = kzalloc(nvdimm_key_type.def_datalen, GFP_KERNEL);
+	if (!payload)
+		return -ENOMEM;
+
+	key->datalen = min(nvdimm_key_type.def_datalen, prep->datalen);
+	memcpy(payload, prep->data, key->datalen);
+	key->payload.data[0] = payload;
+	return 0;
+}
+
+static void nvdimm_key_destroy(struct key *key)
+{
+	kfree(key->payload.data[0]);
+}
 
 /*
  * Retrieve bus and dimm handle and return if this bus supports
@@ -668,7 +710,59 @@ int nvdimm_bus_check_dimm_count(struct nvdimm_bus *nvdimm_bus, int dimm_count)
 }
 EXPORT_SYMBOL_GPL(nvdimm_bus_check_dimm_count);
 
-void __exit nvdimm_devs_exit(void)
+static int nvdimm_register_keyring(void)
+{
+	struct cred *cred;
+	struct key *keyring;
+	int rc;
+
+	rc = register_key_type(&nvdimm_key_type);
+	if (rc < 0)
+		return rc;
+
+	cred = prepare_kernel_cred(NULL);
+	if (!cred) {
+		rc = -ENOMEM;
+		goto failed_cred;
+	}
+
+	keyring = keyring_alloc(".nvdimm",
+			GLOBAL_ROOT_UID, GLOBAL_ROOT_GID, cred,
+			(KEY_POS_ALL & ~KEY_POS_SETATTR) |
+			(KEY_USR_ALL & ~KEY_USR_SETATTR),
+			KEY_ALLOC_NOT_IN_QUOTA, NULL, NULL);
+	if (IS_ERR(keyring)) {
+		rc = PTR_ERR(keyring);
+		goto failed_keyring;
+	}
+
+	set_bit(KEY_FLAG_ROOT_CAN_CLEAR, &keyring->flags);
+	cred->thread_keyring = keyring;
+	cred->jit_keyring = KEY_REQKEY_DEFL_THREAD_KEYRING;
+	nvdimm_cred = cred;
+	return 0;
+
+ failed_cred:
+	unregister_key_type(&nvdimm_key_type);
+ failed_keyring:
+	put_cred(cred);
+	return rc;
+}
+
+static void nvdimm_unregister_keyring(void)
+{
+	key_revoke(nvdimm_cred->thread_keyring);
+	unregister_key_type(&nvdimm_key_type);
+	put_cred(nvdimm_cred);
+}
+
+int __init nvdimm_devs_init(void)
+{
+	return nvdimm_register_keyring();
+}
+
+void  nvdimm_devs_exit(void)
 {
+	nvdimm_unregister_keyring();
 	ida_destroy(&dimm_ida);
 }
diff --git a/drivers/nvdimm/nd-core.h b/drivers/nvdimm/nd-core.h
index 79274ead54fb..2af0f89c4010 100644
--- a/drivers/nvdimm/nd-core.h
+++ b/drivers/nvdimm/nd-core.h
@@ -76,6 +76,7 @@ static inline bool is_memory(struct device *dev)
 struct nvdimm_bus *walk_to_nvdimm_bus(struct device *nd_dev);
 int __init nvdimm_bus_init(void);
 void nvdimm_bus_exit(void);
+int nvdimm_devs_init(void);
 void nvdimm_devs_exit(void);
 void nd_region_devs_exit(void);
 void nd_region_probe_success(struct nvdimm_bus *nvdimm_bus, struct device *dev);
diff --git a/include/linux/libnvdimm.h b/include/linux/libnvdimm.h
index 472171af7f60..09dd06f96f95 100644
--- a/include/linux/libnvdimm.h
+++ b/include/linux/libnvdimm.h
@@ -155,6 +155,11 @@ static inline struct nd_blk_region_desc *to_blk_region_desc(
 
 }
 
+extern struct key_type nvdimm_key_type;
+
+#define NVDIMM_PASSPHRASE_LEN		32
+#define NVDIMM_KEY_DESC_LEN		25
+
 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,

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

[PATCH v5 03/12] nfit/libnvdimm: store dimm id as a member to struct nvdimm

[Dave Jiang]

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>
---
 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 a3a944751e6a..21235d555b5f 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 40b0003b1805..5d274b7a553a 100644
--- a/drivers/acpi/nfit/nfit.h
+++ b/drivers/acpi/nfit/nfit.h
@@ -173,6 +173,8 @@ struct nfit_memdev {
 	struct acpi_nfit_memory_map memdev[0];
 };
 
+#define NFIT_DIMM_ID_LEN	25
+
 /* assembled tables for a given dimm/memory-device */
 struct nfit_mem {
 	struct nvdimm *nvdimm;
@@ -195,6 +197,7 @@ struct nfit_mem {
 	int family;
 	bool has_lsr;
 	bool has_lsw;
+	char id[NFIT_DIMM_ID_LEN];
 };
 
 struct acpi_nfit_desc {
diff --git a/drivers/nvdimm/dimm_devs.c b/drivers/nvdimm/dimm_devs.c
index 1dcbb653455b..1a32ef8cce4c 100644
--- a/drivers/nvdimm/dimm_devs.c
+++ b/drivers/nvdimm/dimm_devs.c
@@ -440,7 +440,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;
@@ -453,6 +453,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 2af0f89c4010..ed650f1607d8 100644
--- a/drivers/nvdimm/nd-core.h
+++ b/drivers/nvdimm/nd-core.h
@@ -42,6 +42,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 09dd06f96f95..6d0247b95e6f 100644
--- a/include/linux/libnvdimm.h
+++ b/include/linux/libnvdimm.h
@@ -183,7 +183,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

[PATCH v5 04/12] nfit/libnvdimm: add unlock of nvdimm support for Intel DIMMs

[Dave Jiang]

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>
---
 drivers/acpi/nfit/Makefile |    1 
 drivers/acpi/nfit/core.c   |    3 +
 drivers/acpi/nfit/intel.c  |  151 ++++++++++++++++++++++++++++++++++++++++++++
 drivers/acpi/nfit/intel.h  |   16 +++++
 drivers/nvdimm/dimm.c      |    7 ++
 drivers/nvdimm/dimm_devs.c |  108 +++++++++++++++++++++++++++++++
 drivers/nvdimm/nd-core.h   |    2 +
 drivers/nvdimm/nd.h        |    2 +
 include/linux/libnvdimm.h  |   23 ++++++-
 9 files changed, 310 insertions(+), 3 deletions(-)
 create mode 100644 drivers/acpi/nfit/intel.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 21235d555b5f..9cb6a108ecba 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..9155b8e63f0e
--- /dev/null
+++ b/drivers/acpi/nfit/intel.c
@@ -0,0 +1,151 @@
+// 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, 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, ND_INTEL_PASSPHRASE_SIZE);
+	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;
+}
+
+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 f60dbcbc5f33..de722420e3b3 100644
--- a/drivers/acpi/nfit/intel.h
+++ b/drivers/acpi/nfit/intel.h
@@ -8,6 +8,8 @@
 
 #ifdef CONFIG_X86
 
+extern struct nvdimm_security_ops intel_security_ops;
+
 #define ND_INTEL_STATUS_SIZE		4
 #define ND_INTEL_PASSPHRASE_SIZE	32
 
@@ -62,6 +64,20 @@ struct nd_intel_overwrite {
 struct nd_intel_query_overwrite {
 	u32 status;
 } __packed;
+
 #endif /* CONFIG_X86 */
 
+static inline 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/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 1a32ef8cce4c..7600bbbb8a91 100644
--- a/drivers/nvdimm/dimm_devs.c
+++ b/drivers/nvdimm/dimm_devs.c
@@ -67,6 +67,110 @@ static void nvdimm_key_destroy(struct key *key)
 	kfree(key->payload.data[0]);
 }
 
+/*
+ * Find key in kernel keyring
+ */
+static struct key *nvdimm_search_key(struct device *dev)
+{
+	struct nvdimm *nvdimm = to_nvdimm(dev);
+	char *desc;
+	key_ref_t keyref;
+	struct key *key = NULL;
+
+	if (!nvdimm->security_ops)
+		return NULL;
+
+	desc = kzalloc(NVDIMM_KEY_DESC_LEN, GFP_KERNEL);
+	if (!desc)
+		return NULL;
+
+	keyref = keyring_search(make_key_ref(nvdimm_cred->thread_keyring, 1),
+			&nvdimm_key_type, nvdimm->dimm_id);
+	if (IS_ERR(keyref))
+		key = NULL;
+	else
+		key = key_ref_to_ptr(keyref);
+
+	kfree(desc);
+	return 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;
+
+	if (!nvdimm->security_ops)
+		return NULL;
+
+	key = request_key(&nvdimm_key_type, nvdimm->dimm_id, nvdimm->dimm_id);
+	if (IS_ERR(key))
+		key = NULL;
+
+	return key;
+}
+
+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;
+	int rc;
+	void *payload;
+	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;
+
+	key = nvdimm_search_key(dev);
+	if (!key)
+		key = nvdimm_request_key(dev);
+	else
+		cached_key = true;
+	if (!key)
+		return -ENXIO;
+
+	dev_dbg(dev, "%s: key: %#x\n", __func__, key->serial);
+	down_read(&key->sem);
+	payload = key->payload.data[0];
+	rc = nvdimm->security_ops->unlock(nvdimm_bus, nvdimm, payload);
+	up_read(&key->sem);
+
+	if (rc == 0) {
+		if (!cached_key)
+			key_link(nvdimm_cred->thread_keyring, key);
+		nvdimm->state = NVDIMM_SECURITY_UNLOCKED;
+		dev_info(dev, "DIMM %s unlocked\n", dev_name(dev));
+	} else {
+		key_invalidate(key);
+		dev_warn(dev, "Failed to unlock dimm: %s\n", dev_name(dev));
+	}
+
+	key_put(key);
+	nvdimm_security_get_state(dev);
+	return rc;
+}
+
 /*
  * Retrieve bus and dimm handle and return if this bus supports
  * get_config_data commands
@@ -440,7 +544,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,
+		struct nvdimm_security_ops *sec_ops)
 {
 	struct nvdimm *nvdimm = kzalloc(sizeof(*nvdimm), GFP_KERNEL);
 	struct device *dev;
@@ -455,6 +560,7 @@ 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;
diff --git a/drivers/nvdimm/nd-core.h b/drivers/nvdimm/nd-core.h
index ed650f1607d8..90f199d0ace2 100644
--- a/drivers/nvdimm/nd-core.h
+++ b/drivers/nvdimm/nd-core.h
@@ -43,6 +43,8 @@ struct nvdimm {
 	int id, num_flush;
 	struct resource *flush_wpq;
 	const char *dimm_id;
+	struct nvdimm_security_ops *security_ops;
+	enum nvdimm_security_state state;
 };
 
 /**
diff --git a/drivers/nvdimm/nd.h b/drivers/nvdimm/nd.h
index 9d17abd9f8d0..9c80e0f8c327 100644
--- a/drivers/nvdimm/nd.h
+++ b/drivers/nvdimm/nd.h
@@ -424,4 +424,6 @@ 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);
+int nvdimm_security_unlock_dimm(struct device *dev);
+int nvdimm_security_get_state(struct device *dev);
 #endif /* __ND_H__ */
diff --git a/include/linux/libnvdimm.h b/include/linux/libnvdimm.h
index 6d0247b95e6f..692217f82c6e 100644
--- a/include/linux/libnvdimm.h
+++ b/include/linux/libnvdimm.h
@@ -157,9 +157,29 @@ static inline struct nd_blk_region_desc *to_blk_region_desc(
 
 extern struct key_type nvdimm_key_type;
 
+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		25
 
+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, 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,
@@ -183,7 +203,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,
+		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,

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

[PATCH v5 05/12] keys: add call key_put_sync() to flush key_gc_work when doing a key_put().

[Dave Jiang]

Adding key_put_sync() that calls flush_work on key_gc_work to ensure the
key we want to remove is gone.

Signed-off-by: Dave Jiang <dave.jiang@intel.com>
---
 include/linux/key.h |    1 +
 security/keys/key.c |   35 +++++++++++++++++++++++++++++------
 2 files changed, 30 insertions(+), 6 deletions(-)

diff --git a/include/linux/key.h b/include/linux/key.h
index e58ee10f6e58..4dffbd23d4e4 100644
--- a/include/linux/key.h
+++ b/include/linux/key.h
@@ -253,6 +253,7 @@ extern struct key *key_alloc(struct key_type *type,
 extern void key_revoke(struct key *key);
 extern void key_invalidate(struct key *key);
 extern void key_put(struct key *key);
+extern void key_put_sync(struct key *key);
 
 static inline struct key *__key_get(struct key *key)
 {
diff --git a/security/keys/key.c b/security/keys/key.c
index d97c9394b5dd..b6f3ec19ab0f 100644
--- a/security/keys/key.c
+++ b/security/keys/key.c
@@ -622,6 +622,19 @@ int key_reject_and_link(struct key *key,
 }
 EXPORT_SYMBOL(key_reject_and_link);
 
+static void __key_put(struct key *key, bool sync)
+{
+	if (key) {
+		key_check(key);
+
+		if (refcount_dec_and_test(&key->usage)) {
+			schedule_work(&key_gc_work);
+			if (sync)
+				flush_work(&key_gc_work);
+		}
+	}
+}
+
 /**
  * key_put - Discard a reference to a key.
  * @key: The key to discard a reference from.
@@ -632,15 +645,25 @@ EXPORT_SYMBOL(key_reject_and_link);
  */
 void key_put(struct key *key)
 {
-	if (key) {
-		key_check(key);
-
-		if (refcount_dec_and_test(&key->usage))
-			schedule_work(&key_gc_work);
-	}
+	__key_put(key, false);
 }
 EXPORT_SYMBOL(key_put);
 
+/**
+ * key_put - Discard a reference to a key and flush the key gc work item.
+ * @key: The key to discard a reference from.
+ *
+ * Discard a reference to a key, and when all the references are gone, we
+ * schedule the cleanup task to come and pull it out of the tree in process
+ * context at some later time. This call flushes the clean up so we are sure
+ * that the key is gone.
+ */
+void key_put_sync(struct key *key)
+{
+	__key_put(key, true);
+}
+EXPORT_SYMBOL(key_put_sync);
+
 /*
  * Find a key by its serial number.
  */

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

[PATCH v5 06/12] nfit/libnvdimm: add set passphrase support for Intel nvdimms

[Dave Jiang]

Add support for setting and/or updating passphrase on the Intel nvdimms.
The passphrase is pulled from userspace through the kernel key management.
We trigger the update via writing "update" to the sysfs attribute
"security". The state of the security can also be read via the "security"
attribute. libnvdimm will generically support the key_change API call.

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

diff --git a/drivers/acpi/nfit/intel.c b/drivers/acpi/nfit/intel.c
index 9155b8e63f0e..b0a62248467d 100644
--- a/drivers/acpi/nfit/intel.c
+++ b/drivers/acpi/nfit/intel.c
@@ -18,6 +18,62 @@
 #include "intel.h"
 #include "nfit.h"
 
+static int intel_dimm_security_update_passphrase(
+		struct nvdimm_bus *nvdimm_bus, struct nvdimm *nvdimm,
+		struct nvdimm_key_data *old_data,
+		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,
+				ND_INTEL_PASSPHRASE_SIZE);
+	memcpy(nd_cmd.cmd.new_pass, new_data->data, ND_INTEL_PASSPHRASE_SIZE);
+	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, struct nvdimm_key_data *nkey)
 {
@@ -148,4 +204,5 @@ static int intel_dimm_security_state(struct nvdimm_bus *nvdimm_bus,
 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 7600bbbb8a91..432bdd8d19e3 100644
--- a/drivers/nvdimm/dimm_devs.c
+++ b/drivers/nvdimm/dimm_devs.c
@@ -171,6 +171,73 @@ int nvdimm_security_unlock_dimm(struct device *dev)
 	return rc;
 }
 
+static int nvdimm_security_change_key(struct device *dev)
+{
+	struct nvdimm *nvdimm = to_nvdimm(dev);
+	struct nvdimm_bus *nvdimm_bus = walk_to_nvdimm_bus(dev);
+	struct key *key = NULL, *old_key = NULL;
+	int rc;
+	void *old_data, *new_data;
+
+	if (!nvdimm->security_ops)
+		return 0;
+
+	if (nvdimm->state == NVDIMM_SECURITY_FROZEN)
+		return -EBUSY;
+
+	/* look for a key from keyring if exists and remove */
+	old_key = nvdimm_search_key(dev);
+	if (old_key) {
+		dev_dbg(dev, "%s: killing old key: %#x\n",
+				__func__, old_key->serial);
+		key_invalidate(old_key);
+		key_put_sync(old_key);
+	}
+
+	/* request new key from userspace */
+	key = nvdimm_request_key(dev);
+	if (!key) {
+		dev_dbg(dev, "%s: failed to acquire new key\n", __func__);
+		return -ENXIO;
+	}
+
+	dev_dbg(dev, "%s: new key: %#x\n", __func__, key->serial);
+
+	if (key->datalen == NVDIMM_PASSPHRASE_LEN) {
+		old_data = NULL;
+		new_data = key->payload.data[0];
+	} else if (key->datalen == (NVDIMM_PASSPHRASE_LEN * 2)) {
+		new_data = key->payload.data[0];
+		old_data = new_data + NVDIMM_PASSPHRASE_LEN;
+	} else {
+		key_invalidate(key);
+		key_put(key);
+		dev_warn(dev, "Incorrect key payload size for update: %u\n",
+				key->datalen);
+		return -EINVAL;
+	}
+
+	down_read(&key->sem);
+	rc = nvdimm->security_ops->change_key(nvdimm_bus, nvdimm, old_data,
+			new_data);
+	if (rc == 0 && key->datalen == (NVDIMM_PASSPHRASE_LEN * 2))
+		memcpy(old_data, new_data, NVDIMM_PASSPHRASE_LEN);
+	up_read(&key->sem);
+
+	/*
+	 * If we successfully update, link the new key to our keyring,
+	 * otherwise, nuke the key.
+	 */
+	if (rc == 0)
+		key_link(nvdimm_cred->thread_keyring, key);
+	else
+		key_invalidate(key);
+
+	key_put(key);
+	nvdimm_security_get_state(dev);
+	return rc;
+}
+
 /*
  * Retrieve bus and dimm handle and return if this bus supports
  * get_config_data commands
@@ -528,11 +595,58 @@ static ssize_t available_slots_show(struct device *dev,
 }
 static DEVICE_ATTR_RO(available_slots);
 
+static ssize_t security_show(struct device *dev,
+		struct device_attribute *attr, char *buf)
+{
+	struct nvdimm *nvdimm = to_nvdimm(dev);
+
+	switch (nvdimm->state) {
+	case NVDIMM_SECURITY_DISABLED:
+		return sprintf(buf, "disabled\n");
+	case NVDIMM_SECURITY_UNLOCKED:
+		return sprintf(buf, "unlocked\n");
+	case NVDIMM_SECURITY_LOCKED:
+		return sprintf(buf, "locked\n");
+	case NVDIMM_SECURITY_FROZEN:
+		return sprintf(buf, "frozen\n");
+	case NVDIMM_SECURITY_UNSUPPORTED:
+	default:
+		return sprintf(buf, "unsupported\n");
+	}
+
+	return -ENOTTY;
+}
+
+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;
+
+        wait_nvdimm_bus_probe_idle(&nvdimm_bus->dev);
+        if (atomic_read(&nvdimm->busy))
+                return -EBUSY;
+
+	if (sysfs_streq(buf, "update"))
+		rc = nvdimm_security_change_key(dev);
+	else
+		return -EINVAL;
+
+	if (rc == 0)
+		rc = len;
+
+	return rc;
+}
+static DEVICE_ATTR_RW(security);
+
 static struct attribute *nvdimm_attributes[] = {
 	&dev_attr_state.attr,
 	&dev_attr_flags.attr,
 	&dev_attr_commands.attr,
 	&dev_attr_available_slots.attr,
+	&dev_attr_security.attr,
 	NULL,
 };
 
diff --git a/include/linux/libnvdimm.h b/include/linux/libnvdimm.h
index 692217f82c6e..7889fea75281 100644
--- a/include/linux/libnvdimm.h
+++ b/include/linux/libnvdimm.h
@@ -162,6 +162,7 @@ enum nvdimm_security_state {
 	NVDIMM_SECURITY_DISABLED,
 	NVDIMM_SECURITY_UNLOCKED,
 	NVDIMM_SECURITY_LOCKED,
+	NVDIMM_SECURITY_FROZEN,
 	NVDIMM_SECURITY_UNSUPPORTED,
 };
 
@@ -178,6 +179,10 @@ struct nvdimm_security_ops {
 			enum nvdimm_security_state *state);
 	int (*unlock)(struct nvdimm_bus *nvdimm_bus,
 			struct nvdimm *nvdimm, struct nvdimm_key_data *nkey);
+	int (*change_key)(struct nvdimm_bus *nvdimm_bus,
+			struct nvdimm *nvdimm,
+			struct nvdimm_key_data *old_data,
+			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

[PATCH v5 07/12] nfit/libnvdimm: add disable passphrase support to Intel nvdimm.

[Dave Jiang]

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" to the sysfs
attribute "security". libnvdimm will support the generic disable API call.

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

diff --git a/drivers/acpi/nfit/intel.c b/drivers/acpi/nfit/intel.c
index b0a62248467d..2418f4b8c1fd 100644
--- a/drivers/acpi/nfit/intel.c
+++ b/drivers/acpi/nfit/intel.c
@@ -18,6 +18,57 @@
 #include "intel.h"
 #include "nfit.h"
 
+static int intel_dimm_security_disable(struct nvdimm_bus *nvdimm_bus,
+		struct nvdimm *nvdimm, 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, ND_INTEL_PASSPHRASE_SIZE);
+	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_update_passphrase(
 		struct nvdimm_bus *nvdimm_bus, struct nvdimm *nvdimm,
 		struct nvdimm_key_data *old_data,
@@ -205,4 +256,5 @@ 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 432bdd8d19e3..def10000c230 100644
--- a/drivers/nvdimm/dimm_devs.c
+++ b/drivers/nvdimm/dimm_devs.c
@@ -125,6 +125,45 @@ int nvdimm_security_get_state(struct device *dev)
 			&nvdimm->state);
 }
 
+static int nvdimm_security_disable(struct device *dev)
+{
+	struct nvdimm *nvdimm = to_nvdimm(dev);
+	struct nvdimm_bus *nvdimm_bus = walk_to_nvdimm_bus(dev);
+	struct key *key;
+	int rc;
+	void *payload;
+	bool cached_key = false;
+
+	if (!nvdimm->security_ops)
+		return 0;
+
+	if (nvdimm->state == NVDIMM_SECURITY_UNSUPPORTED)
+		return 0;
+
+	key = nvdimm_search_key(dev);
+	if (!key)
+		key = nvdimm_request_key(dev);
+	else
+		cached_key = true;
+	if (!key)
+		return -ENXIO;
+
+	down_read(&key->sem);
+	payload = key->payload.data[0];
+	rc = nvdimm->security_ops->disable(nvdimm_bus, nvdimm, payload);
+	up_read(&key->sem);
+	if (rc < 0)
+		goto out;
+
+	/* If we succeed then remove the key */
+	key_invalidate(key);
+
+ out:
+	key_put(key);
+	nvdimm_security_get_state(dev);
+	return rc;
+}
+
 int nvdimm_security_unlock_dimm(struct device *dev)
 {
 	struct nvdimm *nvdimm = to_nvdimm(dev);
@@ -631,6 +670,8 @@ static ssize_t security_store(struct device *dev,
 
 	if (sysfs_streq(buf, "update"))
 		rc = nvdimm_security_change_key(dev);
+	else if (sysfs_streq(buf, "disable"))
+		rc = nvdimm_security_disable(dev);
 	else
 		return -EINVAL;
 
diff --git a/include/linux/libnvdimm.h b/include/linux/libnvdimm.h
index 7889fea75281..59ad04261f34 100644
--- a/include/linux/libnvdimm.h
+++ b/include/linux/libnvdimm.h
@@ -183,6 +183,8 @@ struct nvdimm_security_ops {
 			struct nvdimm *nvdimm,
 			struct nvdimm_key_data *old_data,
 			struct nvdimm_key_data *new_data);
+	int (*disable)(struct nvdimm_bus *nvdimm_bus,
+			struct nvdimm *nvdimm, 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

[PATCH v5 08/12] nfit/libnvdimm: add freeze security support to Intel nvdimm

[Dave Jiang]

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>
---
 drivers/acpi/nfit/intel.c  |   51 ++++++++++++++++++++++++++++++++++++++++++++
 drivers/nvdimm/dimm_devs.c |   22 +++++++++++++++++++
 include/linux/libnvdimm.h  |    2 ++
 3 files changed, 75 insertions(+)

diff --git a/drivers/acpi/nfit/intel.c b/drivers/acpi/nfit/intel.c
index 2418f4b8c1fd..0ab56f03ebc4 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, struct nvdimm_key_data *nkey)
 {
@@ -241,6 +288,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
@@ -257,4 +307,5 @@ 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 def10000c230..6a653476bb7c 100644
--- a/drivers/nvdimm/dimm_devs.c
+++ b/drivers/nvdimm/dimm_devs.c
@@ -125,6 +125,26 @@ int nvdimm_security_get_state(struct device *dev)
 			&nvdimm->state);
 }
 
+static 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 0;
+
+	if (nvdimm->state == NVDIMM_SECURITY_UNSUPPORTED)
+		return 0;
+
+	rc = nvdimm->security_ops->freeze_lock(nvdimm_bus, nvdimm);
+	if (rc < 0)
+		return rc;
+
+	nvdimm_security_get_state(dev);
+	return 0;
+}
+
 static int nvdimm_security_disable(struct device *dev)
 {
 	struct nvdimm *nvdimm = to_nvdimm(dev);
@@ -672,6 +692,8 @@ static ssize_t security_store(struct device *dev,
 		rc = nvdimm_security_change_key(dev);
 	else if (sysfs_streq(buf, "disable"))
 		rc = nvdimm_security_disable(dev);
+	else if (sysfs_streq(buf, "freeze"))
+		rc = nvdimm_security_freeze_lock(dev);
 	else
 		return -EINVAL;
 
diff --git a/include/linux/libnvdimm.h b/include/linux/libnvdimm.h
index 59ad04261f34..1836599ed5b8 100644
--- a/include/linux/libnvdimm.h
+++ b/include/linux/libnvdimm.h
@@ -185,6 +185,8 @@ struct nvdimm_security_ops {
 			struct nvdimm_key_data *new_data);
 	int (*disable)(struct nvdimm_bus *nvdimm_bus,
 			struct nvdimm *nvdimm, 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

[PATCH v5 09/12] nfit/libnvdimm: add support for issue secure erase DSM to Intel nvdimm

[Dave Jiang]

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" is written to the "security"
sysfs attribute.  libnvdimm will support the erase generic API call.

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

diff --git a/drivers/acpi/nfit/intel.c b/drivers/acpi/nfit/intel.c
index 0ab56f03ebc4..6449db6db85d 100644
--- a/drivers/acpi/nfit/intel.c
+++ b/drivers/acpi/nfit/intel.c
@@ -18,6 +18,60 @@
 #include "intel.h"
 #include "nfit.h"
 
+static int intel_dimm_security_erase(struct nvdimm_bus *nvdimm_bus,
+		struct nvdimm *nvdimm, 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;
+
+	memcpy(nd_cmd.cmd.passphrase, nkey->data, ND_INTEL_PASSPHRASE_SIZE);
+	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 unlocked, 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)
 {
@@ -308,4 +362,5 @@ 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 6a653476bb7c..ed56649bc971 100644
--- a/drivers/nvdimm/dimm_devs.c
+++ b/drivers/nvdimm/dimm_devs.c
@@ -125,6 +125,51 @@ int nvdimm_security_get_state(struct device *dev)
 			&nvdimm->state);
 }
 
+static int nvdimm_security_erase(struct device *dev)
+{
+	struct nvdimm *nvdimm = to_nvdimm(dev);
+	struct nvdimm_bus *nvdimm_bus = walk_to_nvdimm_bus(dev);
+	struct key *key;
+	void *payload;
+	int rc = 0;
+
+	if (!nvdimm->security_ops)
+		return 0;
+
+	/* lock the device and disallow driver bind */
+	device_lock(dev);
+	/* No driver data means dimm is disabled. Proceed if so. */
+	if (dev_get_drvdata(dev)) {
+		dev_warn(dev, "Unable to secure erase while DIMM active.\n");
+		rc = -EINVAL;
+		goto out;
+	}
+
+	if (nvdimm->state == NVDIMM_SECURITY_UNSUPPORTED)
+		goto out;
+
+	key = nvdimm_search_key(dev);
+	if (!key)
+		key = nvdimm_request_key(dev);
+	if (!key) {
+		rc = -ENXIO;
+		goto out;
+	}
+
+	down_read(&key->sem);
+	payload = key->payload.data[0];
+	rc = nvdimm->security_ops->erase(nvdimm_bus, nvdimm, payload);
+	up_read(&key->sem);
+	/* remove key since secure erase kills the passphrase */
+	key_invalidate(key);
+	key_put(key);
+
+ out:
+	device_unlock(dev);
+	nvdimm_security_get_state(dev);
+	return rc;
+}
+
 static int nvdimm_security_freeze_lock(struct device *dev)
 {
 	struct nvdimm *nvdimm = to_nvdimm(dev);
@@ -694,6 +739,8 @@ static ssize_t security_store(struct device *dev,
 		rc = nvdimm_security_disable(dev);
 	else if (sysfs_streq(buf, "freeze"))
 		rc = nvdimm_security_freeze_lock(dev);
+	else if (sysfs_streq(buf, "erase"))
+		rc = nvdimm_security_erase(dev);
 	else
 		return -EINVAL;
 
diff --git a/include/linux/libnvdimm.h b/include/linux/libnvdimm.h
index 1836599ed5b8..1ac5acb3c457 100644
--- a/include/linux/libnvdimm.h
+++ b/include/linux/libnvdimm.h
@@ -187,6 +187,8 @@ struct nvdimm_security_ops {
 			struct nvdimm *nvdimm, 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, 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

[PATCH v5 10/12] nfit_test: add context to dimm_dev for nfit_test

[Dave Jiang]

In order to access the nfit_test context via sideband sysfs knobs, the
dimm_dev needs to be more than struct device in order to point back to
struct nfit_test. Wrapping the original struct device with a struct
nfit_dimm_dev and saving the nfit_test as private driver data. Also
changing the nfit_mem to be a member of struct nfit_dimm_dev instead of
saving as private driver data of that device. This is in preparation for
adding security DSM support and allowing the locking of DIMMs for testing
via sideband.

Signed-off-by: Dave Jiang <dave.jiang@intel.com>
Reviewed-by: Dan Williams <dan.j.williams@intel.com>
---
 tools/testing/nvdimm/test/nfit.c |   41 +++++++++++++++++++++++++-------------
 1 file changed, 27 insertions(+), 14 deletions(-)

diff --git a/tools/testing/nvdimm/test/nfit.c b/tools/testing/nvdimm/test/nfit.c
index 18157b0c0d0d..7b3ce2194889 100644
--- a/tools/testing/nvdimm/test/nfit.c
+++ b/tools/testing/nvdimm/test/nfit.c
@@ -150,6 +150,11 @@ struct nfit_test_fw {
 	u64 end_time;
 };
 
+struct nfit_dimm_dev {
+	struct device dev;
+	struct nfit_mem *nfit_mem;
+};
+
 struct nfit_test {
 	struct acpi_nfit_desc acpi_desc;
 	struct platform_device pdev;
@@ -181,7 +186,7 @@ struct nfit_test {
 		unsigned long deadline;
 		spinlock_t lock;
 	} ars_state;
-	struct device *dimm_dev[NUM_DCR];
+	struct nfit_dimm_dev dimm_dev[NUM_DCR];
 	struct nd_intel_smart *smart;
 	struct nd_intel_smart_threshold *smart_threshold;
 	struct badrange badrange;
@@ -981,7 +986,8 @@ static int nfit_test_ctl(struct nvdimm_bus_descriptor *nd_desc,
 						&t->smart_threshold[i -
 							t->dcr_idx],
 						&t->smart[i - t->dcr_idx],
-						&t->pdev.dev, t->dimm_dev[i]);
+						&t->pdev.dev,
+						&t->dimm_dev[i].dev);
 				break;
 			case ND_INTEL_SMART_INJECT:
 				rc = nfit_test_cmd_smart_inject(buf,
@@ -989,7 +995,8 @@ static int nfit_test_ctl(struct nvdimm_bus_descriptor *nd_desc,
 						&t->smart_threshold[i -
 							t->dcr_idx],
 						&t->smart[i - t->dcr_idx],
-						&t->pdev.dev, t->dimm_dev[i]);
+						&t->pdev.dev,
+						&t->dimm_dev[i].dev);
 				break;
 			default:
 				return -ENOTTY;
@@ -1187,8 +1194,7 @@ static void put_dimms(void *data)
 	int i;
 
 	for (i = 0; i < t->num_dcr; i++)
-		if (t->dimm_dev[i])
-			device_unregister(t->dimm_dev[i]);
+		device_unregister(&t->dimm_dev[i].dev);
 }
 
 static struct class *nfit_test_dimm;
@@ -1290,19 +1296,27 @@ static const struct attribute_group *nfit_test_dimm_attribute_groups[] = {
 	NULL,
 };
 
+static void dimm_dev_release(struct device *dev)
+{
+}
+
 static int nfit_test_dimm_init(struct nfit_test *t)
 {
-	int i;
+	int i, rc;
 
 	if (devm_add_action_or_reset(&t->pdev.dev, put_dimms, t))
 		return -ENOMEM;
 	for (i = 0; i < t->num_dcr; i++) {
-		t->dimm_dev[i] = device_create_with_groups(nfit_test_dimm,
-				&t->pdev.dev, 0, NULL,
-				nfit_test_dimm_attribute_groups,
-				"test_dimm%d", i + t->dcr_idx);
-		if (!t->dimm_dev[i])
-			return -ENOMEM;
+		t->dimm_dev[i].dev.parent = &t->pdev.dev;
+		dev_set_name(&t->dimm_dev[i].dev, "test_dimm%d",
+				i + t->dcr_idx);
+		t->dimm_dev[i].dev.class = nfit_test_dimm;
+		t->dimm_dev[i].dev.groups = nfit_test_dimm_attribute_groups;
+		t->dimm_dev[i].dev.release = dimm_dev_release;
+		rc = device_register(&t->dimm_dev[i].dev);
+		if (rc < 0)
+			return rc;
+		dev_set_drvdata(&t->dimm_dev[i].dev, t);
 	}
 	return 0;
 }
@@ -2665,8 +2679,7 @@ static int nfit_test_probe(struct platform_device *pdev)
 
 		for (i = 0; i < NUM_DCR; i++)
 			if (nfit_handle == handle[i])
-				dev_set_drvdata(nfit_test->dimm_dev[i],
-						nfit_mem);
+				nfit_test->dimm_dev[i].nfit_mem = nfit_mem;
 	}
 	mutex_unlock(&acpi_desc->init_mutex);
 

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

[PATCH v5 11/12] nfit_test: add test support for Intel nvdimm security DSMs

[Dave Jiang]

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. Set Passphrase to DIMM X.
1b. Disable 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>
---
 tools/testing/nvdimm/Kbuild      |    1 
 tools/testing/nvdimm/test/nfit.c |  186 ++++++++++++++++++++++++++++++++++++++
 2 files changed, 187 insertions(+)

diff --git a/tools/testing/nvdimm/Kbuild b/tools/testing/nvdimm/Kbuild
index 0392153a0009..a13670d3b389 100644
--- a/tools/testing/nvdimm/Kbuild
+++ b/tools/testing/nvdimm/Kbuild
@@ -36,6 +36,7 @@ obj-$(CONFIG_DEV_DAX) += device_dax.o
 obj-$(CONFIG_DEV_DAX_PMEM) += dax_pmem.o
 
 nfit-y := $(ACPI_SRC)/core.o
+nfit-$(CONFIG_X86) += $(ACPI_SRC)/intel.o
 nfit-$(CONFIG_X86_MCE) += $(ACPI_SRC)/mce.o
 nfit-y += acpi_nfit_test.o
 nfit-y += config_check.o
diff --git a/tools/testing/nvdimm/test/nfit.c b/tools/testing/nvdimm/test/nfit.c
index 7b3ce2194889..e5b62396c41f 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"
@@ -155,6 +156,11 @@ struct nfit_dimm_dev {
 	struct nfit_mem *nfit_mem;
 };
 
+struct nfit_test_sec {
+	u8 state;
+	u8 passphrase[32];
+};
+
 struct nfit_test {
 	struct acpi_nfit_desc acpi_desc;
 	struct platform_device pdev;
@@ -192,6 +198,7 @@ struct nfit_test {
 	struct badrange badrange;
 	struct work_struct work;
 	struct nfit_test_fw *fw;
+	struct nfit_test_sec *sec;
 };
 
 static struct workqueue_struct *nfit_wq;
@@ -899,6 +906,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 idx)
+{
+	struct device *dev = &t->pdev.dev;
+	struct nfit_test_sec *sec = &t->sec[idx];
+
+	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 idx)
+{
+	struct device *dev = &t->pdev.dev;
+	struct nfit_test_sec *sec = &t->sec[idx];
+
+	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 idx)
+{
+	struct device *dev = &t->pdev.dev;
+	struct nfit_test_sec *sec = &t->sec[idx];
+
+	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 idx)
+{
+	struct device *dev = &t->pdev.dev;
+	struct nfit_test_sec *sec = &t->sec[idx];
+
+	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 idx)
+{
+	struct device *dev = &t->pdev.dev;
+	struct nfit_test_sec *sec = &t->sec[idx];
+
+	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 idx)
+{
+	struct device *dev = &t->pdev.dev;
+	struct nfit_test_sec *sec = &t->sec[idx];
+
+	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;
@@ -946,6 +1085,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 - t->dcr_idx);
+				break;
+			case NVDIMM_INTEL_UNLOCK_UNIT:
+				rc = nd_intel_test_cmd_unlock_unit(t,
+						buf, buf_len, i - t->dcr_idx);
+				break;
+			case NVDIMM_INTEL_SET_PASSPHRASE:
+				rc = nd_intel_test_cmd_set_pass(t,
+						buf, buf_len, i - t->dcr_idx);
+				break;
+			case NVDIMM_INTEL_DISABLE_PASSPHRASE:
+				rc = nd_intel_test_cmd_disable_pass(t,
+						buf, buf_len, i - t->dcr_idx);
+				break;
+			case NVDIMM_INTEL_FREEZE_LOCK:
+				rc = nd_intel_test_cmd_freeze_lock(t,
+						buf, buf_len, i - t->dcr_idx);
+				break;
+			case NVDIMM_INTEL_SECURE_ERASE:
+				rc = nd_intel_test_cmd_secure_erase(t,
+						buf, buf_len, i - t->dcr_idx);
+				break;
 			case ND_INTEL_ENABLE_LSS_STATUS:
 				rc = nd_intel_test_cmd_set_lss_status(t,
 						buf, buf_len);
@@ -1280,10 +1443,23 @@ 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 *t = dev_get_drvdata(dev);
+	struct nfit_test_sec *sec = &t->sec[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,
 };
 
@@ -2192,6 +2368,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)
@@ -2611,6 +2795,8 @@ static int nfit_test_probe(struct platform_device *pdev)
 				GFP_KERNEL);
 		nfit_test->fw = devm_kcalloc(dev, num,
 				sizeof(struct nfit_test_fw), GFP_KERNEL);
+		nfit_test->sec = devm_kcalloc(dev, num,
+				sizeof(struct nfit_test_sec), GFP_KERNEL);
 		if (nfit_test->dimm && nfit_test->dimm_dma && nfit_test->label
 				&& nfit_test->label_dma && nfit_test->dcr
 				&& nfit_test->dcr_dma && nfit_test->flush

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

[PATCH v5 12/12] libnvdimm: add documentation for nvdimm security support

[Dave Jiang]

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

Signed-off-by: Dave Jiang <dave.jiang@intel.com>
---
 Documentation/nvdimm/security |   70 +++++++++++++++++++++++++++++++++++++++++
 1 file changed, 70 insertions(+)
 create mode 100644 Documentation/nvdimm/security

diff --git a/Documentation/nvdimm/security b/Documentation/nvdimm/security
new file mode 100644
index 000000000000..b776ef1f5e41
--- /dev/null
+++ b/Documentation/nvdimm/security
@@ -0,0 +1,70 @@
+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 - enable security. Add or update current key.
+disable - disable enabled security and remove key.
+freeze - freeze changing of security states.
+erase - 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.
+
+The payload provided to the key can be a 32bytes payload or 64bytes payload
+when doing an "update". The payload is viewed as 64 bytes in the following
+format:
+[32 bytes new key data zero padded][32 bytes current key data zero padded]
+However, a 32bytes payload can be provided and will be assumed as the old
+key to be 32 bytes of 0s and the provided 32bytes payload is the new key.
+It is up to the user upcall function how that's presented as the key payload
+to the kernel.
+
+All the other security functions that require a provided key can accept a
+32bytes payload or 64bytes. If the payload is 64bytes, then second 32bytes
+will be ignored and the first 32bytes contains the expected "passphrase".
+
+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.
+
+[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

Re: [PATCH v5 00/12] Adding security support for nvdimm

[Eric Biggers]

On Tue, Jul 17, 2018 at 01:54:04PM -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.
> 
> Security folks, thanks in advance for taking a look at my key management
> implementation and making sure that I'm doing something sane. Mainly you'll
> want to review patches 2, 4, 5, and 6 as most relevant ones that need scrutiny.
> 
> 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.
> 
> ---
> 
> Dave Jiang (12):
>       nfit: add support for Intel DSM 1.7 commands
>       libnvdimm: create keyring to store security keys
>       nfit/libnvdimm: store dimm id as a member to struct nvdimm
>       nfit/libnvdimm: add unlock of nvdimm support for Intel DIMMs
>       keys: add call key_put_sync() to flush key_gc_work when doing a key_put().
>       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 context to dimm_dev for nfit_test
>       nfit_test: add test support for Intel nvdimm security DSMs
>       libnvdimm: add documentation for nvdimm security support
> 
> 
>  Documentation/nvdimm/security    |   70 ++++++
>  drivers/acpi/nfit/Makefile       |    1 
>  drivers/acpi/nfit/core.c         |   58 ++++-
>  drivers/acpi/nfit/intel.c        |  366 ++++++++++++++++++++++++++++++++
>  drivers/acpi/nfit/intel.h        |   83 +++++++
>  drivers/acpi/nfit/nfit.h         |   20 ++
>  drivers/nvdimm/bus.c             |    2 
>  drivers/nvdimm/core.c            |    7 +
>  drivers/nvdimm/dimm.c            |    7 +
>  drivers/nvdimm/dimm_devs.c       |  430 ++++++++++++++++++++++++++++++++++++++
>  drivers/nvdimm/nd-core.h         |    4 
>  drivers/nvdimm/nd.h              |    2 
>  include/linux/key.h              |    1 
>  include/linux/libnvdimm.h        |   41 +++-
>  security/keys/key.c              |   35 +++
>  tools/testing/nvdimm/Kbuild      |    1 
>  tools/testing/nvdimm/test/nfit.c |  227 +++++++++++++++++++-
>  17 files changed, 1315 insertions(+), 40 deletions(-)
>  create mode 100644 Documentation/nvdimm/security
>  create mode 100644 drivers/acpi/nfit/intel.c
>  create mode 100644 drivers/acpi/nfit/intel.h
> 

Which git tree does this series apply to?  I tried upstream, linux-next, and
linux-block/for-next, but in all cases patch 4 doesn't apply:

Applying: nfit: add support for Intel DSM 1.7 commands
Applying: libnvdimm: create keyring to store security keys
Applying: nfit/libnvdimm: store dimm id as a member to struct nvdimm
Applying: nfit/libnvdimm: add unlock of nvdimm support for Intel DIMMs
error: sha1 information is lacking or useless (drivers/acpi/nfit/core.c).
error: could not build fake ancestor
Patch failed at 0004 nfit/libnvdimm: add unlock of nvdimm support for Intel DIMMs
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

Re: [PATCH v5 00/12] Adding security support for nvdimm

[Dave Jiang]

On 07/17/2018 04:26 PM, Eric Biggers wrote:
> On Tue, Jul 17, 2018 at 01:54:04PM -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.
>>
>> Security folks, thanks in advance for taking a look at my key management
>> implementation and making sure that I'm doing something sane. Mainly you'll
>> want to review patches 2, 4, 5, and 6 as most relevant ones that need scrutiny.
>>
>> 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.
>>
>> ---
>>
>> Dave Jiang (12):
>>       nfit: add support for Intel DSM 1.7 commands
>>       libnvdimm: create keyring to store security keys
>>       nfit/libnvdimm: store dimm id as a member to struct nvdimm
>>       nfit/libnvdimm: add unlock of nvdimm support for Intel DIMMs
>>       keys: add call key_put_sync() to flush key_gc_work when doing a key_put().
>>       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 context to dimm_dev for nfit_test
>>       nfit_test: add test support for Intel nvdimm security DSMs
>>       libnvdimm: add documentation for nvdimm security support
>>
>>
>>  Documentation/nvdimm/security    |   70 ++++++
>>  drivers/acpi/nfit/Makefile       |    1 
>>  drivers/acpi/nfit/core.c         |   58 ++++-
>>  drivers/acpi/nfit/intel.c        |  366 ++++++++++++++++++++++++++++++++
>>  drivers/acpi/nfit/intel.h        |   83 +++++++
>>  drivers/acpi/nfit/nfit.h         |   20 ++
>>  drivers/nvdimm/bus.c             |    2 
>>  drivers/nvdimm/core.c            |    7 +
>>  drivers/nvdimm/dimm.c            |    7 +
>>  drivers/nvdimm/dimm_devs.c       |  430 ++++++++++++++++++++++++++++++++++++++
>>  drivers/nvdimm/nd-core.h         |    4 
>>  drivers/nvdimm/nd.h              |    2 
>>  include/linux/key.h              |    1 
>>  include/linux/libnvdimm.h        |   41 +++-
>>  security/keys/key.c              |   35 +++
>>  tools/testing/nvdimm/Kbuild      |    1 
>>  tools/testing/nvdimm/test/nfit.c |  227 +++++++++++++++++++-
>>  17 files changed, 1315 insertions(+), 40 deletions(-)
>>  create mode 100644 Documentation/nvdimm/security
>>  create mode 100644 drivers/acpi/nfit/intel.c
>>  create mode 100644 drivers/acpi/nfit/intel.h
>>
> 
> Which git tree does this series apply to?  I tried upstream, linux-next, and
> linux-block/for-next, but in all cases patch 4 doesn't apply:
> 
> Applying: nfit: add support for Intel DSM 1.7 commands
> Applying: libnvdimm: create keyring to store security keys
> Applying: nfit/libnvdimm: store dimm id as a member to struct nvdimm
> Applying: nfit/libnvdimm: add unlock of nvdimm support for Intel DIMMs
> error: sha1 information is lacking or useless (drivers/acpi/nfit/core.c).
> error: could not build fake ancestor
> Patch failed at 0004 nfit/libnvdimm: add unlock of nvdimm support for Intel DIMMs
> 

You can grab it here
https://git.kernel.org/pub/scm/linux/kernel/git/djiang/linux.git/log/?h=nvdimm-security

I based my stuff on top of couple patches from Dan that has to do with
locked DIMM label reading. And those are queued for 4.19.
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

Re: [PATCH v5 05/12] keys: add call key_put_sync() to flush key_gc_work when doing a key_put().

[Eric Biggers]

On Tue, Jul 17, 2018 at 01:54:32PM -0700, Dave Jiang wrote:
> Adding key_put_sync() that calls flush_work on key_gc_work to ensure the
> key we want to remove is gone.
> 
> Signed-off-by: Dave Jiang <dave.jiang@intel.com>

Why is this needed?  Isn't key_unlink() or key_invalidate() enough?

- Eric

> ---
>  include/linux/key.h |    1 +
>  security/keys/key.c |   35 +++++++++++++++++++++++++++++------
>  2 files changed, 30 insertions(+), 6 deletions(-)
> 
> diff --git a/include/linux/key.h b/include/linux/key.h
> index e58ee10f6e58..4dffbd23d4e4 100644
> --- a/include/linux/key.h
> +++ b/include/linux/key.h
> @@ -253,6 +253,7 @@ extern struct key *key_alloc(struct key_type *type,
>  extern void key_revoke(struct key *key);
>  extern void key_invalidate(struct key *key);
>  extern void key_put(struct key *key);
> +extern void key_put_sync(struct key *key);
>  
>  static inline struct key *__key_get(struct key *key)
>  {
> diff --git a/security/keys/key.c b/security/keys/key.c
> index d97c9394b5dd..b6f3ec19ab0f 100644
> --- a/security/keys/key.c
> +++ b/security/keys/key.c
> @@ -622,6 +622,19 @@ int key_reject_and_link(struct key *key,
>  }
>  EXPORT_SYMBOL(key_reject_and_link);
>  
> +static void __key_put(struct key *key, bool sync)
> +{
> +	if (key) {
> +		key_check(key);
> +
> +		if (refcount_dec_and_test(&key->usage)) {
> +			schedule_work(&key_gc_work);
> +			if (sync)
> +				flush_work(&key_gc_work);
> +		}
> +	}
> +}
> +
>  /**
>   * key_put - Discard a reference to a key.
>   * @key: The key to discard a reference from.
> @@ -632,15 +645,25 @@ EXPORT_SYMBOL(key_reject_and_link);
>   */
>  void key_put(struct key *key)
>  {
> -	if (key) {
> -		key_check(key);
> -
> -		if (refcount_dec_and_test(&key->usage))
> -			schedule_work(&key_gc_work);
> -	}
> +	__key_put(key, false);
>  }
>  EXPORT_SYMBOL(key_put);
>  
> +/**
> + * key_put - Discard a reference to a key and flush the key gc work item.
> + * @key: The key to discard a reference from.
> + *
> + * Discard a reference to a key, and when all the references are gone, we
> + * schedule the cleanup task to come and pull it out of the tree in process
> + * context at some later time. This call flushes the clean up so we are sure
> + * that the key is gone.
> + */
> +void key_put_sync(struct key *key)
> +{
> +	__key_put(key, true);
> +}
> +EXPORT_SYMBOL(key_put_sync);
> +
>  /*
>   * Find a key by its serial number.
>   */
> 
> --
> To unsubscribe from this list: send the line "unsubscribe keyrings" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

Re: [PATCH v5 02/12] libnvdimm: create keyring to store security keys

[Eric Biggers]

Just a few superficial comments:

On Tue, Jul 17, 2018 at 01:54:15PM -0700, Dave Jiang wrote:
> Prepping the libnvdimm to support security management by adding a keyring
> in order to provide passphrase management through the kernel key management
> APIs.
> 
> Signed-off-by: Dave Jiang <dave.jiang@intel.com>
> Reviewed-by: Dan Williams <dan.j.williams@intel.com>
> ---
>  drivers/nvdimm/core.c      |    7 +++
>  drivers/nvdimm/dimm_devs.c |   96 ++++++++++++++++++++++++++++++++++++++++++++
>  drivers/nvdimm/nd-core.h   |    1 
>  include/linux/libnvdimm.h  |    5 ++
>  4 files changed, 107 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/nvdimm/core.c b/drivers/nvdimm/core.c
> index acce050856a8..3cd33d5c7cf0 100644
> --- a/drivers/nvdimm/core.c
> +++ b/drivers/nvdimm/core.c
> @@ -437,9 +437,12 @@ static __init int libnvdimm_init(void)
>  {
>  	int rc;
>  
> -	rc = nvdimm_bus_init();
> +	rc = nvdimm_devs_init();
>  	if (rc)
>  		return rc;
> +	rc = nvdimm_bus_init();
> +	if (rc)
> +		goto err_bus;
>  	rc = nvdimm_init();
>  	if (rc)
>  		goto err_dimm;
> @@ -454,6 +457,8 @@ static __init int libnvdimm_init(void)
>  	nvdimm_exit();
>   err_dimm:
>  	nvdimm_bus_exit();
> + err_bus:
> +	nvdimm_devs_exit();
>  	return rc;
>  }
>  
> diff --git a/drivers/nvdimm/dimm_devs.c b/drivers/nvdimm/dimm_devs.c
> index 8d348b22ba45..1dcbb653455b 100644
> --- a/drivers/nvdimm/dimm_devs.c
> +++ b/drivers/nvdimm/dimm_devs.c
> @@ -18,12 +18,54 @@
>  #include <linux/io.h>
>  #include <linux/fs.h>
>  #include <linux/mm.h>
> +#include <linux/cred.h>
> +#include <keys/user-type.h>
> +#include <linux/key-type.h>
> +#include <linux/keyctl.h>
>  #include "nd-core.h"
>  #include "label.h"
>  #include "pmem.h"
>  #include "nd.h"
>  
>  static DEFINE_IDA(dimm_ida);
> +static const struct cred *nvdimm_cred;
> +
> +static int nvdimm_key_instantiate(struct key *key,
> +		struct key_preparsed_payload *prep);
> +static void nvdimm_key_destroy(struct key *key);
> +
> +struct key_type nvdimm_key_type = {
> +	.name = "nvdimm",
> +	.instantiate = nvdimm_key_instantiate,
> +	.destroy = nvdimm_key_destroy,
> +	.describe = user_describe,
> +	/*
> +	 * The reason to have default data length to be len*2 is to
> +	 * accomodate the payload when we are doing a key change where
> +	 * the we stuff the old key and new key in the same payload.
> +	 */
> +	.def_datalen = NVDIMM_PASSPHRASE_LEN * 2,
> +};
> +
> +static int nvdimm_key_instantiate(struct key *key,
> +		struct key_preparsed_payload *prep)
> +{
> +	char *payload;
> +
> +	payload = kzalloc(nvdimm_key_type.def_datalen, GFP_KERNEL);
> +	if (!payload)
> +		return -ENOMEM;
> +
> +	key->datalen = min(nvdimm_key_type.def_datalen, prep->datalen);
> +	memcpy(payload, prep->data, key->datalen);
> +	key->payload.data[0] = payload;
> +	return 0;
> +}

This should return an error code if the user provided a too long payload, rather
than silently truncating it.

> +
> +static void nvdimm_key_destroy(struct key *key)
> +{
> +	kfree(key->payload.data[0]);
> +}

kzfree()

> -void __exit nvdimm_devs_exit(void)
> +static int nvdimm_register_keyring(void)
> +{
> +	struct cred *cred;
> +	struct key *keyring;
> +	int rc;
> +
> +	rc = register_key_type(&nvdimm_key_type);
> +	if (rc < 0)
> +		return rc;
> +
> +	cred = prepare_kernel_cred(NULL);
> +	if (!cred) {
> +		rc = -ENOMEM;
> +		goto failed_cred;
> +	}
> +
> +	keyring = keyring_alloc(".nvdimm",
> +			GLOBAL_ROOT_UID, GLOBAL_ROOT_GID, cred,
> +			(KEY_POS_ALL & ~KEY_POS_SETATTR) |
> +			(KEY_USR_ALL & ~KEY_USR_SETATTR),
> +			KEY_ALLOC_NOT_IN_QUOTA, NULL, NULL);
> +	if (IS_ERR(keyring)) {
> +		rc = PTR_ERR(keyring);
> +		goto failed_keyring;
> +	}
> +
> +	set_bit(KEY_FLAG_ROOT_CAN_CLEAR, &keyring->flags);
> +	cred->thread_keyring = keyring;
> +	cred->jit_keyring = KEY_REQKEY_DEFL_THREAD_KEYRING;
> +	nvdimm_cred = cred;
> +	return 0;
> +
> + failed_cred:
> +	unregister_key_type(&nvdimm_key_type);
> + failed_keyring:
> +	put_cred(cred);
> +	return rc;
> +}

The labels here are backwards.  It should be

failed_keyring:
	put_cred(cred);
failed_cred:
	unregister_key_type(&nvdimm_key_type);
	return rc;

> +
> +static void nvdimm_unregister_keyring(void)
> +{
> +	key_revoke(nvdimm_cred->thread_keyring);
> +	unregister_key_type(&nvdimm_key_type);
> +	put_cred(nvdimm_cred);
> +}
> +
> +int __init nvdimm_devs_init(void)
> +{
> +	return nvdimm_register_keyring();
> +}
> +
> +void  nvdimm_devs_exit(void)
>  {
> +	nvdimm_unregister_keyring();
>  	ida_destroy(&dimm_ida);
>  }
> diff --git a/drivers/nvdimm/nd-core.h b/drivers/nvdimm/nd-core.h
> index 79274ead54fb..2af0f89c4010 100644
> --- a/drivers/nvdimm/nd-core.h
> +++ b/drivers/nvdimm/nd-core.h
> @@ -76,6 +76,7 @@ static inline bool is_memory(struct device *dev)
>  struct nvdimm_bus *walk_to_nvdimm_bus(struct device *nd_dev);
>  int __init nvdimm_bus_init(void);
>  void nvdimm_bus_exit(void);
> +int nvdimm_devs_init(void);
>  void nvdimm_devs_exit(void);
>  void nd_region_devs_exit(void);
>  void nd_region_probe_success(struct nvdimm_bus *nvdimm_bus, struct device *dev);
> diff --git a/include/linux/libnvdimm.h b/include/linux/libnvdimm.h
> index 472171af7f60..09dd06f96f95 100644
> --- a/include/linux/libnvdimm.h
> +++ b/include/linux/libnvdimm.h
> @@ -155,6 +155,11 @@ static inline struct nd_blk_region_desc *to_blk_region_desc(
>  
>  }
>  
> +extern struct key_type nvdimm_key_type;
> +
> +#define NVDIMM_PASSPHRASE_LEN		32
> +#define NVDIMM_KEY_DESC_LEN		25
> +
>  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,
> 
> --
> To unsubscribe from this list: send the line "unsubscribe keyrings" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

Re: [PATCH v5 05/12] keys: add call key_put_sync() to flush key_gc_work when doing a key_put().

[Dave Jiang]

On 07/17/2018 04:53 PM, Eric Biggers wrote:
> On Tue, Jul 17, 2018 at 01:54:32PM -0700, Dave Jiang wrote:
>> Adding key_put_sync() that calls flush_work on key_gc_work to ensure the
>> key we want to remove is gone.
>>
>> Signed-off-by: Dave Jiang <dave.jiang@intel.com>
> 
> Why is this needed?  Isn't key_unlink() or key_invalidate() enough?

Without this, I was getting -EKEYREVOKED when calling request_key() and
it wasn't pulling from userspace, even with key_invalidate() called on
the key. I assume it has something to do with the garbage collector not
run yet and it's still finding the old key? With this I don't hit that
anymore. Please let me know if I'm doing something wrong with the sequence.

> 
> - Eric
> 
>> ---
>>  include/linux/key.h |    1 +
>>  security/keys/key.c |   35 +++++++++++++++++++++++++++++------
>>  2 files changed, 30 insertions(+), 6 deletions(-)
>>
>> diff --git a/include/linux/key.h b/include/linux/key.h
>> index e58ee10f6e58..4dffbd23d4e4 100644
>> --- a/include/linux/key.h
>> +++ b/include/linux/key.h
>> @@ -253,6 +253,7 @@ extern struct key *key_alloc(struct key_type *type,
>>  extern void key_revoke(struct key *key);
>>  extern void key_invalidate(struct key *key);
>>  extern void key_put(struct key *key);
>> +extern void key_put_sync(struct key *key);
>>  
>>  static inline struct key *__key_get(struct key *key)
>>  {
>> diff --git a/security/keys/key.c b/security/keys/key.c
>> index d97c9394b5dd..b6f3ec19ab0f 100644
>> --- a/security/keys/key.c
>> +++ b/security/keys/key.c
>> @@ -622,6 +622,19 @@ int key_reject_and_link(struct key *key,
>>  }
>>  EXPORT_SYMBOL(key_reject_and_link);
>>  
>> +static void __key_put(struct key *key, bool sync)
>> +{
>> +	if (key) {
>> +		key_check(key);
>> +
>> +		if (refcount_dec_and_test(&key->usage)) {
>> +			schedule_work(&key_gc_work);
>> +			if (sync)
>> +				flush_work(&key_gc_work);
>> +		}
>> +	}
>> +}
>> +
>>  /**
>>   * key_put - Discard a reference to a key.
>>   * @key: The key to discard a reference from.
>> @@ -632,15 +645,25 @@ EXPORT_SYMBOL(key_reject_and_link);
>>   */
>>  void key_put(struct key *key)
>>  {
>> -	if (key) {
>> -		key_check(key);
>> -
>> -		if (refcount_dec_and_test(&key->usage))
>> -			schedule_work(&key_gc_work);
>> -	}
>> +	__key_put(key, false);
>>  }
>>  EXPORT_SYMBOL(key_put);
>>  
>> +/**
>> + * key_put - Discard a reference to a key and flush the key gc work item.
>> + * @key: The key to discard a reference from.
>> + *
>> + * Discard a reference to a key, and when all the references are gone, we
>> + * schedule the cleanup task to come and pull it out of the tree in process
>> + * context at some later time. This call flushes the clean up so we are sure
>> + * that the key is gone.
>> + */
>> +void key_put_sync(struct key *key)
>> +{
>> +	__key_put(key, true);
>> +}
>> +EXPORT_SYMBOL(key_put_sync);
>> +
>>  /*
>>   * Find a key by its serial number.
>>   */
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe keyrings" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

Re: [PATCH v5 04/12] nfit/libnvdimm: add unlock of nvdimm support for Intel DIMMs

[Eric Biggers]

Again, just a couple superficial comments:

On Tue, Jul 17, 2018 at 01:54:26PM -0700, Dave Jiang 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>
> ---
>  drivers/acpi/nfit/Makefile |    1 
>  drivers/acpi/nfit/core.c   |    3 +
>  drivers/acpi/nfit/intel.c  |  151 ++++++++++++++++++++++++++++++++++++++++++++
>  drivers/acpi/nfit/intel.h  |   16 +++++
>  drivers/nvdimm/dimm.c      |    7 ++
>  drivers/nvdimm/dimm_devs.c |  108 +++++++++++++++++++++++++++++++
>  drivers/nvdimm/nd-core.h   |    2 +
>  drivers/nvdimm/nd.h        |    2 +
>  include/linux/libnvdimm.h  |   23 ++++++-
>  9 files changed, 310 insertions(+), 3 deletions(-)
>  create mode 100644 drivers/acpi/nfit/intel.c
[...]
> +
> +struct nvdimm_security_ops intel_security_ops = {
> +	.state = intel_dimm_security_state,
> +	.unlock = intel_dimm_security_unlock,
> +};

nvdimm_security_ops should be marked 'const'.

> +/*
> + * Find key in kernel keyring
> + */
> +static struct key *nvdimm_search_key(struct device *dev)
> +{
> +	struct nvdimm *nvdimm = to_nvdimm(dev);
> +	char *desc;
> +	key_ref_t keyref;
> +	struct key *key = NULL;
> +
> +	if (!nvdimm->security_ops)
> +		return NULL;
> +
> +	desc = kzalloc(NVDIMM_KEY_DESC_LEN, GFP_KERNEL);
> +	if (!desc)
> +		return NULL;
> +
> +	keyref = keyring_search(make_key_ref(nvdimm_cred->thread_keyring, 1),
> +			&nvdimm_key_type, nvdimm->dimm_id);
> +	if (IS_ERR(keyref))
> +		key = NULL;
> +	else
> +		key = key_ref_to_ptr(keyref);
> +
> +	kfree(desc);
> +	return key;
> +}

'desc' is allocated and never used.

And with that removed, NVDIMM_KEY_DESC_LEN isn't actually used either.

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

Re: [PATCH v5 05/12] keys: add call key_put_sync() to flush key_gc_work when doing a key_put().

[David Howells]

Dave Jiang <dave.jiang@intel.com> wrote:

> +static void __key_put(struct key *key, bool sync)
> +{
> +	if (key) {
> +		key_check(key);
> +
> +		if (refcount_dec_and_test(&key->usage)) {
> +			schedule_work(&key_gc_work);
> +			if (sync)
> +				flush_work(&key_gc_work);
> +		}
> +	}
> +}

I'm not sure this is a good way to go about it.  This assumes that the gc will
do an entire pass before returning to the dispatcher, but that won't
necessarily always be the case.  Further, it might take two passes to get rid
of a key that has expired, been revoked or been invalidated: the first pass
removes the links and then a second pass might be needed to remove the key,
depending on the serial number of the key relative to the serial numbers of
the keyrings linking to it.  RCU synchronisation is then performed before the
key can actually be cleaned up.

A better way might be to add a global waitqueue and then use that to watch for
an indication that a key is about to be finally destroyed.  A flag can be set
on the key to ask for the waitqueue to be poked

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

Re: [PATCH v5 02/12] libnvdimm: create keyring to store security keys

[David Howells]

Dave Jiang <dave.jiang@intel.com> wrote:

> +static int nvdimm_key_instantiate(struct key *key,
> +		struct key_preparsed_payload *prep)

Please use ->preparse() if you can rather than doing this in ->instantiate().
Ideally ->instantiate() should not return an error, particularly not -EINVAL
or -ENOMEM.

> +{
> +	char *payload;
> +
> +	payload = kzalloc(nvdimm_key_type.def_datalen, GFP_KERNEL);
> +	if (!payload)
> +		return -ENOMEM;

Ummm...  Why not just kmemdup()?  Why not use use prep->datalen here rather
than def_datalen?

> +
> +	key->datalen = min(nvdimm_key_type.def_datalen, prep->datalen);
> +	memcpy(payload, prep->data, key->datalen);
> +	key->payload.data[0] = payload;
> +	return 0;
> +}

I would recommend, firstly, that you use generic_key_instantiate() if you can;
if not, you might want to use rcu_assign_keypointer().

> +	cred = prepare_kernel_cred(NULL);
> +	if (!cred) {
> +		rc = -ENOMEM;
> +		goto failed_cred;
> +	}

I wonder if you could just use &init_cred instead.

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

Re: [PATCH v5 06/12] nfit/libnvdimm: add set passphrase support for Intel nvdimms

[David Howells]

Dave Jiang <dave.jiang@intel.com> wrote:

> Add support for setting and/or updating passphrase on the Intel nvdimms.
> The passphrase is pulled from userspace through the kernel key management.
> We trigger the update via writing "update" to the sysfs attribute
> "security". The state of the security can also be read via the "security"
> attribute. libnvdimm will generically support the key_change API call.

Btw, could you use a logon-type key rather than spinning your own?  Do you
specifically not want it to be updateable?

> +static int intel_dimm_security_update_passphrase(
> +		struct nvdimm_bus *nvdimm_bus, struct nvdimm *nvdimm,
> +		struct nvdimm_key_data *old_data,
> +		struct nvdimm_key_data *new_data)

You might want to mark old_data and new_data as const here.  At least, they
don't appear to be changed.  Also, can you stick a banner comment on the
function to say what it actually does?  Is it changing the key stored in h/w
or is it meant to be changing what's stored in the key payload?

> +	/* request new key from userspace */
> +	key = nvdimm_request_key(dev);
> +	if (!key) {
> +		dev_dbg(dev, "%s: failed to acquire new key\n", __func__);
> +		return -ENXIO;
> +	}
> +
> +	dev_dbg(dev, "%s: new key: %#x\n", __func__, key->serial);
> +
> +	if (key->datalen == NVDIMM_PASSPHRASE_LEN) {
> +		old_data = NULL;
> +		new_data = key->payload.data[0];
> +	} else if (key->datalen == (NVDIMM_PASSPHRASE_LEN * 2)) {
> +		new_data = key->payload.data[0];
> +		old_data = new_data + NVDIMM_PASSPHRASE_LEN;
> +	} else {
> +		key_invalidate(key);
> +		key_put(key);
> +		dev_warn(dev, "Incorrect key payload size for update: %u\n",
> +				key->datalen);
> +		return -EINVAL;
> +	}
> +
> +	down_read(&key->sem);
> +	rc = nvdimm->security_ops->change_key(nvdimm_bus, nvdimm, old_data,
> +			new_data);

Why do you need to get a read lock on key->sem and not a write-lock?

> +	if (rc == 0 && key->datalen == (NVDIMM_PASSPHRASE_LEN * 2))
> +		memcpy(old_data, new_data, NVDIMM_PASSPHRASE_LEN);

You appear to be changing the key content here.  If that's the case, don't you
need to write-lock key->sem?

> +	up_read(&key->sem);

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

Re: [PATCH v5 05/12] keys: add call key_put_sync() to flush key_gc_work when doing a key_put().

[David Howells]

David Howells <dhowells@redhat.com> wrote:

> A better way might be to add a global waitqueue and then use that to watch for
> an indication that a key is about to be finally destroyed.  A flag can be set
> on the key to ask for the waitqueue to be poked

Hmmm...  This wouldn't work unless you added yourself to the waitqueue before
decrementing the key usage count:-/

In fact, if someone creates a link to your key, sync'ing the gc isn't
sufficient.

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

Re: [PATCH v5 06/12] nfit/libnvdimm: add set passphrase support for Intel nvdimms

[David Howells]

Dave Jiang <dave.jiang@intel.com> wrote:

> +	/* look for a key from keyring if exists and remove */
> +	old_key = nvdimm_search_key(dev);
> +	if (old_key) {
> +		dev_dbg(dev, "%s: killing old key: %#x\n",
> +				__func__, old_key->serial);
> +		key_invalidate(old_key);
> +		key_put_sync(old_key);
> +	}

Why don't you unlink the key from the .nvdimm keyring here?

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

RE: [PATCH v5 03/12] nfit/libnvdimm: store dimm id as a member to struct nvdimm

[Elliott, Robert (Persistent Memory)]

> -----Original Message-----
> From: Linux-nvdimm [mailto:linux-nvdimm-bounces@lists.01.org] On Behalf Of Dave Jiang
> Sent: Tuesday, July 17, 2018 3:54 PM
> Subject: [PATCH v5 03/12] nfit/libnvdimm: store dimm id as a member to struct nvdimm
...
> +	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));
...
> 
> +#define NFIT_DIMM_ID_LEN	25
> +	char id[NFIT_DIMM_ID_LEN];

Since the longest ID is:
    4+2+4+8 hex characters = 18 bytes
    +3 dashes = 21 bytes
    +1 NULL = 22 bytes,

why is the define 25?

Same question for this define in patch 02/12 (although there's another comment
by Eric Biggers that this isn't really used, so this might be moot):
> +#define NVDIMM_KEY_DESC_LEN		25


---
Robert Elliott, HPE Persistent Memory




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

Re: [PATCH v5 03/12] nfit/libnvdimm: store dimm id as a member to struct nvdimm

[Dave Jiang]

On 07/18/2018 08:40 AM, Elliott, Robert (Persistent Memory) wrote:
> 
> 
>> -----Original Message-----
>> From: Linux-nvdimm [mailto:linux-nvdimm-bounces@lists.01.org] On Behalf Of Dave Jiang
>> Sent: Tuesday, July 17, 2018 3:54 PM
>> Subject: [PATCH v5 03/12] nfit/libnvdimm: store dimm id as a member to struct nvdimm
> ...
>> +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));
> ...
>>
>> +#define NFIT_DIMM_ID_LEN25
>> +char id[NFIT_DIMM_ID_LEN];
> 
> Since the longest ID is:
>     4+2+4+8 hex characters = 18 bytes
>     +3 dashes = 21 bytes
>     +1 NULL = 22 bytes,
> 
> why is the define 25?

It should be 22. I think I just arbitrarily gave it some more room. I'll
fix it.

> 
> Same question for this define in patch 02/12 (although there's another comment
> by Eric Biggers that this isn't really used, so this might be moot):
>> +#define NVDIMM_KEY_DESC_LEN25
> 
> 
> ---
> Robert Elliott, HPE Persistent Memory
> 
> 
> 
> 
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

Re: [PATCH v5 06/12] nfit/libnvdimm: add set passphrase support for Intel nvdimms

[Dave Jiang]

On 07/18/2018 04:14 AM, David Howells wrote:
> Dave Jiang <dave.jiang@intel.com> wrote:
> 
>> Add support for setting and/or updating passphrase on the Intel nvdimms.
>> The passphrase is pulled from userspace through the kernel key management.
>> We trigger the update via writing "update" to the sysfs attribute
>> "security". The state of the security can also be read via the "security"
>> attribute. libnvdimm will generically support the key_change API call.
> 
> Btw, could you use a logon-type key rather than spinning your own?  Do you
> specifically not want it to be updateable?

Thanks for reviewing this David. I'll take a look and see if I can.
Basically the code is acting as the proxy for the hardware and passing
through the passphrase. What would be the best way to update the
passphrase if I need to retrieve the new passphrase from the userspace?
The hardware still needs the old passphrase in order to verify. There
has to be a nice way to do this but I'm not familiar with key management
API to know better.

> 
>> +static int intel_dimm_security_update_passphrase(
>> +		struct nvdimm_bus *nvdimm_bus, struct nvdimm *nvdimm,
>> +		struct nvdimm_key_data *old_data,
>> +		struct nvdimm_key_data *new_data)
> 
> You might want to mark old_data and new_data as const here.  At least, they
> don't appear to be changed.  Also, can you stick a banner comment on the
> function to say what it actually does?  Is it changing the key stored in h/w
> or is it meant to be changing what's stored in the key payload?
> 
>> +	/* request new key from userspace */
>> +	key = nvdimm_request_key(dev);
>> +	if (!key) {
>> +		dev_dbg(dev, "%s: failed to acquire new key\n", __func__);
>> +		return -ENXIO;
>> +	}
>> +
>> +	dev_dbg(dev, "%s: new key: %#x\n", __func__, key->serial);
>> +
>> +	if (key->datalen == NVDIMM_PASSPHRASE_LEN) {
>> +		old_data = NULL;
>> +		new_data = key->payload.data[0];
>> +	} else if (key->datalen == (NVDIMM_PASSPHRASE_LEN * 2)) {
>> +		new_data = key->payload.data[0];
>> +		old_data = new_data + NVDIMM_PASSPHRASE_LEN;
>> +	} else {
>> +		key_invalidate(key);
>> +		key_put(key);
>> +		dev_warn(dev, "Incorrect key payload size for update: %u\n",
>> +				key->datalen);
>> +		return -EINVAL;
>> +	}
>> +
>> +	down_read(&key->sem);
>> +	rc = nvdimm->security_ops->change_key(nvdimm_bus, nvdimm, old_data,
>> +			new_data);
> 
> Why do you need to get a read lock on key->sem and not a write-lock?
> 
>> +	if (rc == 0 && key->datalen == (NVDIMM_PASSPHRASE_LEN * 2))
>> +		memcpy(old_data, new_data, NVDIMM_PASSPHRASE_LEN);
> 
> You appear to be changing the key content here.  If that's the case, don't you
> need to write-lock key->sem?
> 
>> +	up_read(&key->sem);
> 
> David
> 
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

RE: [PATCH v5 01/12] nfit: add support for Intel DSM 1.7 commands

[Elliott, Robert (Persistent Memory)]

> -----Original Message-----
> From: Linux-nvdimm [mailto:linux-nvdimm-bounces@lists.01.org] On Behalf Of Dave Jiang
> Sent: Tuesday, July 17, 2018 3:54 PM
> Subject: [PATCH v5 01/12] nfit: add support for Intel DSM 1.7 commands
...
> +#ifdef CONFIG_X86
> +
> +#define ND_INTEL_STATUS_SIZE		4
> +#define ND_INTEL_PASSPHRASE_SIZE	32
> +
...
> +
> +struct nd_intel_get_security_state {
> +	u32 status;
> +	u32 reserved;
> +	u8 state;
> +	u8 reserved1[3];
> +} __packed;
> +
> +struct nd_intel_set_passphrase {
> +	u8 old_pass[32];
> +	u8 new_pass[32];
> +	u32 status;
> +} __packed;
> +
> +struct nd_intel_unlock_unit {
> +	u8 passphrase[32];
> +	u32 status;
> +} __packed;
> +
> +struct nd_intel_disable_passphrase {
> +	u8 passphrase[32];
> +	u32 status;
> +} __packed;
> +
> +struct nd_intel_freeze_lock {
> +	u32 status;
> +} __packed;
> +
> +struct nd_intel_secure_erase {
> +	u8 passphrase[32];
> +	u32 status;
> +} __packed;
> +
> +struct nd_intel_overwrite {
> +	u8 passphrase[32];
> +	u32 status;
> +} __packed;

All those should use ND_INTEL_PASSPHRASE_SIZE rather than 32,
or there should be a compile-time check that the macro is
<= 32.

In patch 06/12, there is a lot of code that uses
ND_INTEL_PASSPHRASE_SIZE like this:

> +	if (old_data)
> +		memcpy(nd_cmd.cmd.old_pass, old_data->data,
> +				ND_INTEL_PASSPHRASE_SIZE);
> +	memcpy(nd_cmd.cmd.new_pass, new_data->data, ND_INTEL_PASSPHRASE_SIZE);

I think it's safer to use sizeof the destination field:

+	if (old_data)
+		memcpy(nd_cmd.cmd.old_pass, old_data->data,
+				sizeof(nd_cmd.cmd.old_pass));
+	memcpy(nd_cmd.cmd.new_pass, new_data->data,
	       sizeof(nd_cmd.cmd.new_pass));

That helps if the macro gets out of sync with the real size of
the structure.  If it larger, this change prevents an overrun.  If
it is smaller, then memcpy copies some extra bytes, which is
harmless.


---
Robert Elliott, HPE Persistent Memory





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

RE: [PATCH v5 09/12] nfit/libnvdimm: add support for issue secure erase DSM to Intel nvdimm

[Elliott, Robert (Persistent Memory)]

> -----Original Message-----
> From: Linux-nvdimm [mailto:linux-nvdimm-bounces@lists.01.org] On Behalf Of Dave Jiang
> Sent: Tuesday, July 17, 2018 3:55 PM
> Subject: [PATCH v5 09/12] nfit/libnvdimm: add support for issue secure erase DSM to Intel nvdimm
...
 +static int intel_dimm_security_erase(struct nvdimm_bus *nvdimm_bus,
> +		struct nvdimm *nvdimm, struct nvdimm_key_data *nkey)
...
> +	/* DIMM unlocked, invalidate all CPU caches before we read it */
> +	wbinvd_on_all_cpus();

For this function, that comment should use "erased" rather than
"unlocked".

For both this function and intel_dimm_security_unlock() in patch 04/12,
could the driver do a loop of clflushopts on one CPU via
clflush_cache_range() rather than run wbinvd on all CPUs?

---
Robert Elliott, HPE Persistent Memory




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

Re: [PATCH v5 09/12] nfit/libnvdimm: add support for issue secure erase DSM to Intel nvdimm

[Dave Jiang]

On 07/18/2018 10:27 AM, Elliott, Robert (Persistent Memory) wrote:
> 
> 
>> -----Original Message-----
>> From: Linux-nvdimm [mailto:linux-nvdimm-bounces@lists.01.org] On Behalf Of Dave Jiang
>> Sent: Tuesday, July 17, 2018 3:55 PM
>> Subject: [PATCH v5 09/12] nfit/libnvdimm: add support for issue secure erase DSM to Intel nvdimm
> ...
>  +static int intel_dimm_security_erase(struct nvdimm_bus *nvdimm_bus,
>> +struct nvdimm *nvdimm, struct nvdimm_key_data *nkey)
> ...
>> +/* DIMM unlocked, invalidate all CPU caches before we read it */
>> +wbinvd_on_all_cpus();
> 
> For this function, that comment should use "erased" rather than
> "unlocked".
> 
> For both this function and intel_dimm_security_unlock() in patch 04/12,
> could the driver do a loop of clflushopts on one CPU via
> clflush_cache_range() rather than run wbinvd on all CPUs?

The loop should work, but wbinvd is going to be less overall impact to
the performance for really huge ranges. Also, unlock should happen only
once and during NVDIMM initialization. So wbinvd should be ok.

BTW thanks for looking over the patches.

> 
> ---
> Robert Elliott, HPE Persistent Memory
> 
> 
> 
> 
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

Re: [PATCH v5 02/12] libnvdimm: create keyring to store security keys

[Dave Jiang]

On 07/18/2018 03:50 AM, David Howells wrote:
> Dave Jiang <dave.jiang@intel.com> wrote:
> 
>> +static int nvdimm_key_instantiate(struct key *key,
>> +		struct key_preparsed_payload *prep)
> 
> Please use ->preparse() if you can rather than doing this in ->instantiate().
> Ideally ->instantiate() should not return an error, particularly not -EINVAL
> or -ENOMEM.
> 
>> +{
>> +	char *payload;
>> +
>> +	payload = kzalloc(nvdimm_key_type.def_datalen, GFP_KERNEL);
>> +	if (!payload)
>> +		return -ENOMEM;
> 
> Ummm...  Why not just kmemdup()?  Why not use use prep->datalen here rather
> than def_datalen?
> 
>> +
>> +	key->datalen = min(nvdimm_key_type.def_datalen, prep->datalen);
>> +	memcpy(payload, prep->data, key->datalen);
>> +	key->payload.data[0] = payload;
>> +	return 0;
>> +}
> 
> I would recommend, firstly, that you use generic_key_instantiate() if you can;
> if not, you might want to use rcu_assign_keypointer().
> 
>> +	cred = prepare_kernel_cred(NULL);
>> +	if (!cred) {
>> +		rc = -ENOMEM;
>> +		goto failed_cred;
>> +	}
> 
> I wonder if you could just use &init_cred instead.

You mean also instead of a dedicated keyring, just use the existing ones
that comes with init_cred as well?

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

Re: [PATCH v5 06/12] nfit/libnvdimm: add set passphrase support for Intel nvdimms

[Dave Jiang]

On 07/18/2018 09:05 AM, Dave Jiang wrote:
> 
> 
> On 07/18/2018 04:14 AM, David Howells wrote:
>> Dave Jiang <dave.jiang@intel.com> wrote:
>>
>>> Add support for setting and/or updating passphrase on the Intel nvdimms.
>>> The passphrase is pulled from userspace through the kernel key management.
>>> We trigger the update via writing "update" to the sysfs attribute
>>> "security". The state of the security can also be read via the "security"
>>> attribute. libnvdimm will generically support the key_change API call.
>>
>> Btw, could you use a logon-type key rather than spinning your own?  Do you
>> specifically not want it to be updateable?
> 
> Thanks for reviewing this David. I'll take a look and see if I can.
> Basically the code is acting as the proxy for the hardware and passing
> through the passphrase. What would be the best way to update the
> passphrase if I need to retrieve the new passphrase from the userspace?
> The hardware still needs the old passphrase in order to verify. There
> has to be a nice way to do this but I'm not familiar with key management
> API to know better.

A thought occurred to me. For password update, would it make sense to do
this instead:
1. get the existing key by: request_key("nvdimm:xxxxxxxx")
2. get the new key by: request_key("nvdimm.update:xxxxxxxx")
3. verify key with hardware
	on success, copy new payload to existing key payload
4. invalidate "nvdimm.update" key

This way then we won't have to mess with needing the invalidated key to
be garbage collected. Thoughts?


> 
>>
>>> +static int intel_dimm_security_update_passphrase(
>>> +		struct nvdimm_bus *nvdimm_bus, struct nvdimm *nvdimm,
>>> +		struct nvdimm_key_data *old_data,
>>> +		struct nvdimm_key_data *new_data)
>>
>> You might want to mark old_data and new_data as const here.  At least, they
>> don't appear to be changed.  Also, can you stick a banner comment on the
>> function to say what it actually does?  Is it changing the key stored in h/w
>> or is it meant to be changing what's stored in the key payload?
>>
>>> +	/* request new key from userspace */
>>> +	key = nvdimm_request_key(dev);
>>> +	if (!key) {
>>> +		dev_dbg(dev, "%s: failed to acquire new key\n", __func__);
>>> +		return -ENXIO;
>>> +	}
>>> +
>>> +	dev_dbg(dev, "%s: new key: %#x\n", __func__, key->serial);
>>> +
>>> +	if (key->datalen == NVDIMM_PASSPHRASE_LEN) {
>>> +		old_data = NULL;
>>> +		new_data = key->payload.data[0];
>>> +	} else if (key->datalen == (NVDIMM_PASSPHRASE_LEN * 2)) {
>>> +		new_data = key->payload.data[0];
>>> +		old_data = new_data + NVDIMM_PASSPHRASE_LEN;
>>> +	} else {
>>> +		key_invalidate(key);
>>> +		key_put(key);
>>> +		dev_warn(dev, "Incorrect key payload size for update: %u\n",
>>> +				key->datalen);
>>> +		return -EINVAL;
>>> +	}
>>> +
>>> +	down_read(&key->sem);
>>> +	rc = nvdimm->security_ops->change_key(nvdimm_bus, nvdimm, old_data,
>>> +			new_data);
>>
>> Why do you need to get a read lock on key->sem and not a write-lock?
>>
>>> +	if (rc == 0 && key->datalen == (NVDIMM_PASSPHRASE_LEN * 2))
>>> +		memcpy(old_data, new_data, NVDIMM_PASSPHRASE_LEN);
>>
>> You appear to be changing the key content here.  If that's the case, don't you
>> need to write-lock key->sem?
>>
>>> +	up_read(&key->sem);
>>
>> David
>>
> _______________________________________________
> Linux-nvdimm mailing list
> Linux-nvdimm@lists.01.org
> https://lists.01.org/mailman/listinfo/linux-nvdimm
> 
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

Re: [PATCH v5 02/12] libnvdimm: create keyring to store security keys

[David Howells]

Dave Jiang <dave.jiang@intel.com> wrote:

> > I wonder if you could just use &init_cred instead.
> 
> You mean also instead of a dedicated keyring, just use the existing ones
> that comes with init_cred as well?

You don't need a cred to use your own keyring that I can see.  If you look at
nvdimm_search_key(), you're not actually using the cred, but just taking the
keyring out of it:

	keyref = keyring_search(make_key_ref(nvdimm_cred->thread_keyring, 1),
			&nvdimm_key_type, nvdimm->dimm_id);

Further, when you call request_key(), you want to look in the user's keyrings,
I presume, so you can't actually override current->cred with nvdimm_cred.

As far as I can see, you don't use nvdimm_cred, except as somewhere to stash
the keyring pointer.  You could just skip the creds and store the keyring
pointer directly.

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

Re: [PATCH v5 06/12] nfit/libnvdimm: add set passphrase support for Intel nvdimms

[David Howells]

Dave Jiang <dave.jiang@intel.com> wrote:

> A thought occurred to me. For password update, would it make sense to do
> this instead:
> 1. get the existing key by: request_key("nvdimm:xxxxxxxx")
> 2. get the new key by: request_key("nvdimm.update:xxxxxxxx")
> 3. verify key with hardware
> 	on success, copy new payload to existing key payload
> 4. invalidate "nvdimm.update" key
> 
> This way then we won't have to mess with needing the invalidated key to
> be garbage collected. Thoughts?

Can you tell me at what points you actually access the key?

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

Re: [PATCH v5 06/12] nfit/libnvdimm: add set passphrase support for Intel nvdimms

[Dave Jiang]

On 07/18/2018 01:41 PM, David Howells wrote:
> Dave Jiang <dave.jiang@intel.com> wrote:
> 
>> A thought occurred to me. For password update, would it make sense to do
>> this instead:
>> 1. get the existing key by: request_key("nvdimm:xxxxxxxx")
>> 2. get the new key by: request_key("nvdimm.update:xxxxxxxx")
>> 3. verify key with hardware
>> 	on success, copy new payload to existing key payload
>> 4. invalidate "nvdimm.update" key
>>
>> This way then we won't have to mess with needing the invalidated key to
>> be garbage collected. Thoughts?
> 
> Can you tell me at what points you actually access the key?

When we unlock the DIMM, disable security, update/enable passphrase, and
secure erase.

Unlock is called by the kernel during initialization of NVDIMMs.
The rest are triggered through a knob in sysfs. i.e. echo "erase" >
/sys/devices/..../nmem0/security
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

Re: [PATCH v5 06/12] nfit/libnvdimm: add set passphrase support for Intel nvdimms

[Dave Jiang]

On 07/18/2018 04:14 AM, David Howells wrote:
> Dave Jiang <dave.jiang@intel.com> wrote:
> 
>> Add support for setting and/or updating passphrase on the Intel nvdimms.
>> The passphrase is pulled from userspace through the kernel key management.
>> We trigger the update via writing "update" to the sysfs attribute
>> "security". The state of the security can also be read via the "security"
>> attribute. libnvdimm will generically support the key_change API call.
> 
> Btw, could you use a logon-type key rather than spinning your own?  Do you
> specifically not want it to be updateable?

Ok stupid question David. I'm attempting to use the logon-type key. I
have added this line to the request-key.conf:
create   logon   nvdimm*          *      /usr/sbin/nvdimm-upcall %k

But I can't seem to get /sbin/request-key to trigger this line and call
my upcall app.

On the kernel side it seems ok?
==>
search_nested_keyrings({17212177},{logon,nvdimm.update:cdab-0a-07e0-ffffffff})

<== call_sbin_request_key() = -126
<== construct_key() = -126
    cons failed

What am I doing wrong?


> 
>> +static int intel_dimm_security_update_passphrase(
>> +		struct nvdimm_bus *nvdimm_bus, struct nvdimm *nvdimm,
>> +		struct nvdimm_key_data *old_data,
>> +		struct nvdimm_key_data *new_data)
> 
> You might want to mark old_data and new_data as const here.  At least, they
> don't appear to be changed.  Also, can you stick a banner comment on the
> function to say what it actually does?  Is it changing the key stored in h/w
> or is it meant to be changing what's stored in the key payload?
> 
>> +	/* request new key from userspace */
>> +	key = nvdimm_request_key(dev);
>> +	if (!key) {
>> +		dev_dbg(dev, "%s: failed to acquire new key\n", __func__);
>> +		return -ENXIO;
>> +	}
>> +
>> +	dev_dbg(dev, "%s: new key: %#x\n", __func__, key->serial);
>> +
>> +	if (key->datalen == NVDIMM_PASSPHRASE_LEN) {
>> +		old_data = NULL;
>> +		new_data = key->payload.data[0];
>> +	} else if (key->datalen == (NVDIMM_PASSPHRASE_LEN * 2)) {
>> +		new_data = key->payload.data[0];
>> +		old_data = new_data + NVDIMM_PASSPHRASE_LEN;
>> +	} else {
>> +		key_invalidate(key);
>> +		key_put(key);
>> +		dev_warn(dev, "Incorrect key payload size for update: %u\n",
>> +				key->datalen);
>> +		return -EINVAL;
>> +	}
>> +
>> +	down_read(&key->sem);
>> +	rc = nvdimm->security_ops->change_key(nvdimm_bus, nvdimm, old_data,
>> +			new_data);
> 
> Why do you need to get a read lock on key->sem and not a write-lock?
> 
>> +	if (rc == 0 && key->datalen == (NVDIMM_PASSPHRASE_LEN * 2))
>> +		memcpy(old_data, new_data, NVDIMM_PASSPHRASE_LEN);
> 
> You appear to be changing the key content here.  If that's the case, don't you
> need to write-lock key->sem?
> 
>> +	up_read(&key->sem);
> 
> David
> 
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

RE: [PATCH v5 09/12] nfit/libnvdimm: add support for issue secure erase DSM to Intel nvdimm

[Elliott, Robert (Persistent Memory)]

> -----Original Message-----
> From: Dave Jiang <dave.jiang@intel.com>
> Sent: Wednesday, July 18, 2018 12:41 PM
> To: Elliott, Robert (Persistent Memory) <elliott@hpe.com>; Williams,
> Dan J <dan.j.williams@intel.com>
> Cc: dhowells@redhat.com; Schofield, Alison
> <alison.schofield@intel.com>; keyrings@vger.kernel.org;
> keescook@chromium.org; linux-nvdimm@lists.01.org
> Subject: Re: [PATCH v5 09/12] nfit/libnvdimm: add support for issue
> secure erase DSM to Intel nvdimm
> 
> 
> 
> On 07/18/2018 10:27 AM, Elliott, Robert (Persistent Memory) wrote:
> >
> >
> >> -----Original Message-----
> >> From: Linux-nvdimm [mailto:linux-nvdimm-bounces@lists.01.org] On
> Behalf Of Dave Jiang
> >> Sent: Tuesday, July 17, 2018 3:55 PM
> >> Subject: [PATCH v5 09/12] nfit/libnvdimm: add support for issue
> secure erase DSM to Intel nvdimm
> > ...
> >  +static int intel_dimm_security_erase(struct nvdimm_bus
> *nvdimm_bus,
> >> +struct nvdimm *nvdimm, struct nvdimm_key_data *nkey)
> > ...
> >> +/* DIMM unlocked, invalidate all CPU caches before we read it */
> >> +wbinvd_on_all_cpus();
> >
> > For this function, that comment should use "erased" rather than
> > "unlocked".
> >
> > For both this function and intel_dimm_security_unlock() in patch
> 04/12,
> > could the driver do a loop of clflushopts on one CPU via
> > clflush_cache_range() rather than run wbinvd on all CPUs?
> 
> The loop should work, but wbinvd is going to be less overall impact
> to the performance for really huge ranges. Also, unlock should happen
> only once and during NVDIMM initialization. So wbinvd should be ok.

Unlike unlock, secure erase could be requested at any time.

wbinvd must run on every physical core on every physical CPU, while
clflushopt flushes everything from just one CPU core.

wbinvd adds huge interrupt latencies, generating complaints like these:
	https://patchwork.kernel.org/patch/37090/
	https://lists.xenproject.org/archives/html/xen-devel/2011-09/msg00675.html

Also, there's no need to disrupt cache content for other addresses;
only the data at the addresses just erased or unlocked is a concern.
clflushopt avoids disrupting other threads.

Related topic: a flush is also necessary before sending the secure erase or
unlock command.  Otherwise, there could be dirty write data that gets
written by the concluding flush (overwriting the now-unlocked or just-erased
data).  For unlock during boot, you might assume that no writes having
occurred yet, but that isn't true for secure erase on demand.  Flushing
before both commands is safest.

---
Robert Elliott, HPE Persistent Memory


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

RE: [PATCH v5 09/12] nfit/libnvdimm: add support for issue secure erase DSM to Intel nvdimm

[Li, Juston]

> -----Original Message-----
> From: Linux-nvdimm [mailto:linux-nvdimm-bounces@lists.01.org] On Behalf Of
> Elliott, Robert (Persistent Memory)
> Sent: Wednesday, July 18, 2018 6:43 PM
> To: Jiang, Dave <dave.jiang@intel.com>; Williams, Dan J
> <dan.j.williams@intel.com>
> Cc: dhowells@redhat.com; Schofield, Alison <alison.schofield@intel.com>;
> keyrings@vger.kernel.org; keescook@chromium.org; linux-
> nvdimm@lists.01.org
> Subject: RE: [PATCH v5 09/12] nfit/libnvdimm: add support for issue secure erase
> DSM to Intel nvdimm
> 
>
> Related topic: a flush is also necessary before sending the secure erase or unlock
> command.  Otherwise, there could be dirty write data that gets written by the
> concluding flush (overwriting the now-unlocked or just-erased data).  For unlock
> during boot, you might assume that no writes having occurred yet, but that isn't
> true for secure erase on demand.  Flushing before both commands is safest.
> 

I was wondering this too.

Is it handled by the fact the DIMM must be disabled to do a secure erase?
I'm assuming that means the namespace that the DIMM is a part of also must be
disabled first? Then no further writes can occur and provided that dirty write data is
flushed when the namespace is disabled, it should be safe to issue a secure erase.

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

Re: [PATCH v5 06/12] nfit/libnvdimm: add set passphrase support for Intel nvdimms

[David Howells]

Dave Jiang <dave.jiang@intel.com> wrote:

> Ok stupid question David. I'm attempting to use the logon-type key. I
> have added this line to the request-key.conf:
> create   logon   nvdimm*          *      /usr/sbin/nvdimm-upcall %k

Can you show me the whole file?

Let me ask a stupid question too:  Why do you need to call request_key()?

As I understand it, you poke an attribute file in sysfs by writing "update" to
it and this triggers a request_key() call.  The kernel then links the key it
found across to the internal keyring.

You could instead require that the key be specified directly, ie. you write
"update <keyid>" to the attribute file.  The driver can then call key_lookup()
to get the key - or, better still, we should make lookup_user_key() available
so that you can call that - which will do a security check.

Another advantage of doing this is that the old key is still available in the
internal keyring until it gets replaced.  So you can do your password change
if you want to do it this way.

On the other hand, requiring both the old and the new passwords to be supplied
is probably better from a security point of view, so you could require them
both to be included in the key.

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

Re: [PATCH v5 09/12] nfit/libnvdimm: add support for issue secure erase DSM to Intel nvdimm

[Dave Jiang]

On 07/18/2018 06:43 PM, Elliott, Robert (Persistent Memory) wrote:
> 
> 
>> -----Original Message-----
>> From: Dave Jiang <dave.jiang@intel.com>
>> Sent: Wednesday, July 18, 2018 12:41 PM
>> To: Elliott, Robert (Persistent Memory) <elliott@hpe.com>; Williams,
>> Dan J <dan.j.williams@intel.com>
>> Cc: dhowells@redhat.com; Schofield, Alison
>> <alison.schofield@intel.com>; keyrings@vger.kernel.org;
>> keescook@chromium.org; linux-nvdimm@lists.01.org
>> Subject: Re: [PATCH v5 09/12] nfit/libnvdimm: add support for issue
>> secure erase DSM to Intel nvdimm
>>
>>
>>
>> On 07/18/2018 10:27 AM, Elliott, Robert (Persistent Memory) wrote:
>>>
>>>
>>>> -----Original Message-----
>>>> From: Linux-nvdimm [mailto:linux-nvdimm-bounces@lists.01.org] On
>> Behalf Of Dave Jiang
>>>> Sent: Tuesday, July 17, 2018 3:55 PM
>>>> Subject: [PATCH v5 09/12] nfit/libnvdimm: add support for issue
>> secure erase DSM to Intel nvdimm
>>> ...
>>>  +static int intel_dimm_security_erase(struct nvdimm_bus
>> *nvdimm_bus,
>>>> +struct nvdimm *nvdimm, struct nvdimm_key_data *nkey)
>>> ...
>>>> +/* DIMM unlocked, invalidate all CPU caches before we read it */
>>>> +wbinvd_on_all_cpus();
>>>
>>> For this function, that comment should use "erased" rather than
>>> "unlocked".
>>>
>>> For both this function and intel_dimm_security_unlock() in patch
>> 04/12,
>>> could the driver do a loop of clflushopts on one CPU via
>>> clflush_cache_range() rather than run wbinvd on all CPUs?
>>
>> The loop should work, but wbinvd is going to be less overall impact
>> to the performance for really huge ranges. Also, unlock should happen
>> only once and during NVDIMM initialization. So wbinvd should be ok.
> 
> Unlike unlock, secure erase could be requested at any time.
> 
> wbinvd must run on every physical core on every physical CPU, while
> clflushopt flushes everything from just one CPU core.
> 
> wbinvd adds huge interrupt latencies, generating complaints like these:
> https://patchwork.kernel.org/patch/37090/
> https://lists.xenproject.org/archives/html/xen-devel/2011-09/msg00675.html
> 
> Also, there's no need to disrupt cache content for other addresses;
> only the data at the addresses just erased or unlocked is a concern.
> clflushopt avoids disrupting other threads.

Yes secure erase could be requested at any time, but the likelihood of
that happening frequently is unlikely. Also, in order to do secure
erase, one must disable regions impacted by the dimm and also the dimm
itself. More likely than not, the admin is doing maintenance and not
expecting running workloads (at least not on the pmem). The concern is
more that the admin wants to finish the task quickly rather than if
there's performance impact while the maintenance task is going on.

Also, looping over potentially TB-sized ranges with CLFLUSHOPT may take
a while (many minutes?)? Yes it just flushes cache from one CPU, but
also it causes cross-CPU traffic to maintain coherency, and KTI traffic
and/or reads from the media to check directory bits. WBINVD is pretty
heavy handed but it's the only option we have that doesn't have to plow
through each cache line in the huge range.

> 
> Related topic: a flush is also necessary before sending the secure erase or
> unlock command.  Otherwise, there could be dirty write data that gets
> written by the concluding flush (overwriting the now-unlocked or just-erased
> data).  For unlock during boot, you might assume that no writes having
> occurred yet, but that isn't true for secure erase on demand.  Flushing
> before both commands is safest.

Yes I missed that. Thanks for catching. I'll add the flush before
executing secure erase. It's probably not necessary for unlock since
there's no data that would be in the CPU cache until the DIMMs are
accessible.
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

Re: [PATCH v5 06/12] nfit/libnvdimm: add set passphrase support for Intel nvdimms

[Dave Jiang]

On 07/19/2018 01:22 AM, David Howells wrote:
> Dave Jiang <dave.jiang@intel.com> wrote:
> 
>> Ok stupid question David. I'm attempting to use the logon-type key. I
>> have added this line to the request-key.conf:
>> create   logon   nvdimm*          *      /usr/sbin/nvdimm-upcall %k
> 
> Can you show me the whole file?
https://lists.01.org/pipermail/linux-nvdimm/2018-July/016958.html

I don't believe /sbin/request-key is calling my upcall app.

> 
> Let me ask a stupid question too:  Why do you need to call request_key()?
> 
> As I understand it, you poke an attribute file in sysfs by writing "update" to
> it and this triggers a request_key() call.  The kernel then links the key it
> found across to the internal keyring.

Correct. And when there isn't a key, it needs to fetch from userspace
and construct the key.

> 
> You could instead require that the key be specified directly, ie. you write
> "update <keyid>" to the attribute file.  The driver can then call key_lookup()
> to get the key - or, better still, we should make lookup_user_key() available
> so that you can call that - which will do a security check.

I can attach the keyid to the nvdimm when I initially get a success key
and the update can look it up easily enough. I'm not groking the
lookup_user_key() comment. What determines a key to be a user key or a
kernel key?

I think right now the issue I have is trying to get logon key type
working with creating the initial key from userspace and/or update the
key with new passphrase, which needs a payload from userspace.

> 
> Another advantage of doing this is that the old key is still available in the
> internal keyring until it gets replaced.  So you can do your password change
> if you want to do it this way.
> 
> On the other hand, requiring both the old and the new passwords to be supplied
> is probably better from a security point of view, so you could require them
> both to be included in the key.
> 
> David
> 
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

Re: [PATCH v5 06/12] nfit/libnvdimm: add set passphrase support for Intel nvdimms

[Dave Jiang]

On 07/19/2018 02:28 PM, Dave Jiang wrote:
> 
> 
> On 07/19/2018 01:22 AM, David Howells wrote:
>> Dave Jiang <dave.jiang@intel.com> wrote:
>>
>>> Ok stupid question David. I'm attempting to use the logon-type key. I
>>> have added this line to the request-key.conf:
>>> create   logon   nvdimm*          *      /usr/sbin/nvdimm-upcall %k
>>
>> Can you show me the whole file?
> https://lists.01.org/pipermail/linux-nvdimm/2018-July/016958.html
> 
> I don't believe /sbin/request-key is calling my upcall app.

Ok I got this part working. I didn't realize that request-key looks for
<key type>.conf and I needed to add my line to /etc/request-key.conf and
/etc/request-key.d/nvdimm.conf no longer works.

> 
>>
>> Let me ask a stupid question too:  Why do you need to call request_key()?
>>
>> As I understand it, you poke an attribute file in sysfs by writing "update" to
>> it and this triggers a request_key() call.  The kernel then links the key it
>> found across to the internal keyring.
> 
> Correct. And when there isn't a key, it needs to fetch from userspace
> and construct the key.
> 
>>
>> You could instead require that the key be specified directly, ie. you write
>> "update <keyid>" to the attribute file.  The driver can then call key_lookup()
>> to get the key - or, better still, we should make lookup_user_key() available
>> so that you can call that - which will do a security check.
> 
> I can attach the keyid to the nvdimm when I initially get a success key
> and the update can look it up easily enough. I'm not groking the
> lookup_user_key() comment. What determines a key to be a user key or a
> kernel key?
> 
> I think right now the issue I have is trying to get logon key type
> working with creating the initial key from userspace and/or update the
> key with new passphrase, which needs a payload from userspace.
> 
>>
>> Another advantage of doing this is that the old key is still available in the
>> internal keyring until it gets replaced.  So you can do your password change
>> if you want to do it this way.
>>
>> On the other hand, requiring both the old and the new passwords to be supplied
>> is probably better from a security point of view, so you could require them
>> both to be included in the key.
>>
>> David
>>
> _______________________________________________
> Linux-nvdimm mailing list
> Linux-nvdimm@lists.01.org
> https://lists.01.org/mailman/listinfo/linux-nvdimm
> 
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

Re: [PATCH v5 06/12] nfit/libnvdimm: add set passphrase support for Intel nvdimms

[David Howells]

Dave Jiang <dave.jiang@intel.com> wrote:

> > Can you show me the whole file?
> https://lists.01.org/pipermail/linux-nvdimm/2018-July/016958.html

Is that exactly what's in your /etc/request-key.conf?

> > As I understand it, you poke an attribute file in sysfs by writing
> > "update" to it and this triggers a request_key() call.  The kernel then
> > links the key it found across to the internal keyring.
> 
> Correct. And when there isn't a key, it needs to fetch from userspace
> and construct the key.

I think I've missed a step.  Am I right in thinking that the "update" command
is just for changing the password, and is not the primary way that passwords
get into the kernel to, say, unlock the device?

> > You could instead require that the key be specified directly, ie. you
> > write "update <keyid>" to the attribute file.  The driver can then call
> > key_lookup() to get the key - or, better still, we should make
> > lookup_user_key() available so that you can call that - which will do a
> > security check.
> 
> I can attach the keyid to the nvdimm when I initially get a success key
> and the update can look it up easily enough. I'm not groking the
> lookup_user_key() comment. What determines a key to be a user key or a
> kernel key?

lookup_user_key() converts a key serial number into a key applies all the
security checks to make sure that userspace is allowed to use that key for
some purpose.

key_lookup(), on the other hand, has no security checks whatsoever.

So if you're using a key ID given to you by userspace, you have to use
lookup_user_key().  Whereas if you want to find a key by description - and
potentially create it - you use request_key().  request_key() also does all
the appropriate security checks.

So I would be tempted to make the update command take an explicit key ID
rather than calling request_key() - particularly as you want to control what
the password gets changed to.  Possibly even make it take *two* explicit key
IDs: the key holding the new password and the key holding the old one.  Then
you can have security checks on both and you can compare the descriptions to
make sure they're the same.

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

Re: [PATCH v5 06/12] nfit/libnvdimm: add set passphrase support for Intel nvdimms

[Dave Jiang]

On 07/20/2018 08:40 AM, David Howells wrote:
> Dave Jiang <dave.jiang@intel.com> wrote:
> 
>>> Can you show me the whole file?
>> https://lists.01.org/pipermail/linux-nvdimm/2018-July/016958.html
> 
> Is that exactly what's in your /etc/request-key.conf?
No, I realized that I had to do update when converted to logon key. This
is what I added to /etc/request-key.conf:
create   logon   nvdimm*          *      /usr/sbin/nvdimm-upcall %k

I do have it working now.

> 
>>> As I understand it, you poke an attribute file in sysfs by writing
>>> "update" to it and this triggers a request_key() call.  The kernel then
>>> links the key it found across to the internal keyring.
>>
>> Correct. And when there isn't a key, it needs to fetch from userspace
>> and construct the key.
> 
> I think I've missed a step.  Am I right in thinking that the "update" command
> is just for changing the password, and is not the primary way that passwords
> get into the kernel to, say, unlock the device?

That is correct. There is an unlock operation. But we do not expose that
to the userspace via the sysfs knob. Unlock only happens during NVDIMM
discovery/init. Mainly the security is to prevent someone taking the
NVDIMM to another server or take it out of data center. The update
command either enables security and add a passphrase to the NVDIMM or
change the passphrase on a NVDIMM.

> 
>>> You could instead require that the key be specified directly, ie. you
>>> write "update <keyid>" to the attribute file.  The driver can then call
>>> key_lookup() to get the key - or, better still, we should make
>>> lookup_user_key() available so that you can call that - which will do a
>>> security check.
>>
>> I can attach the keyid to the nvdimm when I initially get a success key
>> and the update can look it up easily enough. I'm not groking the
>> lookup_user_key() comment. What determines a key to be a user key or a
>> kernel key?
> 
> lookup_user_key() converts a key serial number into a key applies all the
> security checks to make sure that userspace is allowed to use that key for
> some purpose.
> 
> key_lookup(), on the other hand, has no security checks whatsoever.
> 
> So if you're using a key ID given to you by userspace, you have to use
> lookup_user_key().  Whereas if you want to find a key by description - and
> potentially create it - you use request_key().  request_key() also does all
> the appropriate security checks.
> 
> So I would be tempted to make the update command take an explicit key ID
> rather than calling request_key() - particularly as you want to control what
> the password gets changed to.  Possibly even make it take *two* explicit key
> IDs: the key holding the new password and the key holding the old one.  Then
> you can have security checks on both and you can compare the descriptions to
> make sure they're the same.

Ok let me paraphrase what you are saying and see if I understand.
The current implementation for change password:
1. trigger update via sysfs
2. kernel looks for key in keyring
3. kernel fetches key from userspace if no key in kernel keyring
4. kernel adds new key to keyring

New:
1. userspace inject key into kernel keyring
2. userspace triggers update with new key id and old key id
3. kernel looks up keys with key-id in kernel
4. kernel tries to update with hardware
5. on success kernel accepts new key (or update old key payload)

This pushes most of the complexity into userspace and is more secure.
But I'm not sure if we need it. The admin with root access has access to
everything already. We are just trying to protect against someone moving
the DIMM w/o the right authority. Also, unlock right now basically is a
request-key to userspace to retrieve the key before we unlock. If we go
the other way it sounds like we need to inject all the keys from
userspace first before unlock can happen? Doesn't this mean we'll end up
having to call request-key anyhow in order to trigger an upcall app to
do all this? Also we can't wait until userland comes up and execute an
app to inject all the user keys before we initialize the DIMMs if we
aren't going through request-key upcall.

Also, this is step one. Eventually we would like to be able to just
retrieve the passphrase from TPM instead of the fs or network.

I do think associating with the current key id with the nvdimm object
can make things a lot easier and I can just do key_lookup() instead of
calling request_key().

What about:
1. user triggers update through sysfs
2. kernel fetches old key with key-id that's associated with the nvdimm
object from keyring
3. kernel request_key from userspace for new key
4. kernel sends both payloads to hardware to attempt update
5. on success kernel invalidates old key and updates key-id associated
with the nvdimm object

In this case, all the other security ops should be able to lookup the
key via key-id if that exists. Or they would request-key if the key does
not exist (if the op is first to execute).
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

Re: [PATCH v5 06/12] nfit/libnvdimm: add set passphrase support for Intel nvdimms

[David Howells]

Dave Jiang <dave.jiang@intel.com> wrote:

> New:
> 1. userspace inject key into kernel keyring

I presume this step is what you do to use the nvdimm?  If so, this can use
request_key() and then splice the key across to the kernel keyring.

> 2. userspace triggers update with new key id and old key id
> 3. kernel looks up keys with key-id in kernel
> 4. kernel tries to update with hardware
> 5. on success kernel accepts new key (or update old key payload)

     ... and splices the key into the kernel keyring.

> Also, unlock right now basically is a request-key to userspace to retrieve
> the key before we unlock. If we go the other way it sounds like we need to
> inject all the keys from userspace first before unlock can happen?

No, you'd still use request_key() for that.

It's the change-password event that I'm suggesting you should use two
explicitly nominated keys for.

> I do think associating with the current key id with the nvdimm object
> can make things a lot easier and I can just do key_lookup() instead of
> calling request_key().

You shouldn't be using key_lookup().

> What about:
> 1. user triggers update through sysfs
> 2. kernel fetches old key with key-id that's associated with the nvdimm
> object from keyring

You can do that.  You'd use key_search() for that.

> 3. kernel request_key from userspace for new key

This bit I quibble with.  I think you should specify it to make sure that you
get the password you intended and don't pick something else up by accident.

> 4. kernel sends both payloads to hardware to attempt update
> 5. on success kernel invalidates old key and updates key-id associated
> with the nvdimm object

I think you should just link the new key across.  It should have the same
description as the old key to the kernel keyring.  This would displace the
old key automatically.

> ... lookup the key via key-id ...

You shouldn't use key IDs within the kernel, except at the UAPI boundary.  You
should be using the key description as the handle - and, in this case, it
should be an identifier you can match uniquely within the system with the
particular nvdimm.

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