Re: [PATCH RFC 06/15] cxl/pmem: Add Disable Passphrase security command support

[Jonathan Cameron <Jonathan.Cameron@huawei.com>]

On Fri, 15 Jul 2022 14:09:07 -0700
Dave Jiang <dave.jiang@intel.com> wrote:

> Create callback function to support the nvdimm_security_ops ->disable()
> callback. Translate the operation to send "Disable Passphrase" security
> command for CXL memory device. The operation supports disabling a
> passphrase for the CXL persistent memory device. In the original
> implementation of nvdimm_security_ops, this operation only supports
> disabling of the user passphrase. This is due to the NFIT version of
> disable passphrase only supported disabling of user passphrase. The CXL
> spec allows disabling of the master passphrase as well which
> nvidmm_security_ops does not support yet. In this commit, the callback
nvdimm...
> function will only support user passphrase.
> 
> See CXL 2.0 spec section 8.2.9.5.6.3 for reference.
> 
> Signed-off-by: Dave Jiang <dave.jiang@intel.com>
Trivial comment inline otherwise lgtm

Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

> ---
>  drivers/cxl/cxlmem.h   |    8 ++++++++
>  drivers/cxl/security.c |   30 ++++++++++++++++++++++++++++++
>  2 files changed, 38 insertions(+)
> 
> diff --git a/drivers/cxl/cxlmem.h b/drivers/cxl/cxlmem.h
> index 1e76d22f4fd2..70a1eb7720d3 100644
> --- a/drivers/cxl/cxlmem.h
> +++ b/drivers/cxl/cxlmem.h
> @@ -252,6 +252,7 @@ enum cxl_opcode {
>  	CXL_MBOX_OP_GET_SCAN_MEDIA	= 0x4305,
>  	CXL_MBOX_OP_GET_SECURITY_STATE	= 0x4500,
>  	CXL_MBOX_OP_SET_PASSPHRASE	= 0x4501,
> +	CXL_MBOX_OP_DISABLE_PASSPHRASE	= 0x4502,
>  	CXL_MBOX_OP_MAX			= 0x10000
>  };
>  
> @@ -359,6 +360,13 @@ struct cxl_set_pass {
>  	u8 new_pass[NVDIMM_PASSPHRASE_LEN];
>  } __packed;
>  
> +/* disable passphrase input payload */
> +struct cxl_disable_pass {
> +	u8 type;
> +	u8 reserved[31];
> +	u8 pass[NVDIMM_PASSPHRASE_LEN];
> +} __packed;
> +
>  enum {
>  	CXL_PMEM_SEC_PASS_MASTER = 0,
>  	CXL_PMEM_SEC_PASS_USER,
> diff --git a/drivers/cxl/security.c b/drivers/cxl/security.c
> index 76ec5087f966..4aec8e41e167 100644
> --- a/drivers/cxl/security.c
> +++ b/drivers/cxl/security.c
> @@ -76,9 +76,39 @@ 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)
> +{
> +	struct cxl_nvdimm *cxl_nvd = nvdimm_provider_data(nvdimm);
> +	struct cxl_memdev *cxlmd = cxl_nvd->cxlmd;
> +	struct cxl_dev_state *cxlds = cxlmd->cxlds;
> +	struct cxl_disable_pass *dis_pass;
> +	int rc;
> +
> +	dis_pass = kzalloc(sizeof(*dis_pass), GFP_KERNEL);

Another fairly small structure. Maybe just put it on the stack...

> +	if (!dis_pass)
> +		return -ENOMEM;
> +
> +	/*
> +	 * While the CXL spec defines the ability to erase the master passphrase,
> +	 * the original nvdimm security ops does not provide that capability.
> +	 * In order to preserve backward compatibility, this callback will
> +	 * only support disable of user passphrase. The disable master passphrase
> +	 * ability will need to be added as a new callback.

Curious. Why is that callback set in stone? If this is exposed directly to userspace
perhaps call that out here.


> +	 */
> +	dis_pass->type = CXL_PMEM_SEC_PASS_USER;
> +	memcpy(dis_pass->pass, key_data->data, NVDIMM_PASSPHRASE_LEN);
> +
> +	rc = cxl_mbox_send_cmd(cxlds, CXL_MBOX_OP_DISABLE_PASSPHRASE,
> +			       dis_pass, sizeof(*dis_pass), NULL, 0);
> +	kfree(dis_pass);
> +	return rc;
> +}
> +
>  static const struct nvdimm_security_ops __cxl_security_ops = {
>  	.get_flags = cxl_pmem_get_security_flags,
>  	.change_key = cxl_pmem_security_change_key,
> +	.disable = cxl_pmem_security_disable,
>  };
>  
>  const struct nvdimm_security_ops *cxl_security_ops = &__cxl_security_ops;
> 
>