Re: [PATCH v4 05/11] nfit/libnvdimm: add set passphrase support for Intel nvdimms

From: Dan Williams <dan.j.williams@intel.com>
To: Dave Jiang <dave.jiang@intel.com>
Cc: David Howells <dhowells@redhat.com>,
	"Schofield, Alison  <alison.schofield@intel.com>,
	keyrings@vger.kernel.org, Kees Cook" <keescook@chromium.org>,
	linux-nvdimm <linux-nvdimm@lists.01.org>
Subject: Re: [PATCH v4 05/11] nfit/libnvdimm: add set passphrase support for Intel nvdimms
Date: Mon, 16 Jul 2018 15:12:26 -0700	[thread overview]
Message-ID: <CAPcyv4iw=vXg4k5JsUE9V1F=AEizAVN3sgxocTqRkr-JLC2vYA@mail.gmail.com> (raw)
In-Reply-To: <a409832f-fccd-7ba2-0bba-c651d326414c@intel.com>

On Mon, Jul 16, 2018 at 2:59 PM, Dave Jiang <dave.jiang@intel.com> wrote:
>
>
> On 07/13/2018 04:26 PM, Dan Williams wrote:
>> On Thu, Jul 12, 2018 at 1:48 PM, Dave Jiang <dave.jiang@intel.com> wrote:
>>> Add support for setting and/or updating passphrase on the Intel nvdimms.
>>> The passphrase is pulled from userspace through the kernel key management.
>>> We trigger the update via writing "update" to the sysfs attribute
>>> "security". The state of the security can also be read via the "security"
>>> attribute. libnvdimm will generically support the key_change API call.
>>>
>>> Signed-off-by: Dave Jiang <dave.jiang@intel.com>
>>> ---
>>>  drivers/acpi/nfit/intel.c  |   57 +++++++++++++++++++++++
>>>  drivers/nvdimm/dimm_devs.c |  110 ++++++++++++++++++++++++++++++++++++++++++++
>>>  include/linux/libnvdimm.h  |    5 ++
>>>  3 files changed, 172 insertions(+)
>>>
>>> diff --git a/drivers/acpi/nfit/intel.c b/drivers/acpi/nfit/intel.c
>>> index 9155b8e63f0e..b0a62248467d 100644
>>> --- a/drivers/acpi/nfit/intel.c
>>> +++ b/drivers/acpi/nfit/intel.c
>>> @@ -18,6 +18,62 @@
>>>  #include "intel.h"
>>>  #include "nfit.h"
>>>
>>> +static int intel_dimm_security_update_passphrase(
>>> +               struct nvdimm_bus *nvdimm_bus, struct nvdimm *nvdimm,
>>> +               struct nvdimm_key_data *old_data,
>>> +               struct nvdimm_key_data *new_data)
>>> +{
>>> +       struct nvdimm_bus_descriptor *nd_desc = to_nd_desc(nvdimm_bus);
>>> +       int cmd_rc, rc = 0;
>>> +       struct nfit_mem *nfit_mem = nvdimm_provider_data(nvdimm);
>>> +       struct {
>>> +               struct nd_cmd_pkg pkg;
>>> +               struct nd_intel_set_passphrase cmd;
>>> +       } nd_cmd = {
>>> +               .pkg = {
>>> +                       .nd_command = NVDIMM_INTEL_SET_PASSPHRASE,
>>> +                       .nd_family = NVDIMM_FAMILY_INTEL,
>>> +                       .nd_size_in = ND_INTEL_PASSPHRASE_SIZE * 2,
>>> +                       .nd_size_out = ND_INTEL_STATUS_SIZE,
>>> +                       .nd_fw_size = ND_INTEL_STATUS_SIZE,
>>> +               },
>>> +               .cmd = {
>>> +                       .status = 0,
>>> +               },
>>> +       };
>>> +
>>> +       if (!test_bit(NVDIMM_INTEL_SET_PASSPHRASE, &nfit_mem->dsm_mask))
>>> +               return -ENOTTY;
>>> +
>>> +       if (old_data)
>>> +               memcpy(nd_cmd.cmd.old_pass, old_data->data,
>>> +                               ND_INTEL_PASSPHRASE_SIZE);
>>> +       memcpy(nd_cmd.cmd.new_pass, new_data->data, ND_INTEL_PASSPHRASE_SIZE);
>>> +       rc = nd_desc->ndctl(nd_desc, nvdimm, ND_CMD_CALL, &nd_cmd,
>>> +                       sizeof(nd_cmd), &cmd_rc);
>>> +       if (rc < 0)
>>> +               goto out;
>>> +       if (cmd_rc < 0) {
>>> +               rc = cmd_rc;
>>> +               goto out;
>>> +       }
>>> +
>>> +       switch (nd_cmd.cmd.status) {
>>> +       case 0:
>>> +               break;
>>> +       case ND_INTEL_STATUS_INVALID_PASS:
>>> +               rc = -EINVAL;
>>> +               goto out;
>>> +       case ND_INTEL_STATUS_INVALID_STATE:
>>> +       default:
>>> +               rc = -ENXIO;
>>> +               goto out;
>>> +       }
>>> +
>>> + out:
>>> +       return rc;
>>> +}
>>> +
>>>  static int intel_dimm_security_unlock(struct nvdimm_bus *nvdimm_bus,
>>>                 struct nvdimm *nvdimm, struct nvdimm_key_data *nkey)
>>>  {
>>> @@ -148,4 +204,5 @@ static int intel_dimm_security_state(struct nvdimm_bus *nvdimm_bus,
>>>  struct nvdimm_security_ops intel_security_ops = {
>>>         .state = intel_dimm_security_state,
>>>         .unlock = intel_dimm_security_unlock,
>>> +       .change_key = intel_dimm_security_update_passphrase,
>>>  };
>>> diff --git a/drivers/nvdimm/dimm_devs.c b/drivers/nvdimm/dimm_devs.c
>>> index 5e190120f4aa..2ab846a2114a 100644
>>> --- a/drivers/nvdimm/dimm_devs.c
>>> +++ b/drivers/nvdimm/dimm_devs.c
>>> @@ -171,6 +171,75 @@ int nvdimm_security_unlock_dimm(struct device *dev)
>>>         return rc;
>>>  }
>>>
>>> +static int nvdimm_security_change_key(struct device *dev)
>>> +{
>>> +       struct nvdimm *nvdimm = to_nvdimm(dev);
>>> +       struct nvdimm_bus *nvdimm_bus = walk_to_nvdimm_bus(dev);
>>> +       struct key *key = NULL, *old_key = NULL;
>>> +       int rc;
>>> +       void *old_data, *new_data;
>>> +
>>> +       if (!nvdimm->security_ops)
>>> +               return 0;
>>> +
>>> +       if (nvdimm->state == NVDIMM_SECURITY_FROZEN)
>>> +               return -EBUSY;
>>> +
>>> +       /* look for a key from keyring if exists and remove */
>>> +       old_key = nvdimm_search_key(dev);
>>> +       if (old_key) {
>>> +               dev_dbg(dev, "%s: killing old key: %#x\n",
>>> +                               __func__, old_key->serial);
>>> +               key_invalidate(old_key);
>>> +               key_put(old_key);
>>> +               /* need key garbage collection to take effect */
>>> +               cond_resched();
>>
>> If we need it to take effect then this won't do it, this is a nop most
>> times, and even if it were plain schedule() what guarantees the
>> garbage collector runs before you get the cpu again?
>>
>> I think you want define a new key_put_sync() api that calls
>> flush_work(&key_gc_work), or otherwise clarify what happens if we
>> don't wait for the garbage collector to run?
>>
>
> It returns -EKEYREVOKED. Can I do something like:
>
> do {
>         key = request_key(...);
>         if (!IS_ERR(key) || PTR_ERR(key) != -EKEYREVOKED)
>                 break;
>         cond_resched();
> } while (1);
>

That's effectively an open-coded flush_work(). I'd just define the new api.

/me goes back to vacation mode.
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm