[patch] nfit: report frozen security state

[patch] nfit: report frozen security state

[Jeff Moyer]

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>

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;
 		}
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

Re: [patch] nfit: report frozen security state

[Dan Williams]

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?
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

Re: [patch] nfit: report frozen security state

[Jeff Moyer]

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

_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

Re: [patch] nfit: report frozen security state

[Dan Williams]

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.
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

Re: [patch] nfit: report frozen security state

[Jeff Moyer]

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
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm