nfit: report frozen security state
diff mbox series

Message ID x49k1bw7dqn.fsf@segfault.boston.devel.redhat.com
State New
Headers show
Series
  • nfit: report frozen security state
Related show

Commit Message

Jeff Moyer Aug. 1, 2019, 9:54 p.m. UTC
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>

Comments

Dan Williams Aug. 7, 2019, 9:36 p.m. UTC | #1
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?
Jeff Moyer Aug. 7, 2019, 9:48 p.m. UTC | #2
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
Dan Williams Aug. 7, 2019, 10:27 p.m. UTC | #3
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.
Jeff Moyer Aug. 8, 2019, 8:20 p.m. UTC | #4
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

Patch
diff mbox series

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;
 		}