[ndctl] ndctl/dimm: Add support for separate security-frozen attribute
diff mbox series

Message ID 156583219134.2816070.2537582454969393648.stgit@dwillia2-desk3.amr.corp.intel.com
State New
Headers show
Series
  • [ndctl] ndctl/dimm: Add support for separate security-frozen attribute
Related show

Commit Message

Dan Williams Aug. 15, 2019, 1:23 a.m. UTC
Given the discovery that the original libnvdimm-security implementation
is unable to communicate both the 'freeze' status and the 'lock' status
simultaneously, newer kernels deploy a new 'frozen' attribute for this
purpose.

Add a new api and update the tests for this new capability. The old test
will fail on newer kernels, but hopefully there were no other
applications depending on the 'security' attribute to communicate the
'freeze' status. It was likely only ever a debug / enumeration aid, not
an application dependency.

Cc: Dave Jiang <dave.jiang@intel.com>
Cc: Vishal Verma <vishal.l.verma@intel.com>
Reported-by: Jeff Moyer <jmoyer@redhat.com>
Signed-off-by: Dan Williams <dan.j.williams@intel.com>
---
 Documentation/ndctl/ndctl-freeze-security.txt |    8 +++++---
 ndctl/dimm.c                                  |    2 +-
 ndctl/lib/dimm.c                              |   25 +++++++++++++++++++++++++
 ndctl/lib/libndctl.sym                        |    4 ++++
 ndctl/libndctl.h                              |    1 +
 test/security.sh                              |   18 ++++++++++++------
 util/json.c                                   |    6 ++++++
 7 files changed, 54 insertions(+), 10 deletions(-)

Comments

Verma, Vishal L Aug. 19, 2019, 8:26 p.m. UTC | #1
On Wed, 2019-08-14 at 18:23 -0700, Dan Williams wrote:
> Given the discovery that the original libnvdimm-security implementation
> is unable to communicate both the 'freeze' status and the 'lock' status
> simultaneously, newer kernels deploy a new 'frozen' attribute for this
> purpose.
> 
> Add a new api and update the tests for this new capability. The old test
> will fail on newer kernels, but hopefully there were no other
> applications depending on the 'security' attribute to communicate the
> 'freeze' status. It was likely only ever a debug / enumeration aid, not
> an application dependency.
> 
> Cc: Dave Jiang <dave.jiang@intel.com>
> Cc: Vishal Verma <vishal.l.verma@intel.com>
> Reported-by: Jeff Moyer <jmoyer@redhat.com>
> Signed-off-by: Dan Williams <dan.j.williams@intel.com>
> ---
>  Documentation/ndctl/ndctl-freeze-security.txt |    8 +++++---
>  ndctl/dimm.c                                  |    2 +-
>  ndctl/lib/dimm.c                              |   25 +++++++++++++++++++++++++
>  ndctl/lib/libndctl.sym                        |    4 ++++
>  ndctl/libndctl.h                              |    1 +
>  test/security.sh                              |   18 ++++++++++++------
>  util/json.c                                   |    6 ++++++
>  7 files changed, 54 insertions(+), 10 deletions(-)
> 
[..]

> @@ -262,6 +267,7 @@ test_4_security_unlock
>  # not impact any key management testing via libkeyctl.
>  echo "Test 5, freeze security"
>  test_5_security_freeze
> +exit 1

Is this a leftover from local development/testing?
Otherwise the patch looks good to me.
Dan Williams Aug. 19, 2019, 8:47 p.m. UTC | #2
On Mon, Aug 19, 2019 at 1:26 PM Verma, Vishal L
<vishal.l.verma@intel.com> wrote:
>
> On Wed, 2019-08-14 at 18:23 -0700, Dan Williams wrote:
> > Given the discovery that the original libnvdimm-security implementation
> > is unable to communicate both the 'freeze' status and the 'lock' status
> > simultaneously, newer kernels deploy a new 'frozen' attribute for this
> > purpose.
> >
> > Add a new api and update the tests for this new capability. The old test
> > will fail on newer kernels, but hopefully there were no other
> > applications depending on the 'security' attribute to communicate the
> > 'freeze' status. It was likely only ever a debug / enumeration aid, not
> > an application dependency.
> >
> > Cc: Dave Jiang <dave.jiang@intel.com>
> > Cc: Vishal Verma <vishal.l.verma@intel.com>
> > Reported-by: Jeff Moyer <jmoyer@redhat.com>
> > Signed-off-by: Dan Williams <dan.j.williams@intel.com>
> > ---
> >  Documentation/ndctl/ndctl-freeze-security.txt |    8 +++++---
> >  ndctl/dimm.c                                  |    2 +-
> >  ndctl/lib/dimm.c                              |   25 +++++++++++++++++++++++++
> >  ndctl/lib/libndctl.sym                        |    4 ++++
> >  ndctl/libndctl.h                              |    1 +
> >  test/security.sh                              |   18 ++++++++++++------
> >  util/json.c                                   |    6 ++++++
> >  7 files changed, 54 insertions(+), 10 deletions(-)
> >
> [..]
>
> > @@ -262,6 +267,7 @@ test_4_security_unlock
> >  # not impact any key management testing via libkeyctl.
> >  echo "Test 5, freeze security"
> >  test_5_security_freeze
> > +exit 1
>
> Is this a leftover from local development/testing?

Yes, whoops. Just trying to keep you on your toes.

> Otherwise the patch looks good to me.

Thanks! Will resend.

Patch
diff mbox series

diff --git a/Documentation/ndctl/ndctl-freeze-security.txt b/Documentation/ndctl/ndctl-freeze-security.txt
index 573577194183..dbb94e7989af 100644
--- a/Documentation/ndctl/ndctl-freeze-security.txt
+++ b/Documentation/ndctl/ndctl-freeze-security.txt
@@ -35,7 +35,7 @@  $ ndctl list -d nmem0
 ]
 
 $ ndctl freeze-security  nmem0
-security freezed 1 nmem.
+security froze 1 nmem.
 
 $ ndctl list -d nmem0
 [
@@ -44,9 +44,11 @@  $ ndctl list -d nmem0
     "id":"cdab-0a-07e0-ffffffff",
     "handle":0,
     "phys_id":0,
-    "security":"frozen"
-  }
+    "security":"unlocked",
+    "security_frozen":true
+  },
 ]
+
 ----
 
 OPTIONS
diff --git a/ndctl/dimm.c b/ndctl/dimm.c
index 5e6fa19bab15..c8821d6110e8 100644
--- a/ndctl/dimm.c
+++ b/ndctl/dimm.c
@@ -1426,7 +1426,7 @@  int cmd_freeze_security(int argc, const char **argv, void *ctx)
 	int count = dimm_action(argc, argv, ctx, action_security_freeze, base_options,
 			"ndctl freeze-security <nmem0> [<nmem1>..<nmemN>] [<options>]");
 
-	fprintf(stderr, "security freezed %d nmem%s.\n", count >= 0 ? count : 0,
+	fprintf(stderr, "security froze %d nmem%s.\n", count >= 0 ? count : 0,
 			count > 1 ? "s" : "");
 	return count >= 0 ? 0 : EXIT_FAILURE;
 }
diff --git a/ndctl/lib/dimm.c b/ndctl/lib/dimm.c
index 37db5570102a..2f145be520fd 100644
--- a/ndctl/lib/dimm.c
+++ b/ndctl/lib/dimm.c
@@ -704,6 +704,31 @@  NDCTL_EXPORT enum ndctl_security_state ndctl_dimm_get_security(
 	return NDCTL_SECURITY_INVALID;
 }
 
+NDCTL_EXPORT bool ndctl_dimm_security_is_frozen(struct ndctl_dimm *dimm)
+{
+	struct ndctl_ctx *ctx = ndctl_dimm_get_ctx(dimm);
+	char *path = dimm->dimm_buf;
+	char buf[SYSFS_ATTR_SIZE];
+	int len = dimm->buf_len;
+	int rc;
+
+
+	if (ndctl_dimm_get_security(dimm) == NDCTL_SECURITY_FROZEN)
+		return true;
+
+	if (snprintf(path, len, "%s/frozen", dimm->dimm_path) >= len) {
+		err(ctx, "%s: buffer too small!\n",
+				ndctl_dimm_get_devname(dimm));
+		return false;
+	}
+
+	rc = sysfs_read_attr(ctx, path, buf);
+	if (rc < 0)
+		return false;
+
+	return !!strtoul(buf, NULL, 0);
+}
+
 static int write_security(struct ndctl_dimm *dimm, const char *cmd)
 {
 	struct ndctl_ctx *ctx = ndctl_dimm_get_ctx(dimm);
diff --git a/ndctl/lib/libndctl.sym b/ndctl/lib/libndctl.sym
index fef2907aa47d..c93c1ee7274c 100644
--- a/ndctl/lib/libndctl.sym
+++ b/ndctl/lib/libndctl.sym
@@ -419,3 +419,7 @@  LIBNDCTL_21 {
 	ndctl_dimm_read_label_extent;
 	ndctl_dimm_zero_label_extent;
 } LIBNDCTL_20;
+
+LIBNDCTL_22 {
+	ndctl_dimm_security_is_frozen;
+} LIBNDCTL_21;
diff --git a/ndctl/libndctl.h b/ndctl/libndctl.h
index f3f2ef66c5a8..d720a98ead1e 100644
--- a/ndctl/libndctl.h
+++ b/ndctl/libndctl.h
@@ -714,6 +714,7 @@  enum ndctl_security_state {
 };
 
 enum ndctl_security_state ndctl_dimm_get_security(struct ndctl_dimm *dimm);
+bool ndctl_dimm_security_is_frozen(struct ndctl_dimm *dimm);
 int ndctl_dimm_update_passphrase(struct ndctl_dimm *dimm,
 		long ckey, long nkey);
 int ndctl_dimm_disable_passphrase(struct ndctl_dimm *dimm, long key);
diff --git a/test/security.sh b/test/security.sh
index c86d2c6591a6..942831c901fa 100755
--- a/test/security.sh
+++ b/test/security.sh
@@ -105,6 +105,11 @@  lock_dimm()
 	fi
 }
 
+get_frozen_state()
+{
+	$NDCTL list -i -b "$NFIT_TEST_BUS0" -d "$dev" | jq -r .[].dimms[0].security_frozen
+}
+
 get_security_state()
 {
 	$NDCTL list -i -b "$NFIT_TEST_BUS0" -d "$dev" | jq -r .[].dimms[0].security
@@ -195,15 +200,15 @@  test_5_security_freeze()
 	setup_passphrase
 	freeze_security
 	sstate="$(get_security_state)"
-	if [ "$sstate" != "frozen" ]; then
-		echo "Incorrect security state: $sstate expected: frozen"
+	fstate="$(get_frozen_state)"
+	if [ "$fstate" != "true" ]; then
+		echo "Incorrect security state: expected: frozen"
 		err "$LINENO"
 	fi
 	$NDCTL remove-passphrase "$dev" && { echo "remove succeed after frozen"; }
-	sstate="$(get_security_state)"
-	echo "$sstate"
-	if [ "$sstate" != "frozen" ]; then
-		echo "Incorrect security state: $sstate expected: frozen"
+	sstate2="$(get_security_state)"
+	if [ "$sstate" != "$sstate2" ]; then
+		echo "Incorrect security state: $sstate2 expected: $sstate"
 		err "$LINENO"
 	fi
 }
@@ -262,6 +267,7 @@  test_4_security_unlock
 # not impact any key management testing via libkeyctl.
 echo "Test 5, freeze security"
 test_5_security_freeze
+exit 1
 
 # Load-keys is independent of actual nvdimm security and is part of key
 # mangement testing.
diff --git a/util/json.c b/util/json.c
index ac834b33d108..524b64fae9a5 100644
--- a/util/json.c
+++ b/util/json.c
@@ -260,6 +260,12 @@  struct json_object *util_dimm_to_json(struct ndctl_dimm *dimm,
 	if (jobj)
 		json_object_object_add(jdimm, "security", jobj);
 
+	if (ndctl_dimm_security_is_frozen(dimm)) {
+		jobj = json_object_new_boolean(true);
+		if (jobj)
+			json_object_object_add(jdimm, "security_frozen", jobj);
+	}
+
 	return jdimm;
  err:
 	json_object_put(jdimm);