| Message ID | x49k1bw7dqn.fsf@segfault.boston.devel.redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | nfit: report frozen security state | expand |
On Thu, Aug 1, 2019 at 2:55 PM Jeff Moyer <jmoyer@redhat.com> wrote: > > If a dimm is frozen, it is currently reported as being "locked". While > that's not technically wrong, it is misleading as the dimm can't be > unlocked. Fix the confusion. This looks ok, but now I wonder about the case where the DIMM is unlocked, but frozen? I think it makes more sense to show "frozen" when the DIMM is frozen-locked, and show "unlocked" when frozen-unlocked. I.e. if the DIMM is frozen the user should assume it's disabled for general purpose operation, and if it's unlocked the fact that it will fail some security operations is a constrained error case. Thoughts?
Dan Williams <dan.j.williams@intel.com> writes: > On Thu, Aug 1, 2019 at 2:55 PM Jeff Moyer <jmoyer@redhat.com> wrote: >> >> If a dimm is frozen, it is currently reported as being "locked". While >> that's not technically wrong, it is misleading as the dimm can't be >> unlocked. Fix the confusion. > > This looks ok, but now I wonder about the case where the DIMM is > unlocked, but frozen? Hah, forgot that was even a possibility. :) > I think it makes more sense to show "frozen" when the DIMM is > frozen-locked, and show "unlocked" when frozen-unlocked. I.e. if the > DIMM is frozen the user should assume it's disabled for general > purpose operation, and if it's unlocked the fact that it will fail > some security operations is a constrained error case. Thoughts? I think that adds confusion. I think we should print out both whether or not it's locked and whether or not it's frozen. Maybe: unlocked, not frozen: "unlocked" locked, not frozen: "locked" unlocked, frozen: "unlocked (frozen)" locked, frozen: "locked (frozen)" Something like that? I think nvdimm_security_state should be a bitmask, not an enum. That may be a part of the problem. -Jeff
On Wed, Aug 7, 2019 at 2:48 PM Jeff Moyer <jmoyer@redhat.com> wrote: > > Dan Williams <dan.j.williams@intel.com> writes: > > > On Thu, Aug 1, 2019 at 2:55 PM Jeff Moyer <jmoyer@redhat.com> wrote: > >> > >> If a dimm is frozen, it is currently reported as being "locked". While > >> that's not technically wrong, it is misleading as the dimm can't be > >> unlocked. Fix the confusion. > > > > This looks ok, but now I wonder about the case where the DIMM is > > unlocked, but frozen? > > Hah, forgot that was even a possibility. :) > > > I think it makes more sense to show "frozen" when the DIMM is > > frozen-locked, and show "unlocked" when frozen-unlocked. I.e. if the > > DIMM is frozen the user should assume it's disabled for general > > purpose operation, and if it's unlocked the fact that it will fail > > some security operations is a constrained error case. Thoughts? > > I think that adds confusion. It does... > I think we should print out both whether > or not it's locked and whether or not it's frozen. Maybe: > > unlocked, not frozen: "unlocked" > locked, not frozen: "locked" > unlocked, frozen: "unlocked (frozen)" > locked, frozen: "locked (frozen)" > > Something like that? I think nvdimm_security_state should be a bitmask, > not an enum. That may be a part of the problem. It should... ...but ABIs are forever, and ndctl has shipped: if (strcmp(buf, "disabled") == 0) return NDCTL_SECURITY_DISABLED; else if (strcmp(buf, "unlocked") == 0) return NDCTL_SECURITY_UNLOCKED; else if (strcmp(buf, "locked") == 0) return NDCTL_SECURITY_LOCKED; else if (strcmp(buf, "frozen") == 0) return NDCTL_SECURITY_FROZEN; else if (strcmp(buf, "overwrite") == 0) return NDCTL_SECURITY_OVERWRITE; return NDCTL_SECURITY_INVALID; I think we could break out the frozen state to its own new attribute and leave security state to only show "locked" / "unlocked". Then go teach new ndctl to go somewhere else to report frozen.
Dan Williams <dan.j.williams@intel.com> writes: > ...but ABIs are forever, and ndctl has shipped: > > if (strcmp(buf, "disabled") == 0) > return NDCTL_SECURITY_DISABLED; > else if (strcmp(buf, "unlocked") == 0) > return NDCTL_SECURITY_UNLOCKED; > else if (strcmp(buf, "locked") == 0) > return NDCTL_SECURITY_LOCKED; > else if (strcmp(buf, "frozen") == 0) > return NDCTL_SECURITY_FROZEN; > else if (strcmp(buf, "overwrite") == 0) > return NDCTL_SECURITY_OVERWRITE; > return NDCTL_SECURITY_INVALID; > > > I think we could break out the frozen state to its own new attribute > and leave security state to only show "locked" / "unlocked". Then go > teach new ndctl to go somewhere else to report frozen. That works for me. -Jeff
diff --git a/drivers/acpi/nfit/intel.c b/drivers/acpi/nfit/intel.c index cddd0fcf622c..0df2216b2c68 100644 --- a/drivers/acpi/nfit/intel.c +++ b/drivers/acpi/nfit/intel.c @@ -54,12 +54,12 @@ static enum nvdimm_security_state intel_security_state(struct nvdimm *nvdimm, 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 + if (nd_cmd.cmd.state & ND_INTEL_SEC_STATE_FROZEN || nd_cmd.cmd.state & ND_INTEL_SEC_STATE_PLIMIT) return NVDIMM_SECURITY_FROZEN; + else if (nd_cmd.cmd.state & ND_INTEL_SEC_STATE_LOCKED) + return NVDIMM_SECURITY_LOCKED; else return NVDIMM_SECURITY_UNLOCKED; }
If a dimm is frozen, it is currently reported as being "locked". While that's not technically wrong, it is misleading as the dimm can't be unlocked. Fix the confusion. Thanks to Dan for pointing this out. Signed-off-by: Jeff Moyer <jmoyer@redhat.com>