Message-ID: <x49blw9897n.fsf@segfault.boston.devel.redhat.com>
Date:   Wed, 28 Aug 2019 13:56:44 -0400
From:   Jeff Moyer <jmoyer@...hat.com>
To:     Dan Williams <dan.j.williams@...el.com>
Cc:     linux-nvdimm@...ts.01.org, Dave Jiang <dave.jiang@...el.com>,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 1/3] libnvdimm/security: Introduce a 'frozen' attribute

Dan Williams <dan.j.williams@...el.com> writes:

> In the process of debugging a system with an NVDIMM that was failing to
> unlock it was found that the kernel is reporting 'locked' while the DIMM
> security interface is 'frozen'. Unfortunately the security state is
> tracked internally as an enum which prevents it from communicating the
> difference between 'locked' and 'locked + frozen'. It follows that the
> enum also prevents the kernel from communicating 'unlocked + frozen'
> which would be useful for debugging why security operations like 'change
> passphrase' are disabled.
>
> Ditch the security state enum for a set of flags and introduce a new
> sysfs attribute explicitly for the 'frozen' state. The regression risk
> is low because the 'frozen' state was already blocked behind the
> 'locked' state, but will need to revisit if there were cases where
> applications need 'frozen' to show up in the primary 'security'
> attribute. The expectation is that communicating 'frozen' is mostly a
> helper for debug and status monitoring.
>
> Reviewed-by: Dave Jiang <dave.jiang@...el.com>
> Reported-by: Jeff Moyer <jmoyer@...hat.com>
> Signed-off-by: Dan Williams <dan.j.williams@...el.com>

Reviewed-by: Jeff Moyer <jmoyer@...hat.com>

> ---
>  drivers/acpi/nfit/intel.c        |   59 ++++++++++++-----------
>  drivers/nvdimm/bus.c             |    2 -
>  drivers/nvdimm/dimm_devs.c       |   59 +++++++++++++----------
>  drivers/nvdimm/nd-core.h         |   21 ++++++--
>  drivers/nvdimm/security.c        |   99 ++++++++++++++++++--------------------
>  include/linux/libnvdimm.h        |    9 ++-
>  tools/testing/nvdimm/dimm_devs.c |   19 ++-----
>  7 files changed, 141 insertions(+), 127 deletions(-)
>
> diff --git a/drivers/acpi/nfit/intel.c b/drivers/acpi/nfit/intel.c
> index cddd0fcf622c..1113b679cd7b 100644
> --- a/drivers/acpi/nfit/intel.c
> +++ b/drivers/acpi/nfit/intel.c
> @@ -7,10 +7,11 @@
>  #include "intel.h"
>  #include "nfit.h"
>  
> -static enum nvdimm_security_state intel_security_state(struct nvdimm *nvdimm,
> +static unsigned long intel_security_flags(struct nvdimm *nvdimm,
>  		enum nvdimm_passphrase_type ptype)
>  {
>  	struct nfit_mem *nfit_mem = nvdimm_provider_data(nvdimm);
> +	unsigned long security_flags = 0;
>  	struct {
>  		struct nd_cmd_pkg pkg;
>  		struct nd_intel_get_security_state cmd;
> @@ -27,7 +28,7 @@ static enum nvdimm_security_state intel_security_state(struct nvdimm *nvdimm,
>  	int rc;
>  
>  	if (!test_bit(NVDIMM_INTEL_GET_SECURITY_STATE, &nfit_mem->dsm_mask))
> -		return -ENXIO;
> +		return 0;
>  
>  	/*
>  	 * Short circuit the state retrieval while we are doing overwrite.
> @@ -35,38 +36,42 @@ static enum nvdimm_security_state intel_security_state(struct nvdimm *nvdimm,
>  	 * until the overwrite DSM completes.
>  	 */
>  	if (nvdimm_in_overwrite(nvdimm) && ptype == NVDIMM_USER)
> -		return NVDIMM_SECURITY_OVERWRITE;
> +		return BIT(NVDIMM_SECURITY_OVERWRITE);
>  
>  	rc = nvdimm_ctl(nvdimm, ND_CMD_CALL, &nd_cmd, sizeof(nd_cmd), NULL);
> -	if (rc < 0)
> -		return rc;
> -	if (nd_cmd.cmd.status)
> -		return -EIO;
> +	if (rc < 0 || nd_cmd.cmd.status) {
> +		pr_err("%s: security state retrieval failed (%d:%#x)\n",
> +				nvdimm_name(nvdimm), rc, nd_cmd.cmd.status);
> +		return 0;
> +	}
>  
>  	/* check and see if security is enabled and locked */
>  	if (ptype == NVDIMM_MASTER) {
>  		if (nd_cmd.cmd.extended_state & ND_INTEL_SEC_ESTATE_ENABLED)
> -			return NVDIMM_SECURITY_UNLOCKED;
> -		else if (nd_cmd.cmd.extended_state &
> -				ND_INTEL_SEC_ESTATE_PLIMIT)
> -			return NVDIMM_SECURITY_FROZEN;
> -	} else {
> -		if (nd_cmd.cmd.state & ND_INTEL_SEC_STATE_UNSUPPORTED)
> -			return -ENXIO;
> -		else if (nd_cmd.cmd.state & ND_INTEL_SEC_STATE_ENABLED) {
> -			if (nd_cmd.cmd.state & ND_INTEL_SEC_STATE_LOCKED)
> -				return NVDIMM_SECURITY_LOCKED;
> -			else if (nd_cmd.cmd.state & ND_INTEL_SEC_STATE_FROZEN
> -					|| nd_cmd.cmd.state &
> -					ND_INTEL_SEC_STATE_PLIMIT)
> -				return NVDIMM_SECURITY_FROZEN;
> -			else
> -				return NVDIMM_SECURITY_UNLOCKED;
> -		}
> +			set_bit(NVDIMM_SECURITY_UNLOCKED, &security_flags);
> +		else
> +			set_bit(NVDIMM_SECURITY_DISABLED, &security_flags);
> +		if (nd_cmd.cmd.extended_state & ND_INTEL_SEC_ESTATE_PLIMIT)
> +			set_bit(NVDIMM_SECURITY_FROZEN, &security_flags);
> +		return security_flags;
>  	}
>  
> -	/* this should cover master security disabled as well */
> -	return NVDIMM_SECURITY_DISABLED;
> +	if (nd_cmd.cmd.state & ND_INTEL_SEC_STATE_UNSUPPORTED)
> +		return 0;
> +
> +	if (nd_cmd.cmd.state & ND_INTEL_SEC_STATE_ENABLED) {
> +		if (nd_cmd.cmd.state & ND_INTEL_SEC_STATE_FROZEN ||
> +		    nd_cmd.cmd.state & ND_INTEL_SEC_STATE_PLIMIT)
> +			set_bit(NVDIMM_SECURITY_FROZEN, &security_flags);
> +
> +		if (nd_cmd.cmd.state & ND_INTEL_SEC_STATE_LOCKED)
> +			set_bit(NVDIMM_SECURITY_LOCKED, &security_flags);
> +		else
> +			set_bit(NVDIMM_SECURITY_UNLOCKED, &security_flags);
> +	} else
> +		set_bit(NVDIMM_SECURITY_DISABLED, &security_flags);
> +
> +	return security_flags;
>  }
>  
>  static int intel_security_freeze(struct nvdimm *nvdimm)
> @@ -371,7 +376,7 @@ static void nvdimm_invalidate_cache(void)
>  #endif
>  
>  static const struct nvdimm_security_ops __intel_security_ops = {
> -	.state = intel_security_state,
> +	.get_flags = intel_security_flags,
>  	.freeze = intel_security_freeze,
>  	.change_key = intel_security_change_key,
>  	.disable = intel_security_disable,
> diff --git a/drivers/nvdimm/bus.c b/drivers/nvdimm/bus.c
> index 798c5c4aea9c..29479d3b01b0 100644
> --- a/drivers/nvdimm/bus.c
> +++ b/drivers/nvdimm/bus.c
> @@ -400,7 +400,7 @@ static int child_unregister(struct device *dev, void *data)
>  
>  		/* We are shutting down. Make state frozen artificially. */
>  		nvdimm_bus_lock(dev);
> -		nvdimm->sec.state = NVDIMM_SECURITY_FROZEN;
> +		set_bit(NVDIMM_SECURITY_FROZEN, &nvdimm->sec.flags);
>  		if (test_and_clear_bit(NDD_WORK_PENDING, &nvdimm->flags))
>  			dev_put = true;
>  		nvdimm_bus_unlock(dev);
> diff --git a/drivers/nvdimm/dimm_devs.c b/drivers/nvdimm/dimm_devs.c
> index 29a065e769ea..53330625fe07 100644
> --- a/drivers/nvdimm/dimm_devs.c
> +++ b/drivers/nvdimm/dimm_devs.c
> @@ -372,24 +372,27 @@ __weak ssize_t security_show(struct device *dev,
>  {
>  	struct nvdimm *nvdimm = to_nvdimm(dev);
>  
> -	switch (nvdimm->sec.state) {
> -	case NVDIMM_SECURITY_DISABLED:
> +	if (test_bit(NVDIMM_SECURITY_DISABLED, &nvdimm->sec.flags))
>  		return sprintf(buf, "disabled\n");
> -	case NVDIMM_SECURITY_UNLOCKED:
> +	if (test_bit(NVDIMM_SECURITY_UNLOCKED, &nvdimm->sec.flags))
>  		return sprintf(buf, "unlocked\n");
> -	case NVDIMM_SECURITY_LOCKED:
> +	if (test_bit(NVDIMM_SECURITY_LOCKED, &nvdimm->sec.flags))
>  		return sprintf(buf, "locked\n");
> -	case NVDIMM_SECURITY_FROZEN:
> -		return sprintf(buf, "frozen\n");
> -	case NVDIMM_SECURITY_OVERWRITE:
> +	if (test_bit(NVDIMM_SECURITY_OVERWRITE, &nvdimm->sec.flags))
>  		return sprintf(buf, "overwrite\n");
> -	default:
> -		return -ENOTTY;
> -	}
> -
>  	return -ENOTTY;
>  }
>  
> +static ssize_t frozen_show(struct device *dev,
> +		struct device_attribute *attr, char *buf)
> +{
> +	struct nvdimm *nvdimm = to_nvdimm(dev);
> +
> +	return sprintf(buf, "%d\n", test_bit(NVDIMM_SECURITY_FROZEN,
> +				&nvdimm->sec.flags));
> +}
> +static DEVICE_ATTR_RO(frozen);
> +
>  #define OPS							\
>  	C( OP_FREEZE,		"freeze",		1),	\
>  	C( OP_DISABLE,		"disable",		2),	\
> @@ -501,6 +504,7 @@ static struct attribute *nvdimm_attributes[] = {
>  	&dev_attr_commands.attr,
>  	&dev_attr_available_slots.attr,
>  	&dev_attr_security.attr,
> +	&dev_attr_frozen.attr,
>  	NULL,
>  };
>  
> @@ -509,17 +513,24 @@ static umode_t nvdimm_visible(struct kobject *kobj, struct attribute *a, int n)
>  	struct device *dev = container_of(kobj, typeof(*dev), kobj);
>  	struct nvdimm *nvdimm = to_nvdimm(dev);
>  
> -	if (a != &dev_attr_security.attr)
> +	if (a != &dev_attr_security.attr && a != &dev_attr_frozen.attr)
>  		return a->mode;
> -	if (nvdimm->sec.state < 0)
> +	if (!nvdimm->sec.flags)
>  		return 0;
> -	/* Are there any state mutation ops? */
> -	if (nvdimm->sec.ops->freeze || nvdimm->sec.ops->disable
> -			|| nvdimm->sec.ops->change_key
> -			|| nvdimm->sec.ops->erase
> -			|| nvdimm->sec.ops->overwrite)
> +
> +	if (a == &dev_attr_security.attr) {
> +		/* Are there any state mutation ops (make writable)? */
> +		if (nvdimm->sec.ops->freeze || nvdimm->sec.ops->disable
> +				|| nvdimm->sec.ops->change_key
> +				|| nvdimm->sec.ops->erase
> +				|| nvdimm->sec.ops->overwrite)
> +			return a->mode;
> +		return 0444;
> +	}
> +
> +	if (nvdimm->sec.ops->freeze)
>  		return a->mode;
> -	return 0444;
> +	return 0;
>  }
>  
>  struct attribute_group nvdimm_attribute_group = {
> @@ -569,8 +580,8 @@ struct nvdimm *__nvdimm_create(struct nvdimm_bus *nvdimm_bus,
>  	 * attribute visibility.
>  	 */
>  	/* get security state and extended (master) state */
> -	nvdimm->sec.state = nvdimm_security_state(nvdimm, NVDIMM_USER);
> -	nvdimm->sec.ext_state = nvdimm_security_state(nvdimm, NVDIMM_MASTER);
> +	nvdimm->sec.flags = nvdimm_security_flags(nvdimm, NVDIMM_USER);
> +	nvdimm->sec.ext_flags = nvdimm_security_flags(nvdimm, NVDIMM_MASTER);
>  	nd_device_register(dev);
>  
>  	return nvdimm;
> @@ -588,7 +599,7 @@ int nvdimm_security_setup_events(struct device *dev)
>  {
>  	struct nvdimm *nvdimm = to_nvdimm(dev);
>  
> -	if (nvdimm->sec.state < 0 || !nvdimm->sec.ops
> +	if (!nvdimm->sec.flags || !nvdimm->sec.ops
>  			|| !nvdimm->sec.ops->overwrite)
>  		return 0;
>  	nvdimm->sec.overwrite_state = sysfs_get_dirent(dev->kobj.sd, "security");
> @@ -614,7 +625,7 @@ int nvdimm_security_freeze(struct nvdimm *nvdimm)
>  	if (!nvdimm->sec.ops || !nvdimm->sec.ops->freeze)
>  		return -EOPNOTSUPP;
>  
> -	if (nvdimm->sec.state < 0)
> +	if (!nvdimm->sec.flags)
>  		return -EIO;
>  
>  	if (test_bit(NDD_SECURITY_OVERWRITE, &nvdimm->flags)) {
> @@ -623,7 +634,7 @@ int nvdimm_security_freeze(struct nvdimm *nvdimm)
>  	}
>  
>  	rc = nvdimm->sec.ops->freeze(nvdimm);
> -	nvdimm->sec.state = nvdimm_security_state(nvdimm, NVDIMM_USER);
> +	nvdimm->sec.flags = nvdimm_security_flags(nvdimm, NVDIMM_USER);
>  
>  	return rc;
>  }
> diff --git a/drivers/nvdimm/nd-core.h b/drivers/nvdimm/nd-core.h
> index 0ac52b6eb00e..da2bbfd56d9f 100644
> --- a/drivers/nvdimm/nd-core.h
> +++ b/drivers/nvdimm/nd-core.h
> @@ -39,21 +39,32 @@ struct nvdimm {
>  	const char *dimm_id;
>  	struct {
>  		const struct nvdimm_security_ops *ops;
> -		enum nvdimm_security_state state;
> -		enum nvdimm_security_state ext_state;
> +		unsigned long flags;
> +		unsigned long ext_flags;
>  		unsigned int overwrite_tmo;
>  		struct kernfs_node *overwrite_state;
>  	} sec;
>  	struct delayed_work dwork;
>  };
>  
> -static inline enum nvdimm_security_state nvdimm_security_state(
> +static inline unsigned long nvdimm_security_flags(
>  		struct nvdimm *nvdimm, enum nvdimm_passphrase_type ptype)
>  {
> +	u64 flags;
> +	const u64 state_flags = 1UL << NVDIMM_SECURITY_DISABLED
> +		| 1UL << NVDIMM_SECURITY_LOCKED
> +		| 1UL << NVDIMM_SECURITY_UNLOCKED
> +		| 1UL << NVDIMM_SECURITY_OVERWRITE;
> +
>  	if (!nvdimm->sec.ops)
> -		return -ENXIO;
> +		return 0;
>  
> -	return nvdimm->sec.ops->state(nvdimm, ptype);
> +	flags = nvdimm->sec.ops->get_flags(nvdimm, ptype);
> +	/* disabled, locked, unlocked, and overwrite are mutually exclusive */
> +	dev_WARN_ONCE(&nvdimm->dev, hweight64(flags & state_flags) > 1,
> +			"reported invalid security state: %#llx\n",
> +			(unsigned long long) flags);
> +	return flags;
>  }
>  int nvdimm_security_freeze(struct nvdimm *nvdimm);
>  #if IS_ENABLED(CONFIG_NVDIMM_KEYS)
> diff --git a/drivers/nvdimm/security.c b/drivers/nvdimm/security.c
> index a570f2263a42..5862d0eee9db 100644
> --- a/drivers/nvdimm/security.c
> +++ b/drivers/nvdimm/security.c
> @@ -158,7 +158,7 @@ static int nvdimm_key_revalidate(struct nvdimm *nvdimm)
>  	}
>  
>  	nvdimm_put_key(key);
> -	nvdimm->sec.state = nvdimm_security_state(nvdimm, NVDIMM_USER);
> +	nvdimm->sec.flags = nvdimm_security_flags(nvdimm, NVDIMM_USER);
>  	return 0;
>  }
>  
> @@ -174,7 +174,7 @@ static int __nvdimm_security_unlock(struct nvdimm *nvdimm)
>  	lockdep_assert_held(&nvdimm_bus->reconfig_mutex);
>  
>  	if (!nvdimm->sec.ops || !nvdimm->sec.ops->unlock
> -			|| nvdimm->sec.state < 0)
> +			|| !nvdimm->sec.flags)
>  		return -EIO;
>  
>  	if (test_bit(NDD_SECURITY_OVERWRITE, &nvdimm->flags)) {
> @@ -189,7 +189,7 @@ static int __nvdimm_security_unlock(struct nvdimm *nvdimm)
>  	 * freeze of the security configuration. I.e. if the OS does not
>  	 * have the key, security is being managed pre-OS.
>  	 */
> -	if (nvdimm->sec.state == NVDIMM_SECURITY_UNLOCKED) {
> +	if (test_bit(NVDIMM_SECURITY_UNLOCKED, &nvdimm->sec.flags)) {
>  		if (!key_revalidate)
>  			return 0;
>  
> @@ -202,7 +202,7 @@ static int __nvdimm_security_unlock(struct nvdimm *nvdimm)
>  			rc == 0 ? "success" : "fail");
>  
>  	nvdimm_put_key(key);
> -	nvdimm->sec.state = nvdimm_security_state(nvdimm, NVDIMM_USER);
> +	nvdimm->sec.flags = nvdimm_security_flags(nvdimm, NVDIMM_USER);
>  	return rc;
>  }
>  
> @@ -217,6 +217,24 @@ int nvdimm_security_unlock(struct device *dev)
>  	return rc;
>  }
>  
> +static int check_security_state(struct nvdimm *nvdimm)
> +{
> +	struct device *dev = &nvdimm->dev;
> +
> +	if (test_bit(NVDIMM_SECURITY_FROZEN, &nvdimm->sec.flags)) {
> +		dev_dbg(dev, "Incorrect security state: %#lx\n",
> +				nvdimm->sec.flags);
> +		return -EIO;
> +	}
> +
> +	if (test_bit(NDD_SECURITY_OVERWRITE, &nvdimm->flags)) {
> +		dev_dbg(dev, "Security operation in progress.\n");
> +		return -EBUSY;
> +	}
> +
> +	return 0;
> +}
> +
>  int nvdimm_security_disable(struct nvdimm *nvdimm, unsigned int keyid)
>  {
>  	struct device *dev = &nvdimm->dev;
> @@ -229,19 +247,12 @@ int nvdimm_security_disable(struct nvdimm *nvdimm, unsigned int keyid)
>  	lockdep_assert_held(&nvdimm_bus->reconfig_mutex);
>  
>  	if (!nvdimm->sec.ops || !nvdimm->sec.ops->disable
> -			|| nvdimm->sec.state < 0)
> +			|| !nvdimm->sec.flags)
>  		return -EOPNOTSUPP;
>  
> -	if (nvdimm->sec.state >= NVDIMM_SECURITY_FROZEN) {
> -		dev_dbg(dev, "Incorrect security state: %d\n",
> -				nvdimm->sec.state);
> -		return -EIO;
> -	}
> -
> -	if (test_bit(NDD_SECURITY_OVERWRITE, &nvdimm->flags)) {
> -		dev_dbg(dev, "Security operation in progress.\n");
> -		return -EBUSY;
> -	}
> +	rc = check_security_state(nvdimm);
> +	if (rc)
> +		return rc;
>  
>  	data = nvdimm_get_user_key_payload(nvdimm, keyid,
>  			NVDIMM_BASE_KEY, &key);
> @@ -253,7 +264,7 @@ int nvdimm_security_disable(struct nvdimm *nvdimm, unsigned int keyid)
>  			rc == 0 ? "success" : "fail");
>  
>  	nvdimm_put_key(key);
> -	nvdimm->sec.state = nvdimm_security_state(nvdimm, NVDIMM_USER);
> +	nvdimm->sec.flags = nvdimm_security_flags(nvdimm, NVDIMM_USER);
>  	return rc;
>  }
>  
> @@ -271,14 +282,12 @@ int nvdimm_security_update(struct nvdimm *nvdimm, unsigned int keyid,
>  	lockdep_assert_held(&nvdimm_bus->reconfig_mutex);
>  
>  	if (!nvdimm->sec.ops || !nvdimm->sec.ops->change_key
> -			|| nvdimm->sec.state < 0)
> +			|| !nvdimm->sec.flags)
>  		return -EOPNOTSUPP;
>  
> -	if (nvdimm->sec.state >= NVDIMM_SECURITY_FROZEN) {
> -		dev_dbg(dev, "Incorrect security state: %d\n",
> -				nvdimm->sec.state);
> -		return -EIO;
> -	}
> +	rc = check_security_state(nvdimm);
> +	if (rc)
> +		return rc;
>  
>  	data = nvdimm_get_user_key_payload(nvdimm, keyid,
>  			NVDIMM_BASE_KEY, &key);
> @@ -301,10 +310,10 @@ int nvdimm_security_update(struct nvdimm *nvdimm, unsigned int keyid,
>  	nvdimm_put_key(newkey);
>  	nvdimm_put_key(key);
>  	if (pass_type == NVDIMM_MASTER)
> -		nvdimm->sec.ext_state = nvdimm_security_state(nvdimm,
> +		nvdimm->sec.ext_flags = nvdimm_security_flags(nvdimm,
>  				NVDIMM_MASTER);
>  	else
> -		nvdimm->sec.state = nvdimm_security_state(nvdimm,
> +		nvdimm->sec.flags = nvdimm_security_flags(nvdimm,
>  				NVDIMM_USER);
>  	return rc;
>  }
> @@ -322,7 +331,7 @@ int nvdimm_security_erase(struct nvdimm *nvdimm, unsigned int keyid,
>  	lockdep_assert_held(&nvdimm_bus->reconfig_mutex);
>  
>  	if (!nvdimm->sec.ops || !nvdimm->sec.ops->erase
> -			|| nvdimm->sec.state < 0)
> +			|| !nvdimm->sec.flags)
>  		return -EOPNOTSUPP;
>  
>  	if (atomic_read(&nvdimm->busy)) {
> @@ -330,18 +339,11 @@ int nvdimm_security_erase(struct nvdimm *nvdimm, unsigned int keyid,
>  		return -EBUSY;
>  	}
>  
> -	if (nvdimm->sec.state >= NVDIMM_SECURITY_FROZEN) {
> -		dev_dbg(dev, "Incorrect security state: %d\n",
> -				nvdimm->sec.state);
> -		return -EIO;
> -	}
> -
> -	if (test_bit(NDD_SECURITY_OVERWRITE, &nvdimm->flags)) {
> -		dev_dbg(dev, "Security operation in progress.\n");
> -		return -EBUSY;
> -	}
> +	rc = check_security_state(nvdimm);
> +	if (rc)
> +		return rc;
>  
> -	if (nvdimm->sec.ext_state != NVDIMM_SECURITY_UNLOCKED
> +	if (!test_bit(NVDIMM_SECURITY_UNLOCKED, &nvdimm->sec.ext_flags)
>  			&& pass_type == NVDIMM_MASTER) {
>  		dev_dbg(dev,
>  			"Attempt to secure erase in wrong master state.\n");
> @@ -359,7 +361,7 @@ int nvdimm_security_erase(struct nvdimm *nvdimm, unsigned int keyid,
>  			rc == 0 ? "success" : "fail");
>  
>  	nvdimm_put_key(key);
> -	nvdimm->sec.state = nvdimm_security_state(nvdimm, NVDIMM_USER);
> +	nvdimm->sec.flags = nvdimm_security_flags(nvdimm, NVDIMM_USER);
>  	return rc;
>  }
>  
> @@ -375,7 +377,7 @@ int nvdimm_security_overwrite(struct nvdimm *nvdimm, unsigned int keyid)
>  	lockdep_assert_held(&nvdimm_bus->reconfig_mutex);
>  
>  	if (!nvdimm->sec.ops || !nvdimm->sec.ops->overwrite
> -			|| nvdimm->sec.state < 0)
> +			|| !nvdimm->sec.flags)
>  		return -EOPNOTSUPP;
>  
>  	if (atomic_read(&nvdimm->busy)) {
> @@ -388,16 +390,9 @@ int nvdimm_security_overwrite(struct nvdimm *nvdimm, unsigned int keyid)
>  		return -EINVAL;
>  	}
>  
> -	if (nvdimm->sec.state >= NVDIMM_SECURITY_FROZEN) {
> -		dev_dbg(dev, "Incorrect security state: %d\n",
> -				nvdimm->sec.state);
> -		return -EIO;
> -	}
> -
> -	if (test_bit(NDD_SECURITY_OVERWRITE, &nvdimm->flags)) {
> -		dev_dbg(dev, "Security operation in progress.\n");
> -		return -EBUSY;
> -	}
> +	rc = check_security_state(nvdimm);
> +	if (rc)
> +		return rc;
>  
>  	data = nvdimm_get_user_key_payload(nvdimm, keyid,
>  			NVDIMM_BASE_KEY, &key);
> @@ -412,7 +407,7 @@ int nvdimm_security_overwrite(struct nvdimm *nvdimm, unsigned int keyid)
>  	if (rc == 0) {
>  		set_bit(NDD_SECURITY_OVERWRITE, &nvdimm->flags);
>  		set_bit(NDD_WORK_PENDING, &nvdimm->flags);
> -		nvdimm->sec.state = NVDIMM_SECURITY_OVERWRITE;
> +		set_bit(NVDIMM_SECURITY_OVERWRITE, &nvdimm->sec.flags);
>  		/*
>  		 * Make sure we don't lose device while doing overwrite
>  		 * query.
> @@ -443,7 +438,7 @@ void __nvdimm_security_overwrite_query(struct nvdimm *nvdimm)
>  	tmo = nvdimm->sec.overwrite_tmo;
>  
>  	if (!nvdimm->sec.ops || !nvdimm->sec.ops->query_overwrite
> -			|| nvdimm->sec.state < 0)
> +			|| !nvdimm->sec.flags)
>  		return;
>  
>  	rc = nvdimm->sec.ops->query_overwrite(nvdimm);
> @@ -467,8 +462,8 @@ void __nvdimm_security_overwrite_query(struct nvdimm *nvdimm)
>  	clear_bit(NDD_SECURITY_OVERWRITE, &nvdimm->flags);
>  	clear_bit(NDD_WORK_PENDING, &nvdimm->flags);
>  	put_device(&nvdimm->dev);
> -	nvdimm->sec.state = nvdimm_security_state(nvdimm, NVDIMM_USER);
> -	nvdimm->sec.ext_state = nvdimm_security_state(nvdimm, NVDIMM_MASTER);
> +	nvdimm->sec.flags = nvdimm_security_flags(nvdimm, NVDIMM_USER);
> +	nvdimm->sec.flags = nvdimm_security_flags(nvdimm, NVDIMM_MASTER);
>  }
>  
>  void nvdimm_security_overwrite_query(struct work_struct *work)
> diff --git a/include/linux/libnvdimm.h b/include/linux/libnvdimm.h
> index 7a64b3ddb408..b6eddf912568 100644
> --- a/include/linux/libnvdimm.h
> +++ b/include/linux/libnvdimm.h
> @@ -160,8 +160,11 @@ static inline struct nd_blk_region_desc *to_blk_region_desc(
>  
>  }
>  
> -enum nvdimm_security_state {
> -	NVDIMM_SECURITY_ERROR = -1,
> +/*
> + * Note that separate bits for locked + unlocked are defined so that
> + * 'flags == 0' corresponds to an error / not-supported state.
> + */
> +enum nvdimm_security_bits {
>  	NVDIMM_SECURITY_DISABLED,
>  	NVDIMM_SECURITY_UNLOCKED,
>  	NVDIMM_SECURITY_LOCKED,
> @@ -182,7 +185,7 @@ enum nvdimm_passphrase_type {
>  };
>  
>  struct nvdimm_security_ops {
> -	enum nvdimm_security_state (*state)(struct nvdimm *nvdimm,
> +	unsigned long (*get_flags)(struct nvdimm *nvdimm,
>  			enum nvdimm_passphrase_type pass_type);
>  	int (*freeze)(struct nvdimm *nvdimm);
>  	int (*change_key)(struct nvdimm *nvdimm,
> diff --git a/tools/testing/nvdimm/dimm_devs.c b/tools/testing/nvdimm/dimm_devs.c
> index 2d4baf57822f..57bd27dedf1f 100644
> --- a/tools/testing/nvdimm/dimm_devs.c
> +++ b/tools/testing/nvdimm/dimm_devs.c
> @@ -18,24 +18,13 @@ ssize_t security_show(struct device *dev,
>  	 * For the test version we need to poll the "hardware" in order
>  	 * to get the updated status for unlock testing.
>  	 */
> -	nvdimm->sec.state = nvdimm_security_state(nvdimm, NVDIMM_USER);
> -	nvdimm->sec.ext_state = nvdimm_security_state(nvdimm, NVDIMM_MASTER);
> +	nvdimm->sec.flags = nvdimm_security_flags(nvdimm, NVDIMM_USER);
>  
> -	switch (nvdimm->sec.state) {
> -	case NVDIMM_SECURITY_DISABLED:
> +	if (test_bit(NVDIMM_SECURITY_DISABLED, &nvdimm->sec.flags))
>  		return sprintf(buf, "disabled\n");
> -	case NVDIMM_SECURITY_UNLOCKED:
> +	if (test_bit(NVDIMM_SECURITY_UNLOCKED, &nvdimm->sec.flags))
>  		return sprintf(buf, "unlocked\n");
> -	case NVDIMM_SECURITY_LOCKED:
> +	if (test_bit(NVDIMM_SECURITY_LOCKED, &nvdimm->sec.flags))
>  		return sprintf(buf, "locked\n");
> -	case NVDIMM_SECURITY_FROZEN:
> -		return sprintf(buf, "frozen\n");
> -	case NVDIMM_SECURITY_OVERWRITE:
> -		return sprintf(buf, "overwrite\n");
> -	default:
> -		return -ENOTTY;
> -	}
> -
>  	return -ENOTTY;
>  }
> -

Powered by blists - more mailing lists