RE: [PATCH v15 07/16] acpi/nfit, libnvdimm: Add unlock of nvdimm support for Intel DIMMs

[Elliott, Robert (Persistent Memory)]

> -----Original Message-----
> From: Linux-nvdimm [mailto:linux-nvdimm-boun...@lists.01.org] On Behalf Of 
> Dave Jiang
> Sent: Thursday, December 13, 2018 5:49 PM
> To: dan.j.willi...@intel.com
> Cc: linux-nvdimm@lists.01.org
> Subject: [PATCH v15 07/16] acpi/nfit, libnvdimm: Add unlock of nvdimm support 
> for Intel DIMMs
> 
> From: Dan Williams <dan.j.willi...@intel.com>
> 
...
> +static int __nvdimm_security_unlock(struct nvdimm *nvdimm)
> +{
> +     struct device *dev = &nvdimm->dev;
> +     struct nvdimm_bus *nvdimm_bus = walk_to_nvdimm_bus(dev);
> +     struct key *key = NULL;
> +     int rc;
> +
> +     /* The bus lock should be held at the top level of the call stack */
> +     lockdep_assert_held(&nvdimm_bus->reconfig_mutex);
> +
> +     if (!nvdimm->sec.ops || !nvdimm->sec.ops->unlock
> +                     || nvdimm->sec.state < 0)
> +             return -EIO;
> +
> +     /*
> +      * If the pre-OS has unlocked the DIMM, attempt to send the key
> +      * from request_key() to the hardware for verification.  Failure
> +      * to revalidate the key against the hardware results in a
> +      * freeze of the security configuration. I.e. if the OS does not
> +      * have the key, security is being managed pre-OS.
> +      */
> +     if (nvdimm->sec.state == NVDIMM_SECURITY_UNLOCKED) {
> +             if (!key_revalidate)
> +                     return 0;
> +
> +             key = nvdimm_key_revalidate(nvdimm);
> +             if (!key)
> +                     return nvdimm_security_freeze(nvdimm);

If it's already unlocked, regardless of whether nvdimm_key_revalidate()
succeeds or fails, I'm not sure you should call nvdimm_security_freeze().  

The kernel doesn't know the key, so a hands-off policy seems prudent -
don't send any commands that change the security state.  Some other
software (pre-OS or application) must be handling the feature, and
sending freeze lock might interfere with that software.

Maybe you intended that to be "if (key)", intending to freeze if the
kernel does have the correct key (so no application can change it and
render the kernel out of sync)?


> +     } else
> +             key = nvdimm_request_key(nvdimm);
> +
> +     if (!key)
> +             return -ENOKEY;
> +
> +     rc = nvdimm->sec.ops->unlock(nvdimm, key_data(key));
> +     dev_dbg(dev, "key: %d unlock: %s\n", key_serial(key),
> +                     rc == 0 ? "success" : "fail");
> +
> +     nvdimm_put_key(key);
> +     nvdimm->sec.state = nvdimm_security_state(nvdimm);
> +     return rc;
> +}
> +

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