| Message ID | 155329790943.63917.7917763965011178186.stgit@djiang5-desk3.ch.intel.com (mailing list archive) |
|---|---|
| State | Mainlined |
| Commit | 037c8489ade669e0f09ad40d5b91e5e1159a14b1 |
| Headers | show |
| Series | [v2] libnvdimm/security: Support a zero-key for secure-erase | expand |
On Fri, Mar 22, 2019 at 4:40 PM Dave Jiang <dave.jiang@intel.com> wrote: > > Adding support to allow secure erase to happen when security state is not > enabled. Key data of 0's will be passed in. > > Signed-off-by: Dave Jiang <dave.jiang@intel.com> > --- > > v2: > - Make patch header explicitly zero key (Dan) > - Declare global static zero key (Dan) > - Make nfit_test explicitly test zero key (Dan) Looks good. I'm going to add a sentence to the changelog pointing out that the kernel is already supporting a zero key for some of the other security interfaces. That makes this effectively a fix to unify semantics across commands, i.e. something I can reasonably justify as a commit that needs to be backported to v5.0 and suitable for v5.1-rc.
On Fri, Mar 22, 2019 at 4:45 PM Dan Williams <dan.j.williams@intel.com> wrote: > > On Fri, Mar 22, 2019 at 4:40 PM Dave Jiang <dave.jiang@intel.com> wrote: > > > > Adding support to allow secure erase to happen when security state is not > > enabled. Key data of 0's will be passed in. > > > > Signed-off-by: Dave Jiang <dave.jiang@intel.com> > > --- > > > > v2: > > - Make patch header explicitly zero key (Dan) > > - Declare global static zero key (Dan) > > - Make nfit_test explicitly test zero key (Dan) > > Looks good. I'm going to add a sentence to the changelog pointing out > that the kernel is already supporting a zero key for some of the other > security interfaces. That makes this effectively a fix to unify > semantics across commands, i.e. something I can reasonably justify as > a commit that needs to be backported to v5.0 and suitable for v5.1-rc. Actually, now that I check we're now supporting the concept of a zero-key differently across commands. The existing semantic is that NULL key data means "use zero key" to the Intel command implementation. Can you respin this with the note about it being a fix to unify semantics across DIMMs and the send a follow on cleanup to switch the other key-id-0 cases to use the new zero key rather than require the ops implementations to special case "NULL" key data?
diff --git a/drivers/nvdimm/security.c b/drivers/nvdimm/security.c index f8bb746a549f..6bea6852bf27 100644 --- a/drivers/nvdimm/security.c +++ b/drivers/nvdimm/security.c @@ -22,6 +22,8 @@ static bool key_revalidate = true; module_param(key_revalidate, bool, 0444); MODULE_PARM_DESC(key_revalidate, "Require key validation at init."); +static const char zero_key[NVDIMM_PASSPHRASE_LEN]; + static void *key_data(struct key *key) { struct encrypted_key_payload *epayload = dereference_key_locked(key); @@ -286,8 +288,9 @@ int nvdimm_security_erase(struct nvdimm *nvdimm, unsigned int keyid, { struct device *dev = &nvdimm->dev; struct nvdimm_bus *nvdimm_bus = walk_to_nvdimm_bus(dev); - struct key *key; + struct key *key = NULL; int rc; + const void *data; /* The bus lock should be held at the top level of the call stack */ lockdep_assert_held(&nvdimm_bus->reconfig_mutex); @@ -319,11 +322,15 @@ int nvdimm_security_erase(struct nvdimm *nvdimm, unsigned int keyid, return -EOPNOTSUPP; } - key = nvdimm_lookup_user_key(nvdimm, keyid, NVDIMM_BASE_KEY); - if (!key) - return -ENOKEY; + if (keyid != 0) { + key = nvdimm_lookup_user_key(nvdimm, keyid, NVDIMM_BASE_KEY); + if (!key) + return -ENOKEY; + data = key_data(key); + } else + data = zero_key; - rc = nvdimm->sec.ops->erase(nvdimm, key_data(key), pass_type); + rc = nvdimm->sec.ops->erase(nvdimm, data, pass_type); dev_dbg(dev, "key: %d erase%s: %s\n", key_serial(key), pass_type == NVDIMM_MASTER ? "(master)" : "(user)", rc == 0 ? "success" : "fail"); diff --git a/tools/testing/nvdimm/test/nfit.c b/tools/testing/nvdimm/test/nfit.c index b579f962451d..cad719876ef4 100644 --- a/tools/testing/nvdimm/test/nfit.c +++ b/tools/testing/nvdimm/test/nfit.c @@ -225,6 +225,8 @@ static struct workqueue_struct *nfit_wq; static struct gen_pool *nfit_pool; +static const char zero_key[NVDIMM_PASSPHRASE_LEN]; + static struct nfit_test *to_nfit_test(struct device *dev) { struct platform_device *pdev = to_platform_device(dev); @@ -1059,8 +1061,7 @@ static int nd_intel_test_cmd_secure_erase(struct nfit_test *t, struct device *dev = &t->pdev.dev; struct nfit_test_sec *sec = &dimm_sec_info[dimm]; - if (!(sec->state & ND_INTEL_SEC_STATE_ENABLED) || - (sec->state & ND_INTEL_SEC_STATE_FROZEN)) { + if (sec->state & ND_INTEL_SEC_STATE_FROZEN) { nd_cmd->status = ND_INTEL_STATUS_INVALID_STATE; dev_dbg(dev, "secure erase: wrong security state\n"); } else if (memcmp(nd_cmd->passphrase, sec->passphrase, @@ -1068,6 +1069,12 @@ static int nd_intel_test_cmd_secure_erase(struct nfit_test *t, nd_cmd->status = ND_INTEL_STATUS_INVALID_PASS; dev_dbg(dev, "secure erase: wrong passphrase\n"); } else { + if (!(sec->state & ND_INTEL_SEC_STATE_ENABLED) + && (memcmp(nd_cmd->passphrase, zero_key, + ND_INTEL_PASSPHRASE_SIZE) != 0)) { + dev_dbg(dev, "invalid zero key\n"); + return 0; + } memset(sec->passphrase, 0, ND_INTEL_PASSPHRASE_SIZE); memset(sec->master_passphrase, 0, ND_INTEL_PASSPHRASE_SIZE); sec->state = 0;
Adding support to allow secure erase to happen when security state is not enabled. Key data of 0's will be passed in. Signed-off-by: Dave Jiang <dave.jiang@intel.com> --- v2: - Make patch header explicitly zero key (Dan) - Declare global static zero key (Dan) - Make nfit_test explicitly test zero key (Dan) drivers/nvdimm/security.c | 17 ++++++++++++----- tools/testing/nvdimm/test/nfit.c | 11 +++++++++-- 2 files changed, 21 insertions(+), 7 deletions(-)