Dan Williams <dan.j.willi...@intel.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. > > Cc: Dave Jiang <dave.ji...@intel.com> > Reported-by: Jeff Moyer <jmo...@redhat.com> > Signed-off-by: Dan Williams <dan.j.willi...@intel.com> > --- > drivers/acpi/nfit/intel.c | 65 ++++++++++++++----------- > 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, 146 insertions(+), 128 deletions(-) > > diff --git a/drivers/acpi/nfit/intel.c b/drivers/acpi/nfit/intel.c > index cddd0fcf622c..2c51ca4155dc 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,46 +28,54 @@ 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. > * The DSM spec states that the security state is indeterminate > * until the overwrite DSM completes. > */ > - if (nvdimm_in_overwrite(nvdimm) && ptype == NVDIMM_USER) > - return NVDIMM_SECURITY_OVERWRITE; > + if (nvdimm_in_overwrite(nvdimm) && ptype == NVDIMM_USER) { > + set_bit(NVDIMM_SECURITY_OVERWRITE, &security_flags); > + return security_flags; > + }
Why not just 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; > + Extra space. > + 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) > + set_bit(NVDIMM_SECURITY_FROZEN, &security_flags); > + if (nd_cmd.cmd.state & ND_INTEL_SEC_STATE_PLIMIT) > + set_bit(NVDIMM_SECURITY_FROZEN, &security_flags); Change to the following? + 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); I was going to comment on 2 bits being overkill, but you addressed that in the enum. :) The rest looks good. -Jeff