Re: [PATCH v4 13/18] nvdimm/cxl/pmem: Add support for master passphrase disable security command

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

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 {
>>
>>