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

[Dave Jiang <dave.jiang@intel.com>]

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


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