7:54 a.m.
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(a)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;
}
7:36 a.m.
On Thu, Aug 1, 2019 at 2:55 PM Jeff Moyer <jmoyer(a)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?
7:48 a.m.
Dan Williams <dan.j.williams(a)intel.com> writes:
On Thu, Aug 1, 2019 at 2:55 PM Jeff Moyer <jmoyer(a)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
8:27 a.m.
On Wed, Aug 7, 2019 at 2:48 PM Jeff Moyer <jmoyer(a)redhat.com> wrote:
Dan Williams <dan.j.williams(a)intel.com> writes:
> On Thu, Aug 1, 2019 at 2:55 PM Jeff Moyer <jmoyer(a)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.
6:20 a.m.
Dan Williams <dan.j.williams(a)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
1137
days inactive
1144
days old
4 comments
2 participants
participants (2)
-
Dan Williams -
Jeff Moyer