diff mbox series

[v4,1/2] libnvdimm/security: Support a zero-key for secure-erase

Message ID 155361997046.42665.15819732417231761031.stgit@djiang5-desk3.ch.intel.com (mailing list archive)
State New, archived
Headers show
Series [v4,1/2] libnvdimm/security: Support a zero-key for secure-erase | expand

Commit Message

Dave Jiang March 26, 2019, 5:06 p.m. UTC 
Adding support to allow secure erase to happen when security state is not
enabled. Key data of 0's will be passed in.

Some other security commands already use zero keys. This is to unifiy
semantics cross commands with respect to using zero keys.

Signed-off-by: Dave Jiang <dave.jiang@intel.com>
---
v4: No change

v3:
- Add note in commit header about syncing zero key usage. (Dan)

v2:
- Make patch header explicitly zero key (Dan)
- Declare global static zero key (Dan)
- Make nfit_test explicitly test zero key (Dan)

 drivers/nvdimm/security.c        |   17 ++++++++++++-----
 tools/testing/nvdimm/test/nfit.c |   11 +++++++++--
 2 files changed, 21 insertions(+), 7 deletions(-)

Comments

Dan Williams March 26, 2019, 8:28 p.m. UTC | #1 
On Tue, Mar 26, 2019 at 10:06 AM Dave Jiang <dave.jiang@intel.com> wrote:
>
> Adding support to allow secure erase to happen when security state is not
> enabled. Key data of 0's will be passed in.

That still sounds more like a feature than a fix, and if I'm looking
closer than the fix is to make sure that all nvdimm_lookup_user_key()
invocations always return data, at least for all the "current / old"
passphrase cases. Because, afaics there's no way to unlock a DIMM if a
platform implements a zero-key default.

Also the current "freeze on no key" functionality will fail if the
DIMM expects the zero-key.

nvdimm_lookup_user_key(good_key_id, current_key) => user key payload

nvdimm_lookup_user_key(bad_key_id) => zero key payload, but should
fail if this was the key-id for the new-passphrase / key

nvdimm_lookup_user_key(0) => zero key payload.

This guarantees that zero-key semantic is uniform across all current /
old-passphrase use cases.

> Some other security commands already use zero keys. This is to unifiy
> semantics cross commands with respect to using zero keys.

We should say a bit why this is important and the knowledge that there
are some platforms that default to a zero-key. This is the primary
justification for the change. For a -rc change of this nature the
status as a fix needs to be clear otherwise it will be deferred to
v5.2. For example if I were Linus I would reject any patch that starts
"Adding support" and not read the rest of the changelog at this stage
of the development cycle.
diff mbox series

Patch

diff --git a/drivers/nvdimm/security.c b/drivers/nvdimm/security.c
index f8bb746a549f..6bea6852bf27 100644
--- a/drivers/nvdimm/security.c
+++ b/drivers/nvdimm/security.c
@@ -22,6 +22,8 @@  static bool key_revalidate = true;
 module_param(key_revalidate, bool, 0444);
 MODULE_PARM_DESC(key_revalidate, "Require key validation at init.");
 
+static const char zero_key[NVDIMM_PASSPHRASE_LEN];
+
 static void *key_data(struct key *key)
 {
 	struct encrypted_key_payload *epayload = dereference_key_locked(key);
@@ -286,8 +288,9 @@  int nvdimm_security_erase(struct nvdimm *nvdimm, unsigned int keyid,
 {
 	struct device *dev = &nvdimm->dev;
 	struct nvdimm_bus *nvdimm_bus = walk_to_nvdimm_bus(dev);
-	struct key *key;
+	struct key *key = NULL;
 	int rc;
+	const void *data;
 
 	/* The bus lock should be held at the top level of the call stack */
 	lockdep_assert_held(&nvdimm_bus->reconfig_mutex);
@@ -319,11 +322,15 @@  int nvdimm_security_erase(struct nvdimm *nvdimm, unsigned int keyid,
 		return -EOPNOTSUPP;
 	}
 
-	key = nvdimm_lookup_user_key(nvdimm, keyid, NVDIMM_BASE_KEY);
-	if (!key)
-		return -ENOKEY;
+	if (keyid != 0) {
+		key = nvdimm_lookup_user_key(nvdimm, keyid, NVDIMM_BASE_KEY);
+		if (!key)
+			return -ENOKEY;
+		data = key_data(key);
+	} else
+		data = zero_key;
 
-	rc = nvdimm->sec.ops->erase(nvdimm, key_data(key), pass_type);
+	rc = nvdimm->sec.ops->erase(nvdimm, data, pass_type);
 	dev_dbg(dev, "key: %d erase%s: %s\n", key_serial(key),
 			pass_type == NVDIMM_MASTER ? "(master)" : "(user)",
 			rc == 0 ? "success" : "fail");
diff --git a/tools/testing/nvdimm/test/nfit.c b/tools/testing/nvdimm/test/nfit.c
index b579f962451d..cad719876ef4 100644
--- a/tools/testing/nvdimm/test/nfit.c
+++ b/tools/testing/nvdimm/test/nfit.c
@@ -225,6 +225,8 @@  static struct workqueue_struct *nfit_wq;
 
 static struct gen_pool *nfit_pool;
 
+static const char zero_key[NVDIMM_PASSPHRASE_LEN];
+
 static struct nfit_test *to_nfit_test(struct device *dev)
 {
 	struct platform_device *pdev = to_platform_device(dev);
@@ -1059,8 +1061,7 @@  static int nd_intel_test_cmd_secure_erase(struct nfit_test *t,
 	struct device *dev = &t->pdev.dev;
 	struct nfit_test_sec *sec = &dimm_sec_info[dimm];
 
-	if (!(sec->state & ND_INTEL_SEC_STATE_ENABLED) ||
-			(sec->state & ND_INTEL_SEC_STATE_FROZEN)) {
+	if (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,
@@ -1068,6 +1069,12 @@  static int nd_intel_test_cmd_secure_erase(struct nfit_test *t,
 		nd_cmd->status = ND_INTEL_STATUS_INVALID_PASS;
 		dev_dbg(dev, "secure erase: wrong passphrase\n");
 	} else {
+		if (!(sec->state & ND_INTEL_SEC_STATE_ENABLED)
+				&& (memcmp(nd_cmd->passphrase, zero_key,
+					ND_INTEL_PASSPHRASE_SIZE) != 0)) {
+			dev_dbg(dev, "invalid zero key\n");
+			return 0;
+		}
 		memset(sec->passphrase, 0, ND_INTEL_PASSPHRASE_SIZE);
 		memset(sec->master_passphrase, 0, ND_INTEL_PASSPHRASE_SIZE);
 		sec->state = 0;