| Message ID | 166845805988.2496228.8804764265372893076.stgit@djiang5-desk3.ch.intel.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Introduce security commands for CXL pmem device | expand |
On 11/14/22 2:34 PM, Dave Jiang wrote: > The original nvdimm_security_ops ->disable() only supports user passphrase > for security disable. The CXL spec introduced the disabling of master > passphrase. Add a ->disable_master() callback to support this new operation > and leaving the old ->disable() mechanism alone. A "disable_master" command > is added for the sysfs attribute in order to allow command to be issued > from userspace. ndctl will need enabling in order to utilize this new > operation. > > Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com> > Signed-off-by: Dave Jiang <dave.jiang@intel.com> > --- > drivers/cxl/security.c | 20 ++++++++++++++++++-- > drivers/nvdimm/security.c | 33 ++++++++++++++++++++++++++------- > include/linux/libnvdimm.h | 2 ++ > 3 files changed, 46 insertions(+), 9 deletions(-) > > diff --git a/drivers/cxl/security.c b/drivers/cxl/security.c > index 631a474939d6..f4df7d38e4cd 100644 > --- a/drivers/cxl/security.c > +++ b/drivers/cxl/security.c > @@ -71,8 +71,9 @@ static int cxl_pmem_security_change_key(struct nvdimm *nvdimm, > return rc; > } > > -static int cxl_pmem_security_disable(struct nvdimm *nvdimm, > - const struct nvdimm_key_data *key_data) > +static int __cxl_pmem_security_disable(struct nvdimm *nvdimm, > + const struct nvdimm_key_data *key_data, > + enum nvdimm_passphrase_type ptype) > { > struct cxl_nvdimm *cxl_nvd = nvdimm_provider_data(nvdimm); > struct cxl_memdev *cxlmd = cxl_nvd->cxlmd; > @@ -88,6 +89,8 @@ static int cxl_pmem_security_disable(struct nvdimm *nvdimm, > * will only support disable of user passphrase. The disable master passphrase > * ability will need to be added as a new callback. > */ > + dis_pass.type = ptype == NVDIMM_MASTER ? > + CXL_PMEM_SEC_PASS_MASTER : CXL_PMEM_SEC_PASS_USER; > dis_pass.type = CXL_PMEM_SEC_PASS_USER; Hey Dave, I noticed that you are overwriting dis_pass.type with CXL_PMEM_SEC_PASS_USER after your added change here. I imagine that's not intentional considering the rest of the work in this patch! Ben > memcpy(dis_pass.pass, key_data->data, NVDIMM_PASSPHRASE_LEN); > > @@ -96,6 +99,18 @@ static int cxl_pmem_security_disable(struct nvdimm *nvdimm, > return rc; > } > > +static int cxl_pmem_security_disable(struct nvdimm *nvdimm, > + const struct nvdimm_key_data *key_data) > +{ > + return __cxl_pmem_security_disable(nvdimm, key_data, NVDIMM_USER); > +} > + > +static int cxl_pmem_security_disable_master(struct nvdimm *nvdimm, > + const struct nvdimm_key_data *key_data) > +{ > + return __cxl_pmem_security_disable(nvdimm, key_data, NVDIMM_MASTER); > +} > + > static int cxl_pmem_security_freeze(struct nvdimm *nvdimm) > { > struct cxl_nvdimm *cxl_nvd = nvdimm_provider_data(nvdimm); > @@ -163,6 +178,7 @@ static const struct nvdimm_security_ops __cxl_security_ops = { > .freeze = cxl_pmem_security_freeze, > .unlock = cxl_pmem_security_unlock, > .erase = cxl_pmem_security_passphrase_erase, > + .disable_master = cxl_pmem_security_disable_master, > }; > > const struct nvdimm_security_ops *cxl_security_ops = &__cxl_security_ops; > diff --git a/drivers/nvdimm/security.c b/drivers/nvdimm/security.c > index 8aefb60c42ff..92af4c3ca0d3 100644 > --- a/drivers/nvdimm/security.c > +++ b/drivers/nvdimm/security.c > @@ -239,7 +239,8 @@ static int check_security_state(struct nvdimm *nvdimm) > return 0; > } > > -static int security_disable(struct nvdimm *nvdimm, unsigned int keyid) > +static int security_disable(struct nvdimm *nvdimm, unsigned int keyid, > + enum nvdimm_passphrase_type pass_type) > { > struct device *dev = &nvdimm->dev; > struct nvdimm_bus *nvdimm_bus = walk_to_nvdimm_bus(dev); > @@ -250,8 +251,13 @@ static int security_disable(struct nvdimm *nvdimm, unsigned int keyid) > /* 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->disable > - || !nvdimm->sec.flags) > + if (!nvdimm->sec.ops || !nvdimm->sec.flags) > + return -EOPNOTSUPP; > + > + if (pass_type == NVDIMM_USER && !nvdimm->sec.ops->disable) > + return -EOPNOTSUPP; > + > + if (pass_type == NVDIMM_MASTER && !nvdimm->sec.ops->disable_master) > return -EOPNOTSUPP; > > rc = check_security_state(nvdimm); > @@ -263,12 +269,21 @@ static int security_disable(struct nvdimm *nvdimm, unsigned int keyid) > if (!data) > return -ENOKEY; > > - rc = nvdimm->sec.ops->disable(nvdimm, data); > - dev_dbg(dev, "key: %d disable: %s\n", key_serial(key), > + if (pass_type == NVDIMM_MASTER) { > + rc = nvdimm->sec.ops->disable_master(nvdimm, data); > + dev_dbg(dev, "key: %d disable_master: %s\n", key_serial(key), > rc == 0 ? "success" : "fail"); > + } else { > + rc = nvdimm->sec.ops->disable(nvdimm, data); > + dev_dbg(dev, "key: %d disable: %s\n", key_serial(key), > + rc == 0 ? "success" : "fail"); > + } > > nvdimm_put_key(key); > - nvdimm->sec.flags = nvdimm_security_flags(nvdimm, NVDIMM_USER); > + if (pass_type == NVDIMM_MASTER) > + nvdimm->sec.ext_flags = nvdimm_security_flags(nvdimm, NVDIMM_MASTER); > + else > + nvdimm->sec.flags = nvdimm_security_flags(nvdimm, NVDIMM_USER); > return rc; > } > > @@ -473,6 +488,7 @@ void nvdimm_security_overwrite_query(struct work_struct *work) > #define OPS \ > C( OP_FREEZE, "freeze", 1), \ > C( OP_DISABLE, "disable", 2), \ > + C( OP_DISABLE_MASTER, "disable_master", 2), \ > C( OP_UPDATE, "update", 3), \ > C( OP_ERASE, "erase", 2), \ > C( OP_OVERWRITE, "overwrite", 2), \ > @@ -524,7 +540,10 @@ ssize_t nvdimm_security_store(struct device *dev, const char *buf, size_t len) > rc = nvdimm_security_freeze(nvdimm); > } else if (i == OP_DISABLE) { > dev_dbg(dev, "disable %u\n", key); > - rc = security_disable(nvdimm, key); > + rc = security_disable(nvdimm, key, NVDIMM_USER); > + } else if (i == OP_DISABLE_MASTER) { > + dev_dbg(dev, "disable_master %u\n", key); > + rc = security_disable(nvdimm, key, NVDIMM_MASTER); > } else if (i == OP_UPDATE || i == OP_MASTER_UPDATE) { > dev_dbg(dev, "%s %u %u\n", ops[i].name, key, newkey); > rc = security_update(nvdimm, key, newkey, i == OP_UPDATE > diff --git a/include/linux/libnvdimm.h b/include/linux/libnvdimm.h > index c74acfa1a3fe..3bf658a74ccb 100644 > --- a/include/linux/libnvdimm.h > +++ b/include/linux/libnvdimm.h > @@ -183,6 +183,8 @@ struct nvdimm_security_ops { > int (*overwrite)(struct nvdimm *nvdimm, > const struct nvdimm_key_data *key_data); > int (*query_overwrite)(struct nvdimm *nvdimm); > + int (*disable_master)(struct nvdimm *nvdimm, > + const struct nvdimm_key_data *key_data); > }; > > enum nvdimm_fwa_state { > >
On 11/14/2022 2:27 PM, Ben Cheatham wrote: > On 11/14/22 2:34 PM, Dave Jiang wrote: >> The original nvdimm_security_ops ->disable() only supports user >> passphrase >> for security disable. The CXL spec introduced the disabling of master >> passphrase. Add a ->disable_master() callback to support this new >> operation >> and leaving the old ->disable() mechanism alone. A "disable_master" >> command >> is added for the sysfs attribute in order to allow command to be issued >> from userspace. ndctl will need enabling in order to utilize this new >> operation. >> >> Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com> >> Signed-off-by: Dave Jiang <dave.jiang@intel.com> >> --- >> drivers/cxl/security.c | 20 ++++++++++++++++++-- >> drivers/nvdimm/security.c | 33 ++++++++++++++++++++++++++------- >> include/linux/libnvdimm.h | 2 ++ >> 3 files changed, 46 insertions(+), 9 deletions(-) >> >> diff --git a/drivers/cxl/security.c b/drivers/cxl/security.c >> index 631a474939d6..f4df7d38e4cd 100644 >> --- a/drivers/cxl/security.c >> +++ b/drivers/cxl/security.c >> @@ -71,8 +71,9 @@ static int cxl_pmem_security_change_key(struct >> nvdimm *nvdimm, >> return rc; >> } >> -static int cxl_pmem_security_disable(struct nvdimm *nvdimm, >> - const struct nvdimm_key_data *key_data) >> +static int __cxl_pmem_security_disable(struct nvdimm *nvdimm, >> + const struct nvdimm_key_data *key_data, >> + enum nvdimm_passphrase_type ptype) >> { >> struct cxl_nvdimm *cxl_nvd = nvdimm_provider_data(nvdimm); >> struct cxl_memdev *cxlmd = cxl_nvd->cxlmd; >> @@ -88,6 +89,8 @@ static int cxl_pmem_security_disable(struct nvdimm >> *nvdimm, >> * will only support disable of user passphrase. The disable >> master passphrase >> * ability will need to be added as a new callback. >> */ >> + dis_pass.type = ptype == NVDIMM_MASTER ? >> + CXL_PMEM_SEC_PASS_MASTER : CXL_PMEM_SEC_PASS_USER; >> dis_pass.type = CXL_PMEM_SEC_PASS_USER; > > Hey Dave, > > I noticed that you are overwriting dis_pass.type with > CXL_PMEM_SEC_PASS_USER after your added change here. I imagine that's > not intentional considering the rest of the work in this patch! Hi Ben. Great catch! That was suppose to be replaced with the changed code. - dis_pass.type = CXL_PMEM_SEC_PASS_USER; + dis_pass.type = ptype == NVDIMM_MASTER ? + CXL_PMEM_SEC_PASS_MASTER : CXL_PMEM_SEC_PASS_USER; Thanks for the find. > > Ben >> memcpy(dis_pass.pass, key_data->data, NVDIMM_PASSPHRASE_LEN); >> @@ -96,6 +99,18 @@ static int cxl_pmem_security_disable(struct nvdimm >> *nvdimm, >> return rc; >> } >> +static int cxl_pmem_security_disable(struct nvdimm *nvdimm, >> + const struct nvdimm_key_data *key_data) >> +{ >> + return __cxl_pmem_security_disable(nvdimm, key_data, NVDIMM_USER); >> +} >> + >> +static int cxl_pmem_security_disable_master(struct nvdimm *nvdimm, >> + const struct nvdimm_key_data *key_data) >> +{ >> + return __cxl_pmem_security_disable(nvdimm, key_data, NVDIMM_MASTER); >> +} >> + >> static int cxl_pmem_security_freeze(struct nvdimm *nvdimm) >> { >> struct cxl_nvdimm *cxl_nvd = nvdimm_provider_data(nvdimm); >> @@ -163,6 +178,7 @@ static const struct nvdimm_security_ops >> __cxl_security_ops = { >> .freeze = cxl_pmem_security_freeze, >> .unlock = cxl_pmem_security_unlock, >> .erase = cxl_pmem_security_passphrase_erase, >> + .disable_master = cxl_pmem_security_disable_master, >> }; >> const struct nvdimm_security_ops *cxl_security_ops = >> &__cxl_security_ops; >> diff --git a/drivers/nvdimm/security.c b/drivers/nvdimm/security.c >> index 8aefb60c42ff..92af4c3ca0d3 100644 >> --- a/drivers/nvdimm/security.c >> +++ b/drivers/nvdimm/security.c >> @@ -239,7 +239,8 @@ static int check_security_state(struct nvdimm >> *nvdimm) >> return 0; >> } >> -static int security_disable(struct nvdimm *nvdimm, unsigned int keyid) >> +static int security_disable(struct nvdimm *nvdimm, unsigned int keyid, >> + enum nvdimm_passphrase_type pass_type) >> { >> struct device *dev = &nvdimm->dev; >> struct nvdimm_bus *nvdimm_bus = walk_to_nvdimm_bus(dev); >> @@ -250,8 +251,13 @@ static int security_disable(struct nvdimm >> *nvdimm, unsigned int keyid) >> /* 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->disable >> - || !nvdimm->sec.flags) >> + if (!nvdimm->sec.ops || !nvdimm->sec.flags) >> + return -EOPNOTSUPP; >> + >> + if (pass_type == NVDIMM_USER && !nvdimm->sec.ops->disable) >> + return -EOPNOTSUPP; >> + >> + if (pass_type == NVDIMM_MASTER && !nvdimm->sec.ops->disable_master) >> return -EOPNOTSUPP; >> rc = check_security_state(nvdimm); >> @@ -263,12 +269,21 @@ static int security_disable(struct nvdimm >> *nvdimm, unsigned int keyid) >> if (!data) >> return -ENOKEY; >> - rc = nvdimm->sec.ops->disable(nvdimm, data); >> - dev_dbg(dev, "key: %d disable: %s\n", key_serial(key), >> + if (pass_type == NVDIMM_MASTER) { >> + rc = nvdimm->sec.ops->disable_master(nvdimm, data); >> + dev_dbg(dev, "key: %d disable_master: %s\n", key_serial(key), >> rc == 0 ? "success" : "fail"); >> + } else { >> + rc = nvdimm->sec.ops->disable(nvdimm, data); >> + dev_dbg(dev, "key: %d disable: %s\n", key_serial(key), >> + rc == 0 ? "success" : "fail"); >> + } >> nvdimm_put_key(key); >> - nvdimm->sec.flags = nvdimm_security_flags(nvdimm, NVDIMM_USER); >> + if (pass_type == NVDIMM_MASTER) >> + nvdimm->sec.ext_flags = nvdimm_security_flags(nvdimm, >> NVDIMM_MASTER); >> + else >> + nvdimm->sec.flags = nvdimm_security_flags(nvdimm, NVDIMM_USER); >> return rc; >> } >> @@ -473,6 +488,7 @@ void nvdimm_security_overwrite_query(struct >> work_struct *work) >> #define OPS \ >> C( OP_FREEZE, "freeze", 1), \ >> C( OP_DISABLE, "disable", 2), \ >> + C( OP_DISABLE_MASTER, "disable_master", 2), \ >> C( OP_UPDATE, "update", 3), \ >> C( OP_ERASE, "erase", 2), \ >> C( OP_OVERWRITE, "overwrite", 2), \ >> @@ -524,7 +540,10 @@ ssize_t nvdimm_security_store(struct device *dev, >> const char *buf, size_t len) >> rc = nvdimm_security_freeze(nvdimm); >> } else if (i == OP_DISABLE) { >> dev_dbg(dev, "disable %u\n", key); >> - rc = security_disable(nvdimm, key); >> + rc = security_disable(nvdimm, key, NVDIMM_USER); >> + } else if (i == OP_DISABLE_MASTER) { >> + dev_dbg(dev, "disable_master %u\n", key); >> + rc = security_disable(nvdimm, key, NVDIMM_MASTER); >> } else if (i == OP_UPDATE || i == OP_MASTER_UPDATE) { >> dev_dbg(dev, "%s %u %u\n", ops[i].name, key, newkey); >> rc = security_update(nvdimm, key, newkey, i == OP_UPDATE >> diff --git a/include/linux/libnvdimm.h b/include/linux/libnvdimm.h >> index c74acfa1a3fe..3bf658a74ccb 100644 >> --- a/include/linux/libnvdimm.h >> +++ b/include/linux/libnvdimm.h >> @@ -183,6 +183,8 @@ struct nvdimm_security_ops { >> int (*overwrite)(struct nvdimm *nvdimm, >> const struct nvdimm_key_data *key_data); >> int (*query_overwrite)(struct nvdimm *nvdimm); >> + int (*disable_master)(struct nvdimm *nvdimm, >> + const struct nvdimm_key_data *key_data); >> }; >> enum nvdimm_fwa_state { >> >>
diff --git a/drivers/cxl/security.c b/drivers/cxl/security.c index 631a474939d6..f4df7d38e4cd 100644 --- a/drivers/cxl/security.c +++ b/drivers/cxl/security.c @@ -71,8 +71,9 @@ static int cxl_pmem_security_change_key(struct nvdimm *nvdimm, return rc; } -static int cxl_pmem_security_disable(struct nvdimm *nvdimm, - const struct nvdimm_key_data *key_data) +static int __cxl_pmem_security_disable(struct nvdimm *nvdimm, + const struct nvdimm_key_data *key_data, + enum nvdimm_passphrase_type ptype) { struct cxl_nvdimm *cxl_nvd = nvdimm_provider_data(nvdimm); struct cxl_memdev *cxlmd = cxl_nvd->cxlmd; @@ -88,6 +89,8 @@ static int cxl_pmem_security_disable(struct nvdimm *nvdimm, * will only support disable of user passphrase. The disable master passphrase * ability will need to be added as a new callback. */ + dis_pass.type = ptype == NVDIMM_MASTER ? + CXL_PMEM_SEC_PASS_MASTER : CXL_PMEM_SEC_PASS_USER; dis_pass.type = CXL_PMEM_SEC_PASS_USER; memcpy(dis_pass.pass, key_data->data, NVDIMM_PASSPHRASE_LEN); @@ -96,6 +99,18 @@ static int cxl_pmem_security_disable(struct nvdimm *nvdimm, return rc; } +static int cxl_pmem_security_disable(struct nvdimm *nvdimm, + const struct nvdimm_key_data *key_data) +{ + return __cxl_pmem_security_disable(nvdimm, key_data, NVDIMM_USER); +} + +static int cxl_pmem_security_disable_master(struct nvdimm *nvdimm, + const struct nvdimm_key_data *key_data) +{ + return __cxl_pmem_security_disable(nvdimm, key_data, NVDIMM_MASTER); +} + static int cxl_pmem_security_freeze(struct nvdimm *nvdimm) { struct cxl_nvdimm *cxl_nvd = nvdimm_provider_data(nvdimm); @@ -163,6 +178,7 @@ static const struct nvdimm_security_ops __cxl_security_ops = { .freeze = cxl_pmem_security_freeze, .unlock = cxl_pmem_security_unlock, .erase = cxl_pmem_security_passphrase_erase, + .disable_master = cxl_pmem_security_disable_master, }; const struct nvdimm_security_ops *cxl_security_ops = &__cxl_security_ops; diff --git a/drivers/nvdimm/security.c b/drivers/nvdimm/security.c index 8aefb60c42ff..92af4c3ca0d3 100644 --- a/drivers/nvdimm/security.c +++ b/drivers/nvdimm/security.c @@ -239,7 +239,8 @@ static int check_security_state(struct nvdimm *nvdimm) return 0; } -static int security_disable(struct nvdimm *nvdimm, unsigned int keyid) +static int security_disable(struct nvdimm *nvdimm, unsigned int keyid, + enum nvdimm_passphrase_type pass_type) { struct device *dev = &nvdimm->dev; struct nvdimm_bus *nvdimm_bus = walk_to_nvdimm_bus(dev); @@ -250,8 +251,13 @@ static int security_disable(struct nvdimm *nvdimm, unsigned int keyid) /* 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->disable - || !nvdimm->sec.flags) + if (!nvdimm->sec.ops || !nvdimm->sec.flags) + return -EOPNOTSUPP; + + if (pass_type == NVDIMM_USER && !nvdimm->sec.ops->disable) + return -EOPNOTSUPP; + + if (pass_type == NVDIMM_MASTER && !nvdimm->sec.ops->disable_master) return -EOPNOTSUPP; rc = check_security_state(nvdimm); @@ -263,12 +269,21 @@ static int security_disable(struct nvdimm *nvdimm, unsigned int keyid) if (!data) return -ENOKEY; - rc = nvdimm->sec.ops->disable(nvdimm, data); - dev_dbg(dev, "key: %d disable: %s\n", key_serial(key), + if (pass_type == NVDIMM_MASTER) { + rc = nvdimm->sec.ops->disable_master(nvdimm, data); + dev_dbg(dev, "key: %d disable_master: %s\n", key_serial(key), rc == 0 ? "success" : "fail"); + } else { + rc = nvdimm->sec.ops->disable(nvdimm, data); + dev_dbg(dev, "key: %d disable: %s\n", key_serial(key), + rc == 0 ? "success" : "fail"); + } nvdimm_put_key(key); - nvdimm->sec.flags = nvdimm_security_flags(nvdimm, NVDIMM_USER); + if (pass_type == NVDIMM_MASTER) + nvdimm->sec.ext_flags = nvdimm_security_flags(nvdimm, NVDIMM_MASTER); + else + nvdimm->sec.flags = nvdimm_security_flags(nvdimm, NVDIMM_USER); return rc; } @@ -473,6 +488,7 @@ void nvdimm_security_overwrite_query(struct work_struct *work) #define OPS \ C( OP_FREEZE, "freeze", 1), \ C( OP_DISABLE, "disable", 2), \ + C( OP_DISABLE_MASTER, "disable_master", 2), \ C( OP_UPDATE, "update", 3), \ C( OP_ERASE, "erase", 2), \ C( OP_OVERWRITE, "overwrite", 2), \ @@ -524,7 +540,10 @@ ssize_t nvdimm_security_store(struct device *dev, const char *buf, size_t len) rc = nvdimm_security_freeze(nvdimm); } else if (i == OP_DISABLE) { dev_dbg(dev, "disable %u\n", key); - rc = security_disable(nvdimm, key); + rc = security_disable(nvdimm, key, NVDIMM_USER); + } else if (i == OP_DISABLE_MASTER) { + dev_dbg(dev, "disable_master %u\n", key); + rc = security_disable(nvdimm, key, NVDIMM_MASTER); } else if (i == OP_UPDATE || i == OP_MASTER_UPDATE) { dev_dbg(dev, "%s %u %u\n", ops[i].name, key, newkey); rc = security_update(nvdimm, key, newkey, i == OP_UPDATE diff --git a/include/linux/libnvdimm.h b/include/linux/libnvdimm.h index c74acfa1a3fe..3bf658a74ccb 100644 --- a/include/linux/libnvdimm.h +++ b/include/linux/libnvdimm.h @@ -183,6 +183,8 @@ struct nvdimm_security_ops { int (*overwrite)(struct nvdimm *nvdimm, const struct nvdimm_key_data *key_data); int (*query_overwrite)(struct nvdimm *nvdimm); + int (*disable_master)(struct nvdimm *nvdimm, + const struct nvdimm_key_data *key_data); }; enum nvdimm_fwa_state {