[PATCH 0/4] cxl/mbox: Output payload validation reworks

[PATCH 0/4] cxl/mbox: Output payload validation reworks

[Dan Williams]

cxl_mbox_send_cmd() mentions:

        /*
         * Variable sized commands can't be validated and so it's up to the
         * caller to do that if they wish.
         */

...but it turns out that is not true. The caller never sees the
resulting output size, so all currently all kernel-internal
variable-output-size command code paths skip output payload size
validation. Additionally, to get any output size validation even for
non-variable sized commands, the command must appear in the
cxl_mem_commands array. That is a waste, especially for internal only
commands like PMEM security commands, because appearing in
cxl_mem_commands currently also a requires new entries in
include/uapi/linux/cxl_mem.h.

Fix this situation by pushing the responsibility to construct a 'struct
cxl_mbox_cmd' to callers. Rename cxl_mbox_send_cmd() to differentiate it
from the ioctl path handling. Add support for validating variable sized
output payloads optionally by a minimum size. Unrelated, but needs to be
fixed before this rework, fixup the endian handling of "Get Security
State" output in cxl_pmem_get_security_flags(). Lastly, remove the
uapi definitions for the security commands, only the opcodes need to be
defined for internal command usage.

---

Dan Williams (4):
      cxl/security: Fix Get Security State output payload endian handling
      cxl/mbox: Enable cxl_mbox_send_cmd() users to validate output size
      cxl/mbox: Add variable output size validation for internal commands
      cxl/security: Drop security command ioctl uapi


 drivers/cxl/core/mbox.c      |  118 ++++++++++++++++++++++--------------------
 drivers/cxl/cxlmem.h         |    6 +-
 drivers/cxl/pmem.c           |   21 ++++++-
 drivers/cxl/security.c       |   81 +++++++++++++++++++++--------
 include/uapi/linux/cxl_mem.h |    6 --
 5 files changed, 142 insertions(+), 90 deletions(-)

base-commit: 02fedf1466567424c336cd11cf368dcf78f2af33

[PATCH 1/4] cxl/security: Fix Get Security State output payload endian handling

[Dan Williams]

Multi-byte integer values in CXL mailbox payloads are little endian. Add
a definition of the Get Security State output payload and convert the
value before testing flags.

Fixes: 328281155539 ("cxl/pmem: Introduce nvdimm_security_ops with ->get_flags() operation")
Cc: Jonathan Cameron <Jonathan.Cameron@huawei.com>
Cc: Dave Jiang <dave.jiang@intel.com>
Signed-off-by: Dan Williams <dan.j.williams@intel.com>
---
 drivers/cxl/security.c |    6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/cxl/security.c b/drivers/cxl/security.c
index 5484d4eecfd1..ebb78b8944f5 100644
--- a/drivers/cxl/security.c
+++ b/drivers/cxl/security.c
@@ -16,14 +16,18 @@ static unsigned long cxl_pmem_get_security_flags(struct nvdimm *nvdimm,
 	struct cxl_memdev *cxlmd = cxl_nvd->cxlmd;
 	struct cxl_dev_state *cxlds = cxlmd->cxlds;
 	unsigned long security_flags = 0;
+	struct cxl_get_security_output {
+		__le32 flags;
+	} out;
 	u32 sec_out;
 	int rc;
 
 	rc = cxl_mbox_send_cmd(cxlds, CXL_MBOX_OP_GET_SECURITY_STATE, NULL, 0,
-			       &sec_out, sizeof(sec_out));
+			       &out, sizeof(out));
 	if (rc < 0)
 		return 0;
 
+	sec_out = le32_to_cpu(out.flags);
 	if (ptype == NVDIMM_MASTER) {
 		if (sec_out & CXL_PMEM_SEC_STATE_MASTER_PASS_SET)
 			set_bit(NVDIMM_SECURITY_UNLOCKED, &security_flags);

Re: [PATCH 1/4] cxl/security: Fix Get Security State output payload endian handling

[Ira Weiny]

On Mon, Dec 05, 2022 at 08:22:28PM -0800, Dan Williams wrote:
> Multi-byte integer values in CXL mailbox payloads are little endian. Add
> a definition of the Get Security State output payload and convert the
> value before testing flags.
> 
> Fixes: 328281155539 ("cxl/pmem: Introduce nvdimm_security_ops with ->get_flags() operation")
> Cc: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> Cc: Dave Jiang <dave.jiang@intel.com>
> Signed-off-by: Dan Williams <dan.j.williams@intel.com>

Reviewed-by: Ira Weiny <ira.weiny@intel.com>

> ---
>  drivers/cxl/security.c |    6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/cxl/security.c b/drivers/cxl/security.c
> index 5484d4eecfd1..ebb78b8944f5 100644
> --- a/drivers/cxl/security.c
> +++ b/drivers/cxl/security.c
> @@ -16,14 +16,18 @@ static unsigned long cxl_pmem_get_security_flags(struct nvdimm *nvdimm,
>  	struct cxl_memdev *cxlmd = cxl_nvd->cxlmd;
>  	struct cxl_dev_state *cxlds = cxlmd->cxlds;
>  	unsigned long security_flags = 0;
> +	struct cxl_get_security_output {
> +		__le32 flags;
> +	} out;
>  	u32 sec_out;
>  	int rc;
>  
>  	rc = cxl_mbox_send_cmd(cxlds, CXL_MBOX_OP_GET_SECURITY_STATE, NULL, 0,
> -			       &sec_out, sizeof(sec_out));
> +			       &out, sizeof(out));
>  	if (rc < 0)
>  		return 0;
>  
> +	sec_out = le32_to_cpu(out.flags);
>  	if (ptype == NVDIMM_MASTER) {
>  		if (sec_out & CXL_PMEM_SEC_STATE_MASTER_PASS_SET)
>  			set_bit(NVDIMM_SECURITY_UNLOCKED, &security_flags);
> 

Re: [PATCH 1/4] cxl/security: Fix Get Security State output payload endian handling

[Dave Jiang]

On 12/5/2022 9:22 PM, Dan Williams wrote:
> Multi-byte integer values in CXL mailbox payloads are little endian. Add
> a definition of the Get Security State output payload and convert the
> value before testing flags.
> 
> Fixes: 328281155539 ("cxl/pmem: Introduce nvdimm_security_ops with ->get_flags() operation")
> Cc: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> Cc: Dave Jiang <dave.jiang@intel.com>
> Signed-off-by: Dan Williams <dan.j.williams@intel.com>

Reviewed-by: Dave Jiang <dave.jiang@intel.com>

Looks like I missed the endian conversion. Thanks!

> ---
>   drivers/cxl/security.c |    6 +++++-
>   1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/cxl/security.c b/drivers/cxl/security.c
> index 5484d4eecfd1..ebb78b8944f5 100644
> --- a/drivers/cxl/security.c
> +++ b/drivers/cxl/security.c
> @@ -16,14 +16,18 @@ static unsigned long cxl_pmem_get_security_flags(struct nvdimm *nvdimm,
>   	struct cxl_memdev *cxlmd = cxl_nvd->cxlmd;
>   	struct cxl_dev_state *cxlds = cxlmd->cxlds;
>   	unsigned long security_flags = 0;
> +	struct cxl_get_security_output {
> +		__le32 flags;
> +	} out;
>   	u32 sec_out;
>   	int rc;
>   
>   	rc = cxl_mbox_send_cmd(cxlds, CXL_MBOX_OP_GET_SECURITY_STATE, NULL, 0,
> -			       &sec_out, sizeof(sec_out));
> +			       &out, sizeof(out));
>   	if (rc < 0)
>   		return 0;
>   
> +	sec_out = le32_to_cpu(out.flags);
>   	if (ptype == NVDIMM_MASTER) {
>   		if (sec_out & CXL_PMEM_SEC_STATE_MASTER_PASS_SET)
>   			set_bit(NVDIMM_SECURITY_UNLOCKED, &security_flags);
> 

Re: [PATCH 1/4] cxl/security: Fix Get Security State output payload endian handling

[Jonathan Cameron]

On Mon, 05 Dec 2022 20:22:28 -0800
Dan Williams <dan.j.williams@intel.com> wrote:

> Multi-byte integer values in CXL mailbox payloads are little endian. Add
> a definition of the Get Security State output payload and convert the
> value before testing flags.
> 
> Fixes: 328281155539 ("cxl/pmem: Introduce nvdimm_security_ops with ->get_flags() operation")
> Cc: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> Cc: Dave Jiang <dave.jiang@intel.com>
> Signed-off-by: Dan Williams <dan.j.williams@intel.com>
Ah. Missed this one. Good catch.

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


> ---
>  drivers/cxl/security.c |    6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/cxl/security.c b/drivers/cxl/security.c
> index 5484d4eecfd1..ebb78b8944f5 100644
> --- a/drivers/cxl/security.c
> +++ b/drivers/cxl/security.c
> @@ -16,14 +16,18 @@ static unsigned long cxl_pmem_get_security_flags(struct nvdimm *nvdimm,
>  	struct cxl_memdev *cxlmd = cxl_nvd->cxlmd;
>  	struct cxl_dev_state *cxlds = cxlmd->cxlds;
>  	unsigned long security_flags = 0;
> +	struct cxl_get_security_output {
> +		__le32 flags;
> +	} out;
>  	u32 sec_out;
>  	int rc;
>  
>  	rc = cxl_mbox_send_cmd(cxlds, CXL_MBOX_OP_GET_SECURITY_STATE, NULL, 0,
> -			       &sec_out, sizeof(sec_out));
> +			       &out, sizeof(out));
>  	if (rc < 0)
>  		return 0;
>  
> +	sec_out = le32_to_cpu(out.flags);
>  	if (ptype == NVDIMM_MASTER) {
>  		if (sec_out & CXL_PMEM_SEC_STATE_MASTER_PASS_SET)
>  			set_bit(NVDIMM_SECURITY_UNLOCKED, &security_flags);
> 

[PATCH 2/4] cxl/mbox: Enable cxl_mbox_send_cmd() users to validate output size

[Dan Williams]

Internally cxl_mbox_send_cmd() converts all passed-in parameters to a
'struct cxl_mbox_cmd' instance and sends that to cxlds->mbox_send(). It
then teases the possibilty that the caller can validate the output size.
However, they cannot since the resulting output size is not conveyed to
the called. Fix that by making the caller pass in a constructed 'struct
cxl_mbox_cmd'. This prepares for a future patch to add output size
validation on a per-command basis.

Given the change in signature, also change the name to differentiate it
from the user command submission path that performs more validation
before generating the 'struct cxl_mbox_cmd' instance to execute.

Signed-off-by: Dan Williams <dan.j.williams@intel.com>
---
 drivers/cxl/core/mbox.c |   86 ++++++++++++++++++++++++++++-------------------
 drivers/cxl/cxlmem.h    |    4 +-
 drivers/cxl/pmem.c      |   21 +++++++++--
 drivers/cxl/security.c  |   77 +++++++++++++++++++++++++++++++-----------
 4 files changed, 126 insertions(+), 62 deletions(-)

diff --git a/drivers/cxl/core/mbox.c b/drivers/cxl/core/mbox.c
index 35dd889f1d3a..ed451ca60ce5 100644
--- a/drivers/cxl/core/mbox.c
+++ b/drivers/cxl/core/mbox.c
@@ -146,13 +146,9 @@ static const char *cxl_mem_opcode_to_name(u16 opcode)
 }
 
 /**
- * cxl_mbox_send_cmd() - Send a mailbox command to a device.
+ * cxl_internal_send_cmd() - Kernel internal interface to send a mailbox command
  * @cxlds: The device data for the operation
- * @opcode: Opcode for the mailbox command.
- * @in: The input payload for the mailbox command.
- * @in_size: The length of the input payload
- * @out: Caller allocated buffer for the output.
- * @out_size: Expected size of output.
+ * @mbox_cmd: initialized command to execute
  *
  * Context: Any context.
  * Return:
@@ -167,40 +163,37 @@ static const char *cxl_mem_opcode_to_name(u16 opcode)
  * error. While this distinction can be useful for commands from userspace, the
  * kernel will only be able to use results when both are successful.
  */
-int cxl_mbox_send_cmd(struct cxl_dev_state *cxlds, u16 opcode, void *in,
-		      size_t in_size, void *out, size_t out_size)
+int cxl_internal_send_cmd(struct cxl_dev_state *cxlds,
+			  struct cxl_mbox_cmd *mbox_cmd)
 {
-	const struct cxl_mem_command *cmd = cxl_mem_find_command(opcode);
-	struct cxl_mbox_cmd mbox_cmd = {
-		.opcode = opcode,
-		.payload_in = in,
-		.size_in = in_size,
-		.size_out = out_size,
-		.payload_out = out,
-	};
+	const struct cxl_mem_command *cmd =
+		cxl_mem_find_command(mbox_cmd->opcode);
+	size_t out_size;
 	int rc;
 
-	if (in_size > cxlds->payload_size || out_size > cxlds->payload_size)
+	if (mbox_cmd->size_in > cxlds->payload_size ||
+	    mbox_cmd->size_out > cxlds->payload_size)
 		return -E2BIG;
 
-	rc = cxlds->mbox_send(cxlds, &mbox_cmd);
+	out_size = mbox_cmd->size_out;
+	rc = cxlds->mbox_send(cxlds, mbox_cmd);
 	if (rc)
 		return rc;
 
-	if (mbox_cmd.return_code != CXL_MBOX_CMD_RC_SUCCESS)
-		return cxl_mbox_cmd_rc2errno(&mbox_cmd);
+	if (mbox_cmd->return_code != CXL_MBOX_CMD_RC_SUCCESS)
+		return cxl_mbox_cmd_rc2errno(mbox_cmd);
 
 	/*
 	 * Variable sized commands can't be validated and so it's up to the
 	 * caller to do that if they wish.
 	 */
 	if (cmd->info.size_out != CXL_VARIABLE_PAYLOAD) {
-		if (mbox_cmd.size_out != out_size)
+		if (mbox_cmd->size_out != out_size)
 			return -EIO;
 	}
 	return 0;
 }
-EXPORT_SYMBOL_NS_GPL(cxl_mbox_send_cmd, CXL);
+EXPORT_SYMBOL_NS_GPL(cxl_internal_send_cmd, CXL);
 
 static bool cxl_mem_raw_command_allowed(u16 opcode)
 {
@@ -567,15 +560,25 @@ static int cxl_xfer_log(struct cxl_dev_state *cxlds, uuid_t *uuid, u32 size, u8
 
 	while (remaining) {
 		u32 xfer_size = min_t(u32, remaining, cxlds->payload_size);
-		struct cxl_mbox_get_log log = {
+		struct cxl_mbox_cmd mbox_cmd;
+		struct cxl_mbox_get_log log;
+		int rc;
+
+		log = (struct cxl_mbox_get_log) {
 			.uuid = *uuid,
 			.offset = cpu_to_le32(offset),
-			.length = cpu_to_le32(xfer_size)
+			.length = cpu_to_le32(xfer_size),
+		};
+
+		mbox_cmd = (struct cxl_mbox_cmd) {
+			.opcode = CXL_MBOX_OP_GET_LOG,
+			.size_in = sizeof(log),
+			.payload_in = &log,
+			.size_out = xfer_size,
+			.payload_out = out,
 		};
-		int rc;
 
-		rc = cxl_mbox_send_cmd(cxlds, CXL_MBOX_OP_GET_LOG, &log, sizeof(log),
-				       out, xfer_size);
+		rc = cxl_internal_send_cmd(cxlds, &mbox_cmd);
 		if (rc < 0)
 			return rc;
 
@@ -621,19 +624,25 @@ static void cxl_walk_cel(struct cxl_dev_state *cxlds, size_t size, u8 *cel)
 static struct cxl_mbox_get_supported_logs *cxl_get_gsl(struct cxl_dev_state *cxlds)
 {
 	struct cxl_mbox_get_supported_logs *ret;
+	struct cxl_mbox_cmd mbox_cmd;
 	int rc;
 
 	ret = kvmalloc(cxlds->payload_size, GFP_KERNEL);
 	if (!ret)
 		return ERR_PTR(-ENOMEM);
 
-	rc = cxl_mbox_send_cmd(cxlds, CXL_MBOX_OP_GET_SUPPORTED_LOGS, NULL, 0, ret,
-			       cxlds->payload_size);
+	mbox_cmd = (struct cxl_mbox_cmd) {
+		.opcode = CXL_MBOX_OP_GET_SUPPORTED_LOGS,
+		.size_out = cxlds->payload_size,
+		.payload_out = ret,
+	};
+	rc = cxl_internal_send_cmd(cxlds, &mbox_cmd);
 	if (rc < 0) {
 		kvfree(ret);
 		return ERR_PTR(rc);
 	}
 
+
 	return ret;
 }
 
@@ -735,11 +744,15 @@ EXPORT_SYMBOL_NS_GPL(cxl_enumerate_cmds, CXL);
 static int cxl_mem_get_partition_info(struct cxl_dev_state *cxlds)
 {
 	struct cxl_mbox_get_partition_info pi;
+	struct cxl_mbox_cmd mbox_cmd;
 	int rc;
 
-	rc = cxl_mbox_send_cmd(cxlds, CXL_MBOX_OP_GET_PARTITION_INFO, NULL, 0,
-			       &pi, sizeof(pi));
-
+	mbox_cmd = (struct cxl_mbox_cmd) {
+		.opcode = CXL_MBOX_OP_GET_PARTITION_INFO,
+		.size_out = sizeof(pi),
+		.payload_out = &pi,
+	};
+	rc = cxl_internal_send_cmd(cxlds, &mbox_cmd);
 	if (rc)
 		return rc;
 
@@ -768,10 +781,15 @@ int cxl_dev_state_identify(struct cxl_dev_state *cxlds)
 {
 	/* See CXL 2.0 Table 175 Identify Memory Device Output Payload */
 	struct cxl_mbox_identify id;
+	struct cxl_mbox_cmd mbox_cmd;
 	int rc;
 
-	rc = cxl_mbox_send_cmd(cxlds, CXL_MBOX_OP_IDENTIFY, NULL, 0, &id,
-			       sizeof(id));
+	mbox_cmd = (struct cxl_mbox_cmd) {
+		.opcode = CXL_MBOX_OP_IDENTIFY,
+		.size_out = sizeof(id),
+		.payload_out = &id,
+	};
+	rc = cxl_internal_send_cmd(cxlds, &mbox_cmd);
 	if (rc < 0)
 		return rc;
 
diff --git a/drivers/cxl/cxlmem.h b/drivers/cxl/cxlmem.h
index 785c6c12515d..c447577f5ad5 100644
--- a/drivers/cxl/cxlmem.h
+++ b/drivers/cxl/cxlmem.h
@@ -430,8 +430,8 @@ enum {
 	CXL_PMEM_SEC_PASS_USER,
 };
 
-int cxl_mbox_send_cmd(struct cxl_dev_state *cxlds, u16 opcode, void *in,
-		      size_t in_size, void *out, size_t out_size);
+int cxl_internal_send_cmd(struct cxl_dev_state *cxlds,
+			  struct cxl_mbox_cmd *cmd);
 int cxl_dev_state_identify(struct cxl_dev_state *cxlds);
 int cxl_await_media_ready(struct cxl_dev_state *cxlds);
 int cxl_enumerate_cmds(struct cxl_dev_state *cxlds);
diff --git a/drivers/cxl/pmem.c b/drivers/cxl/pmem.c
index 2fc8070b6a17..eedefebc4283 100644
--- a/drivers/cxl/pmem.c
+++ b/drivers/cxl/pmem.c
@@ -119,6 +119,7 @@ static int cxl_pmem_get_config_data(struct cxl_dev_state *cxlds,
 				    unsigned int buf_len)
 {
 	struct cxl_mbox_get_lsa get_lsa;
+	struct cxl_mbox_cmd mbox_cmd;
 	int rc;
 
 	if (sizeof(*cmd) > buf_len)
@@ -130,9 +131,15 @@ static int cxl_pmem_get_config_data(struct cxl_dev_state *cxlds,
 		.offset = cpu_to_le32(cmd->in_offset),
 		.length = cpu_to_le32(cmd->in_length),
 	};
+	mbox_cmd = (struct cxl_mbox_cmd) {
+		.opcode = CXL_MBOX_OP_GET_LSA,
+		.payload_in = &get_lsa,
+		.size_in = sizeof(get_lsa),
+		.size_out = cmd->in_length,
+		.payload_out = cmd->out_buf,
+	};
 
-	rc = cxl_mbox_send_cmd(cxlds, CXL_MBOX_OP_GET_LSA, &get_lsa,
-			       sizeof(get_lsa), cmd->out_buf, cmd->in_length);
+	rc = cxl_internal_send_cmd(cxlds, &mbox_cmd);
 	cmd->status = 0;
 
 	return rc;
@@ -143,6 +150,7 @@ static int cxl_pmem_set_config_data(struct cxl_dev_state *cxlds,
 				    unsigned int buf_len)
 {
 	struct cxl_mbox_set_lsa *set_lsa;
+	struct cxl_mbox_cmd mbox_cmd;
 	int rc;
 
 	if (sizeof(*cmd) > buf_len)
@@ -161,10 +169,13 @@ static int cxl_pmem_set_config_data(struct cxl_dev_state *cxlds,
 		.offset = cpu_to_le32(cmd->in_offset),
 	};
 	memcpy(set_lsa->data, cmd->in_buf, cmd->in_length);
+	mbox_cmd = (struct cxl_mbox_cmd) {
+		.opcode = CXL_MBOX_OP_SET_LSA,
+		.payload_in = set_lsa,
+		.size_in = struct_size(set_lsa, data, cmd->in_length),
+	};
 
-	rc = cxl_mbox_send_cmd(cxlds, CXL_MBOX_OP_SET_LSA, set_lsa,
-			       struct_size(set_lsa, data, cmd->in_length),
-			       NULL, 0);
+	rc = cxl_internal_send_cmd(cxlds, &mbox_cmd);
 
 	/*
 	 * Set "firmware" status (4-packed bytes at the end of the input
diff --git a/drivers/cxl/security.c b/drivers/cxl/security.c
index ebb78b8944f5..4ad4bda2d18e 100644
--- a/drivers/cxl/security.c
+++ b/drivers/cxl/security.c
@@ -19,11 +19,17 @@ static unsigned long cxl_pmem_get_security_flags(struct nvdimm *nvdimm,
 	struct cxl_get_security_output {
 		__le32 flags;
 	} out;
+	struct cxl_mbox_cmd mbox_cmd;
 	u32 sec_out;
 	int rc;
 
-	rc = cxl_mbox_send_cmd(cxlds, CXL_MBOX_OP_GET_SECURITY_STATE, NULL, 0,
-			       &out, sizeof(out));
+	mbox_cmd = (struct cxl_mbox_cmd) {
+		.opcode = CXL_MBOX_OP_GET_SECURITY_STATE,
+		.size_out = sizeof(out),
+		.payload_out = &out,
+	};
+
+	rc = cxl_internal_send_cmd(cxlds, &mbox_cmd);
 	if (rc < 0)
 		return 0;
 
@@ -62,17 +68,23 @@ static int cxl_pmem_security_change_key(struct nvdimm *nvdimm,
 	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_mbox_cmd mbox_cmd;
 	struct cxl_set_pass set_pass;
-	int rc;
 
-	set_pass.type = ptype == NVDIMM_MASTER ?
-		CXL_PMEM_SEC_PASS_MASTER : CXL_PMEM_SEC_PASS_USER;
+	set_pass = (struct cxl_set_pass) {
+		.type = ptype == NVDIMM_MASTER ? CXL_PMEM_SEC_PASS_MASTER :
+						 CXL_PMEM_SEC_PASS_USER,
+	};
 	memcpy(set_pass.old_pass, old_data->data, NVDIMM_PASSPHRASE_LEN);
 	memcpy(set_pass.new_pass, new_data->data, NVDIMM_PASSPHRASE_LEN);
 
-	rc = cxl_mbox_send_cmd(cxlds, CXL_MBOX_OP_SET_PASSPHRASE,
-			       &set_pass, sizeof(set_pass), NULL, 0);
-	return rc;
+	mbox_cmd = (struct cxl_mbox_cmd) {
+		.opcode = CXL_MBOX_OP_SET_PASSPHRASE,
+		.size_in = sizeof(set_pass),
+		.payload_in = &set_pass,
+	};
+
+	return cxl_internal_send_cmd(cxlds, &mbox_cmd);
 }
 
 static int __cxl_pmem_security_disable(struct nvdimm *nvdimm,
@@ -83,15 +95,21 @@ static int __cxl_pmem_security_disable(struct nvdimm *nvdimm,
 	struct cxl_memdev *cxlmd = cxl_nvd->cxlmd;
 	struct cxl_dev_state *cxlds = cxlmd->cxlds;
 	struct cxl_disable_pass dis_pass;
-	int rc;
+	struct cxl_mbox_cmd mbox_cmd;
 
-	dis_pass.type = ptype == NVDIMM_MASTER ?
-		CXL_PMEM_SEC_PASS_MASTER : CXL_PMEM_SEC_PASS_USER;
+	dis_pass = (struct cxl_disable_pass) {
+		.type = ptype == NVDIMM_MASTER ? CXL_PMEM_SEC_PASS_MASTER :
+						 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);
-	return rc;
+	mbox_cmd = (struct cxl_mbox_cmd) {
+		.opcode = CXL_MBOX_OP_DISABLE_PASSPHRASE,
+		.size_in = sizeof(dis_pass),
+		.payload_in = &dis_pass,
+	};
+
+	return cxl_internal_send_cmd(cxlds, &mbox_cmd);
 }
 
 static int cxl_pmem_security_disable(struct nvdimm *nvdimm,
@@ -111,8 +129,11 @@ static int cxl_pmem_security_freeze(struct nvdimm *nvdimm)
 	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_mbox_cmd mbox_cmd = {
+		.opcode = CXL_MBOX_OP_FREEZE_SECURITY,
+	};
 
-	return cxl_mbox_send_cmd(cxlds, CXL_MBOX_OP_FREEZE_SECURITY, NULL, 0, NULL, 0);
+	return cxl_internal_send_cmd(cxlds, &mbox_cmd);
 }
 
 static int cxl_pmem_security_unlock(struct nvdimm *nvdimm,
@@ -122,11 +143,17 @@ static int cxl_pmem_security_unlock(struct nvdimm *nvdimm,
 	struct cxl_memdev *cxlmd = cxl_nvd->cxlmd;
 	struct cxl_dev_state *cxlds = cxlmd->cxlds;
 	u8 pass[NVDIMM_PASSPHRASE_LEN];
+	struct cxl_mbox_cmd mbox_cmd;
 	int rc;
 
 	memcpy(pass, key_data->data, NVDIMM_PASSPHRASE_LEN);
-	rc = cxl_mbox_send_cmd(cxlds, CXL_MBOX_OP_UNLOCK,
-			       pass, NVDIMM_PASSPHRASE_LEN, NULL, 0);
+	mbox_cmd = (struct cxl_mbox_cmd) {
+		.opcode = CXL_MBOX_OP_UNLOCK,
+		.size_in = NVDIMM_PASSPHRASE_LEN,
+		.payload_in = pass,
+	};
+
+	rc = cxl_internal_send_cmd(cxlds, &mbox_cmd);
 	if (rc < 0)
 		return rc;
 
@@ -140,14 +167,22 @@ static int cxl_pmem_security_passphrase_erase(struct nvdimm *nvdimm,
 	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_mbox_cmd mbox_cmd;
 	struct cxl_pass_erase erase;
 	int rc;
 
-	erase.type = ptype == NVDIMM_MASTER ?
-		CXL_PMEM_SEC_PASS_MASTER : CXL_PMEM_SEC_PASS_USER;
+	erase = (struct cxl_pass_erase) {
+		.type = ptype == NVDIMM_MASTER ? CXL_PMEM_SEC_PASS_MASTER :
+						 CXL_PMEM_SEC_PASS_USER,
+	};
 	memcpy(erase.pass, key->data, NVDIMM_PASSPHRASE_LEN);
-	rc = cxl_mbox_send_cmd(cxlds, CXL_MBOX_OP_PASSPHRASE_SECURE_ERASE,
-			       &erase, sizeof(erase), NULL, 0);
+	mbox_cmd = (struct cxl_mbox_cmd) {
+		.opcode = CXL_MBOX_OP_PASSPHRASE_SECURE_ERASE,
+		.size_in = sizeof(erase),
+		.payload_in = &erase,
+	};
+
+	rc = cxl_internal_send_cmd(cxlds, &mbox_cmd);
 	if (rc < 0)
 		return rc;
 

Re: [PATCH 2/4] cxl/mbox: Enable cxl_mbox_send_cmd() users to validate output size

[Ira Weiny]

On Mon, Dec 05, 2022 at 08:22:33PM -0800, Dan Williams wrote:
> Internally cxl_mbox_send_cmd() converts all passed-in parameters to a
> 'struct cxl_mbox_cmd' instance and sends that to cxlds->mbox_send(). It
> then teases the possibilty that the caller can validate the output size.
> However, they cannot since the resulting output size is not conveyed to
> the called. Fix that by making the caller pass in a constructed 'struct
      ^^^^^^
      caller

> cxl_mbox_cmd'. This prepares for a future patch to add output size
> validation on a per-command basis.
> 
> Given the change in signature, also change the name to differentiate it
> from the user command submission path that performs more validation
> before generating the 'struct cxl_mbox_cmd' instance to execute.
> 

Minor comments below.

Reviewed-by: Ira Weiny <ira.weiny@intel.com>

> Signed-off-by: Dan Williams <dan.j.williams@intel.com>
> ---
>  drivers/cxl/core/mbox.c |   86 ++++++++++++++++++++++++++++-------------------
>  drivers/cxl/cxlmem.h    |    4 +-
>  drivers/cxl/pmem.c      |   21 +++++++++--
>  drivers/cxl/security.c  |   77 +++++++++++++++++++++++++++++++-----------
>  4 files changed, 126 insertions(+), 62 deletions(-)
> 
> diff --git a/drivers/cxl/core/mbox.c b/drivers/cxl/core/mbox.c
> index 35dd889f1d3a..ed451ca60ce5 100644
> --- a/drivers/cxl/core/mbox.c
> +++ b/drivers/cxl/core/mbox.c
> @@ -146,13 +146,9 @@ static const char *cxl_mem_opcode_to_name(u16 opcode)
>  }
>  
>  /**
> - * cxl_mbox_send_cmd() - Send a mailbox command to a device.
> + * cxl_internal_send_cmd() - Kernel internal interface to send a mailbox command
>   * @cxlds: The device data for the operation
> - * @opcode: Opcode for the mailbox command.
> - * @in: The input payload for the mailbox command.
> - * @in_size: The length of the input payload
> - * @out: Caller allocated buffer for the output.
> - * @out_size: Expected size of output.
> + * @mbox_cmd: initialized command to execute
>   *
>   * Context: Any context.
>   * Return:
> @@ -167,40 +163,37 @@ static const char *cxl_mem_opcode_to_name(u16 opcode)
>   * error. While this distinction can be useful for commands from userspace, the
>   * kernel will only be able to use results when both are successful.
>   */
> -int cxl_mbox_send_cmd(struct cxl_dev_state *cxlds, u16 opcode, void *in,
> -		      size_t in_size, void *out, size_t out_size)
> +int cxl_internal_send_cmd(struct cxl_dev_state *cxlds,
> +			  struct cxl_mbox_cmd *mbox_cmd)
>  {
> -	const struct cxl_mem_command *cmd = cxl_mem_find_command(opcode);
> -	struct cxl_mbox_cmd mbox_cmd = {
> -		.opcode = opcode,
> -		.payload_in = in,
> -		.size_in = in_size,
> -		.size_out = out_size,
> -		.payload_out = out,
> -	};
> +	const struct cxl_mem_command *cmd =
> +		cxl_mem_find_command(mbox_cmd->opcode);
> +	size_t out_size;
>  	int rc;
>  
> -	if (in_size > cxlds->payload_size || out_size > cxlds->payload_size)
> +	if (mbox_cmd->size_in > cxlds->payload_size ||
> +	    mbox_cmd->size_out > cxlds->payload_size)
>  		return -E2BIG;
>  
> -	rc = cxlds->mbox_send(cxlds, &mbox_cmd);
> +	out_size = mbox_cmd->size_out;
> +	rc = cxlds->mbox_send(cxlds, mbox_cmd);
>  	if (rc)
>  		return rc;
>  
> -	if (mbox_cmd.return_code != CXL_MBOX_CMD_RC_SUCCESS)
> -		return cxl_mbox_cmd_rc2errno(&mbox_cmd);
> +	if (mbox_cmd->return_code != CXL_MBOX_CMD_RC_SUCCESS)
> +		return cxl_mbox_cmd_rc2errno(mbox_cmd);
>  
>  	/*
>  	 * Variable sized commands can't be validated and so it's up to the
>  	 * caller to do that if they wish.
>  	 */
>  	if (cmd->info.size_out != CXL_VARIABLE_PAYLOAD) {
> -		if (mbox_cmd.size_out != out_size)
> +		if (mbox_cmd->size_out != out_size)
>  			return -EIO;
>  	}
>  	return 0;
>  }
> -EXPORT_SYMBOL_NS_GPL(cxl_mbox_send_cmd, CXL);
> +EXPORT_SYMBOL_NS_GPL(cxl_internal_send_cmd, CXL);
>  
>  static bool cxl_mem_raw_command_allowed(u16 opcode)
>  {
> @@ -567,15 +560,25 @@ static int cxl_xfer_log(struct cxl_dev_state *cxlds, uuid_t *uuid, u32 size, u8
>  
>  	while (remaining) {
>  		u32 xfer_size = min_t(u32, remaining, cxlds->payload_size);
> -		struct cxl_mbox_get_log log = {
> +		struct cxl_mbox_cmd mbox_cmd;
> +		struct cxl_mbox_get_log log;
> +		int rc;
> +
> +		log = (struct cxl_mbox_get_log) {
>  			.uuid = *uuid,
>  			.offset = cpu_to_le32(offset),
> -			.length = cpu_to_le32(xfer_size)
> +			.length = cpu_to_le32(xfer_size),
> +		};
> +
> +		mbox_cmd = (struct cxl_mbox_cmd) {
> +			.opcode = CXL_MBOX_OP_GET_LOG,
> +			.size_in = sizeof(log),
> +			.payload_in = &log,
> +			.size_out = xfer_size,
> +			.payload_out = out,
>  		};
> -		int rc;
>  
> -		rc = cxl_mbox_send_cmd(cxlds, CXL_MBOX_OP_GET_LOG, &log, sizeof(log),
> -				       out, xfer_size);
> +		rc = cxl_internal_send_cmd(cxlds, &mbox_cmd);
>  		if (rc < 0)
>  			return rc;
>  
> @@ -621,19 +624,25 @@ static void cxl_walk_cel(struct cxl_dev_state *cxlds, size_t size, u8 *cel)
>  static struct cxl_mbox_get_supported_logs *cxl_get_gsl(struct cxl_dev_state *cxlds)
>  {
>  	struct cxl_mbox_get_supported_logs *ret;
> +	struct cxl_mbox_cmd mbox_cmd;
>  	int rc;
>  
>  	ret = kvmalloc(cxlds->payload_size, GFP_KERNEL);
>  	if (!ret)
>  		return ERR_PTR(-ENOMEM);
>  
> -	rc = cxl_mbox_send_cmd(cxlds, CXL_MBOX_OP_GET_SUPPORTED_LOGS, NULL, 0, ret,
> -			       cxlds->payload_size);
> +	mbox_cmd = (struct cxl_mbox_cmd) {
> +		.opcode = CXL_MBOX_OP_GET_SUPPORTED_LOGS,
> +		.size_out = cxlds->payload_size,
> +		.payload_out = ret,
> +	};
> +	rc = cxl_internal_send_cmd(cxlds, &mbox_cmd);
>  	if (rc < 0) {
>  		kvfree(ret);
>  		return ERR_PTR(rc);
>  	}
>  
> +

extra...

>  	return ret;
>  }
>  
> @@ -735,11 +744,15 @@ EXPORT_SYMBOL_NS_GPL(cxl_enumerate_cmds, CXL);
>  static int cxl_mem_get_partition_info(struct cxl_dev_state *cxlds)
>  {
>  	struct cxl_mbox_get_partition_info pi;
> +	struct cxl_mbox_cmd mbox_cmd;
>  	int rc;
>  
> -	rc = cxl_mbox_send_cmd(cxlds, CXL_MBOX_OP_GET_PARTITION_INFO, NULL, 0,
> -			       &pi, sizeof(pi));
> -
> +	mbox_cmd = (struct cxl_mbox_cmd) {
> +		.opcode = CXL_MBOX_OP_GET_PARTITION_INFO,
> +		.size_out = sizeof(pi),
> +		.payload_out = &pi,
> +	};
> +	rc = cxl_internal_send_cmd(cxlds, &mbox_cmd);
>  	if (rc)
>  		return rc;
>  
> @@ -768,10 +781,15 @@ int cxl_dev_state_identify(struct cxl_dev_state *cxlds)
>  {
>  	/* See CXL 2.0 Table 175 Identify Memory Device Output Payload */
>  	struct cxl_mbox_identify id;
> +	struct cxl_mbox_cmd mbox_cmd;
>  	int rc;
>  
> -	rc = cxl_mbox_send_cmd(cxlds, CXL_MBOX_OP_IDENTIFY, NULL, 0, &id,
> -			       sizeof(id));
> +	mbox_cmd = (struct cxl_mbox_cmd) {
> +		.opcode = CXL_MBOX_OP_IDENTIFY,
> +		.size_out = sizeof(id),
> +		.payload_out = &id,
> +	};
> +	rc = cxl_internal_send_cmd(cxlds, &mbox_cmd);
>  	if (rc < 0)
>  		return rc;
>  
> diff --git a/drivers/cxl/cxlmem.h b/drivers/cxl/cxlmem.h
> index 785c6c12515d..c447577f5ad5 100644
> --- a/drivers/cxl/cxlmem.h
> +++ b/drivers/cxl/cxlmem.h
> @@ -430,8 +430,8 @@ enum {
>  	CXL_PMEM_SEC_PASS_USER,
>  };
>  
> -int cxl_mbox_send_cmd(struct cxl_dev_state *cxlds, u16 opcode, void *in,
> -		      size_t in_size, void *out, size_t out_size);
> +int cxl_internal_send_cmd(struct cxl_dev_state *cxlds,
> +			  struct cxl_mbox_cmd *cmd);
>  int cxl_dev_state_identify(struct cxl_dev_state *cxlds);
>  int cxl_await_media_ready(struct cxl_dev_state *cxlds);
>  int cxl_enumerate_cmds(struct cxl_dev_state *cxlds);
> diff --git a/drivers/cxl/pmem.c b/drivers/cxl/pmem.c
> index 2fc8070b6a17..eedefebc4283 100644
> --- a/drivers/cxl/pmem.c
> +++ b/drivers/cxl/pmem.c
> @@ -119,6 +119,7 @@ static int cxl_pmem_get_config_data(struct cxl_dev_state *cxlds,
>  				    unsigned int buf_len)
>  {
>  	struct cxl_mbox_get_lsa get_lsa;
> +	struct cxl_mbox_cmd mbox_cmd;
>  	int rc;
>  
>  	if (sizeof(*cmd) > buf_len)
> @@ -130,9 +131,15 @@ static int cxl_pmem_get_config_data(struct cxl_dev_state *cxlds,
>  		.offset = cpu_to_le32(cmd->in_offset),
>  		.length = cpu_to_le32(cmd->in_length),
>  	};
> +	mbox_cmd = (struct cxl_mbox_cmd) {
> +		.opcode = CXL_MBOX_OP_GET_LSA,
> +		.payload_in = &get_lsa,
> +		.size_in = sizeof(get_lsa),
> +		.size_out = cmd->in_length,
> +		.payload_out = cmd->out_buf,

FWIW NIT: I find it harder to review when the initializers are out of order WRT
the struct definition.  ie should be in, in_size, out, out_size for each of
these.

> +	};
>  
> -	rc = cxl_mbox_send_cmd(cxlds, CXL_MBOX_OP_GET_LSA, &get_lsa,
> -			       sizeof(get_lsa), cmd->out_buf, cmd->in_length);
> +	rc = cxl_internal_send_cmd(cxlds, &mbox_cmd);
>  	cmd->status = 0;
>  
>  	return rc;
> @@ -143,6 +150,7 @@ static int cxl_pmem_set_config_data(struct cxl_dev_state *cxlds,
>  				    unsigned int buf_len)
>  {
>  	struct cxl_mbox_set_lsa *set_lsa;
> +	struct cxl_mbox_cmd mbox_cmd;
>  	int rc;
>  
>  	if (sizeof(*cmd) > buf_len)
> @@ -161,10 +169,13 @@ static int cxl_pmem_set_config_data(struct cxl_dev_state *cxlds,
>  		.offset = cpu_to_le32(cmd->in_offset),
>  	};
>  	memcpy(set_lsa->data, cmd->in_buf, cmd->in_length);
> +	mbox_cmd = (struct cxl_mbox_cmd) {
> +		.opcode = CXL_MBOX_OP_SET_LSA,
> +		.payload_in = set_lsa,
> +		.size_in = struct_size(set_lsa, data, cmd->in_length),
> +	};
>  
> -	rc = cxl_mbox_send_cmd(cxlds, CXL_MBOX_OP_SET_LSA, set_lsa,
> -			       struct_size(set_lsa, data, cmd->in_length),
> -			       NULL, 0);
> +	rc = cxl_internal_send_cmd(cxlds, &mbox_cmd);
>  
>  	/*
>  	 * Set "firmware" status (4-packed bytes at the end of the input
> diff --git a/drivers/cxl/security.c b/drivers/cxl/security.c
> index ebb78b8944f5..4ad4bda2d18e 100644
> --- a/drivers/cxl/security.c
> +++ b/drivers/cxl/security.c
> @@ -19,11 +19,17 @@ static unsigned long cxl_pmem_get_security_flags(struct nvdimm *nvdimm,
>  	struct cxl_get_security_output {
>  		__le32 flags;
>  	} out;
> +	struct cxl_mbox_cmd mbox_cmd;
>  	u32 sec_out;
>  	int rc;
>  
> -	rc = cxl_mbox_send_cmd(cxlds, CXL_MBOX_OP_GET_SECURITY_STATE, NULL, 0,
> -			       &out, sizeof(out));
> +	mbox_cmd = (struct cxl_mbox_cmd) {
> +		.opcode = CXL_MBOX_OP_GET_SECURITY_STATE,
> +		.size_out = sizeof(out),
> +		.payload_out = &out,
> +	};
> +
> +	rc = cxl_internal_send_cmd(cxlds, &mbox_cmd);
>  	if (rc < 0)
>  		return 0;
>  
> @@ -62,17 +68,23 @@ static int cxl_pmem_security_change_key(struct nvdimm *nvdimm,
>  	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_mbox_cmd mbox_cmd;
>  	struct cxl_set_pass set_pass;
> -	int rc;
>  
> -	set_pass.type = ptype == NVDIMM_MASTER ?
> -		CXL_PMEM_SEC_PASS_MASTER : CXL_PMEM_SEC_PASS_USER;
> +	set_pass = (struct cxl_set_pass) {
> +		.type = ptype == NVDIMM_MASTER ? CXL_PMEM_SEC_PASS_MASTER :
> +						 CXL_PMEM_SEC_PASS_USER,
> +	};

Extra change?  But ok.

>  	memcpy(set_pass.old_pass, old_data->data, NVDIMM_PASSPHRASE_LEN);
>  	memcpy(set_pass.new_pass, new_data->data, NVDIMM_PASSPHRASE_LEN);
>  
> -	rc = cxl_mbox_send_cmd(cxlds, CXL_MBOX_OP_SET_PASSPHRASE,
> -			       &set_pass, sizeof(set_pass), NULL, 0);
> -	return rc;
> +	mbox_cmd = (struct cxl_mbox_cmd) {
> +		.opcode = CXL_MBOX_OP_SET_PASSPHRASE,
> +		.size_in = sizeof(set_pass),
> +		.payload_in = &set_pass,
> +	};
> +
> +	return cxl_internal_send_cmd(cxlds, &mbox_cmd);
>  }
>  
>  static int __cxl_pmem_security_disable(struct nvdimm *nvdimm,
> @@ -83,15 +95,21 @@ static int __cxl_pmem_security_disable(struct nvdimm *nvdimm,
>  	struct cxl_memdev *cxlmd = cxl_nvd->cxlmd;
>  	struct cxl_dev_state *cxlds = cxlmd->cxlds;
>  	struct cxl_disable_pass dis_pass;
> -	int rc;
> +	struct cxl_mbox_cmd mbox_cmd;
>  
> -	dis_pass.type = ptype == NVDIMM_MASTER ?
> -		CXL_PMEM_SEC_PASS_MASTER : CXL_PMEM_SEC_PASS_USER;
> +	dis_pass = (struct cxl_disable_pass) {
> +		.type = ptype == NVDIMM_MASTER ? CXL_PMEM_SEC_PASS_MASTER :
> +						 CXL_PMEM_SEC_PASS_USER,
> +	};

ditto

Ira

>  	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);
> -	return rc;
> +	mbox_cmd = (struct cxl_mbox_cmd) {
> +		.opcode = CXL_MBOX_OP_DISABLE_PASSPHRASE,
> +		.size_in = sizeof(dis_pass),
> +		.payload_in = &dis_pass,
> +	};
> +
> +	return cxl_internal_send_cmd(cxlds, &mbox_cmd);
>  }
>  
>  static int cxl_pmem_security_disable(struct nvdimm *nvdimm,
> @@ -111,8 +129,11 @@ static int cxl_pmem_security_freeze(struct nvdimm *nvdimm)
>  	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_mbox_cmd mbox_cmd = {
> +		.opcode = CXL_MBOX_OP_FREEZE_SECURITY,
> +	};
>  
> -	return cxl_mbox_send_cmd(cxlds, CXL_MBOX_OP_FREEZE_SECURITY, NULL, 0, NULL, 0);
> +	return cxl_internal_send_cmd(cxlds, &mbox_cmd);
>  }
>  
>  static int cxl_pmem_security_unlock(struct nvdimm *nvdimm,
> @@ -122,11 +143,17 @@ static int cxl_pmem_security_unlock(struct nvdimm *nvdimm,
>  	struct cxl_memdev *cxlmd = cxl_nvd->cxlmd;
>  	struct cxl_dev_state *cxlds = cxlmd->cxlds;
>  	u8 pass[NVDIMM_PASSPHRASE_LEN];
> +	struct cxl_mbox_cmd mbox_cmd;
>  	int rc;
>  
>  	memcpy(pass, key_data->data, NVDIMM_PASSPHRASE_LEN);
> -	rc = cxl_mbox_send_cmd(cxlds, CXL_MBOX_OP_UNLOCK,
> -			       pass, NVDIMM_PASSPHRASE_LEN, NULL, 0);
> +	mbox_cmd = (struct cxl_mbox_cmd) {
> +		.opcode = CXL_MBOX_OP_UNLOCK,
> +		.size_in = NVDIMM_PASSPHRASE_LEN,
> +		.payload_in = pass,
> +	};
> +
> +	rc = cxl_internal_send_cmd(cxlds, &mbox_cmd);
>  	if (rc < 0)
>  		return rc;
>  
> @@ -140,14 +167,22 @@ static int cxl_pmem_security_passphrase_erase(struct nvdimm *nvdimm,
>  	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_mbox_cmd mbox_cmd;
>  	struct cxl_pass_erase erase;
>  	int rc;
>  
> -	erase.type = ptype == NVDIMM_MASTER ?
> -		CXL_PMEM_SEC_PASS_MASTER : CXL_PMEM_SEC_PASS_USER;
> +	erase = (struct cxl_pass_erase) {
> +		.type = ptype == NVDIMM_MASTER ? CXL_PMEM_SEC_PASS_MASTER :
> +						 CXL_PMEM_SEC_PASS_USER,
> +	};
>  	memcpy(erase.pass, key->data, NVDIMM_PASSPHRASE_LEN);
> -	rc = cxl_mbox_send_cmd(cxlds, CXL_MBOX_OP_PASSPHRASE_SECURE_ERASE,
> -			       &erase, sizeof(erase), NULL, 0);
> +	mbox_cmd = (struct cxl_mbox_cmd) {
> +		.opcode = CXL_MBOX_OP_PASSPHRASE_SECURE_ERASE,
> +		.size_in = sizeof(erase),
> +		.payload_in = &erase,
> +	};
> +
> +	rc = cxl_internal_send_cmd(cxlds, &mbox_cmd);
>  	if (rc < 0)
>  		return rc;
>  
> 

Re: [PATCH 2/4] cxl/mbox: Enable cxl_mbox_send_cmd() users to validate output size

[Dave Jiang]

On 12/5/2022 9:22 PM, Dan Williams wrote:
> Internally cxl_mbox_send_cmd() converts all passed-in parameters to a
> 'struct cxl_mbox_cmd' instance and sends that to cxlds->mbox_send(). It
> then teases the possibilty that the caller can validate the output size.
> However, they cannot since the resulting output size is not conveyed to
> the called. Fix that by making the caller pass in a constructed 'struct
> cxl_mbox_cmd'. This prepares for a future patch to add output size
> validation on a per-command basis.
> 
> Given the change in signature, also change the name to differentiate it
> from the user command submission path that performs more validation
> before generating the 'struct cxl_mbox_cmd' instance to execute.
> 
> Signed-off-by: Dan Williams <dan.j.williams@intel.com>

Reviewed-by: Dave Jiang <dave.jiang@intel.com>

Just minor nit below.

> ---
>   drivers/cxl/core/mbox.c |   86 ++++++++++++++++++++++++++++-------------------
>   drivers/cxl/cxlmem.h    |    4 +-
>   drivers/cxl/pmem.c      |   21 +++++++++--
>   drivers/cxl/security.c  |   77 +++++++++++++++++++++++++++++++-----------
>   4 files changed, 126 insertions(+), 62 deletions(-)
> 
> diff --git a/drivers/cxl/core/mbox.c b/drivers/cxl/core/mbox.c
> index 35dd889f1d3a..ed451ca60ce5 100644
> --- a/drivers/cxl/core/mbox.c
> +++ b/drivers/cxl/core/mbox.c
> @@ -146,13 +146,9 @@ static const char *cxl_mem_opcode_to_name(u16 opcode)
>   }
>   
>   /**
> - * cxl_mbox_send_cmd() - Send a mailbox command to a device.
> + * cxl_internal_send_cmd() - Kernel internal interface to send a mailbox command
>    * @cxlds: The device data for the operation
> - * @opcode: Opcode for the mailbox command.
> - * @in: The input payload for the mailbox command.
> - * @in_size: The length of the input payload
> - * @out: Caller allocated buffer for the output.
> - * @out_size: Expected size of output.
> + * @mbox_cmd: initialized command to execute
>    *
>    * Context: Any context.
>    * Return:
> @@ -167,40 +163,37 @@ static const char *cxl_mem_opcode_to_name(u16 opcode)
>    * error. While this distinction can be useful for commands from userspace, the
>    * kernel will only be able to use results when both are successful.
>    */
> -int cxl_mbox_send_cmd(struct cxl_dev_state *cxlds, u16 opcode, void *in,
> -		      size_t in_size, void *out, size_t out_size)
> +int cxl_internal_send_cmd(struct cxl_dev_state *cxlds,
> +			  struct cxl_mbox_cmd *mbox_cmd)
>   {
> -	const struct cxl_mem_command *cmd = cxl_mem_find_command(opcode);
> -	struct cxl_mbox_cmd mbox_cmd = {
> -		.opcode = opcode,
> -		.payload_in = in,
> -		.size_in = in_size,
> -		.size_out = out_size,
> -		.payload_out = out,
> -	};
> +	const struct cxl_mem_command *cmd =
> +		cxl_mem_find_command(mbox_cmd->opcode);

Less line change if newline is not introduced here?

DJ

> +	size_t out_size;
>   	int rc;
>   
> -	if (in_size > cxlds->payload_size || out_size > cxlds->payload_size)
> +	if (mbox_cmd->size_in > cxlds->payload_size ||
> +	    mbox_cmd->size_out > cxlds->payload_size)
>   		return -E2BIG;
>   
> -	rc = cxlds->mbox_send(cxlds, &mbox_cmd);
> +	out_size = mbox_cmd->size_out;
> +	rc = cxlds->mbox_send(cxlds, mbox_cmd);
>   	if (rc)
>   		return rc;
>   
> -	if (mbox_cmd.return_code != CXL_MBOX_CMD_RC_SUCCESS)
> -		return cxl_mbox_cmd_rc2errno(&mbox_cmd);
> +	if (mbox_cmd->return_code != CXL_MBOX_CMD_RC_SUCCESS)
> +		return cxl_mbox_cmd_rc2errno(mbox_cmd);
>   
>   	/*
>   	 * Variable sized commands can't be validated and so it's up to the
>   	 * caller to do that if they wish.
>   	 */
>   	if (cmd->info.size_out != CXL_VARIABLE_PAYLOAD) {
> -		if (mbox_cmd.size_out != out_size)
> +		if (mbox_cmd->size_out != out_size)
>   			return -EIO;
>   	}
>   	return 0;
>   }
> -EXPORT_SYMBOL_NS_GPL(cxl_mbox_send_cmd, CXL);
> +EXPORT_SYMBOL_NS_GPL(cxl_internal_send_cmd, CXL);
>   
>   static bool cxl_mem_raw_command_allowed(u16 opcode)
>   {
> @@ -567,15 +560,25 @@ static int cxl_xfer_log(struct cxl_dev_state *cxlds, uuid_t *uuid, u32 size, u8
>   
>   	while (remaining) {
>   		u32 xfer_size = min_t(u32, remaining, cxlds->payload_size);
> -		struct cxl_mbox_get_log log = {
> +		struct cxl_mbox_cmd mbox_cmd;
> +		struct cxl_mbox_get_log log;
> +		int rc;
> +
> +		log = (struct cxl_mbox_get_log) {
>   			.uuid = *uuid,
>   			.offset = cpu_to_le32(offset),
> -			.length = cpu_to_le32(xfer_size)
> +			.length = cpu_to_le32(xfer_size),
> +		};
> +
> +		mbox_cmd = (struct cxl_mbox_cmd) {
> +			.opcode = CXL_MBOX_OP_GET_LOG,
> +			.size_in = sizeof(log),
> +			.payload_in = &log,
> +			.size_out = xfer_size,
> +			.payload_out = out,
>   		};
> -		int rc;
>   
> -		rc = cxl_mbox_send_cmd(cxlds, CXL_MBOX_OP_GET_LOG, &log, sizeof(log),
> -				       out, xfer_size);
> +		rc = cxl_internal_send_cmd(cxlds, &mbox_cmd);
>   		if (rc < 0)
>   			return rc;
>   
> @@ -621,19 +624,25 @@ static void cxl_walk_cel(struct cxl_dev_state *cxlds, size_t size, u8 *cel)
>   static struct cxl_mbox_get_supported_logs *cxl_get_gsl(struct cxl_dev_state *cxlds)
>   {
>   	struct cxl_mbox_get_supported_logs *ret;
> +	struct cxl_mbox_cmd mbox_cmd;
>   	int rc;
>   
>   	ret = kvmalloc(cxlds->payload_size, GFP_KERNEL);
>   	if (!ret)
>   		return ERR_PTR(-ENOMEM);
>   
> -	rc = cxl_mbox_send_cmd(cxlds, CXL_MBOX_OP_GET_SUPPORTED_LOGS, NULL, 0, ret,
> -			       cxlds->payload_size);
> +	mbox_cmd = (struct cxl_mbox_cmd) {
> +		.opcode = CXL_MBOX_OP_GET_SUPPORTED_LOGS,
> +		.size_out = cxlds->payload_size,
> +		.payload_out = ret,
> +	};
> +	rc = cxl_internal_send_cmd(cxlds, &mbox_cmd);
>   	if (rc < 0) {
>   		kvfree(ret);
>   		return ERR_PTR(rc);
>   	}
>   
> +
>   	return ret;
>   }
>   
> @@ -735,11 +744,15 @@ EXPORT_SYMBOL_NS_GPL(cxl_enumerate_cmds, CXL);
>   static int cxl_mem_get_partition_info(struct cxl_dev_state *cxlds)
>   {
>   	struct cxl_mbox_get_partition_info pi;
> +	struct cxl_mbox_cmd mbox_cmd;
>   	int rc;
>   
> -	rc = cxl_mbox_send_cmd(cxlds, CXL_MBOX_OP_GET_PARTITION_INFO, NULL, 0,
> -			       &pi, sizeof(pi));
> -
> +	mbox_cmd = (struct cxl_mbox_cmd) {
> +		.opcode = CXL_MBOX_OP_GET_PARTITION_INFO,
> +		.size_out = sizeof(pi),
> +		.payload_out = &pi,
> +	};
> +	rc = cxl_internal_send_cmd(cxlds, &mbox_cmd);
>   	if (rc)
>   		return rc;
>   
> @@ -768,10 +781,15 @@ int cxl_dev_state_identify(struct cxl_dev_state *cxlds)
>   {
>   	/* See CXL 2.0 Table 175 Identify Memory Device Output Payload */
>   	struct cxl_mbox_identify id;
> +	struct cxl_mbox_cmd mbox_cmd;
>   	int rc;
>   
> -	rc = cxl_mbox_send_cmd(cxlds, CXL_MBOX_OP_IDENTIFY, NULL, 0, &id,
> -			       sizeof(id));
> +	mbox_cmd = (struct cxl_mbox_cmd) {
> +		.opcode = CXL_MBOX_OP_IDENTIFY,
> +		.size_out = sizeof(id),
> +		.payload_out = &id,
> +	};
> +	rc = cxl_internal_send_cmd(cxlds, &mbox_cmd);
>   	if (rc < 0)
>   		return rc;
>   
> diff --git a/drivers/cxl/cxlmem.h b/drivers/cxl/cxlmem.h
> index 785c6c12515d..c447577f5ad5 100644
> --- a/drivers/cxl/cxlmem.h
> +++ b/drivers/cxl/cxlmem.h
> @@ -430,8 +430,8 @@ enum {
>   	CXL_PMEM_SEC_PASS_USER,
>   };
>   
> -int cxl_mbox_send_cmd(struct cxl_dev_state *cxlds, u16 opcode, void *in,
> -		      size_t in_size, void *out, size_t out_size);
> +int cxl_internal_send_cmd(struct cxl_dev_state *cxlds,
> +			  struct cxl_mbox_cmd *cmd);
>   int cxl_dev_state_identify(struct cxl_dev_state *cxlds);
>   int cxl_await_media_ready(struct cxl_dev_state *cxlds);
>   int cxl_enumerate_cmds(struct cxl_dev_state *cxlds);
> diff --git a/drivers/cxl/pmem.c b/drivers/cxl/pmem.c
> index 2fc8070b6a17..eedefebc4283 100644
> --- a/drivers/cxl/pmem.c
> +++ b/drivers/cxl/pmem.c
> @@ -119,6 +119,7 @@ static int cxl_pmem_get_config_data(struct cxl_dev_state *cxlds,
>   				    unsigned int buf_len)
>   {
>   	struct cxl_mbox_get_lsa get_lsa;
> +	struct cxl_mbox_cmd mbox_cmd;
>   	int rc;
>   
>   	if (sizeof(*cmd) > buf_len)
> @@ -130,9 +131,15 @@ static int cxl_pmem_get_config_data(struct cxl_dev_state *cxlds,
>   		.offset = cpu_to_le32(cmd->in_offset),
>   		.length = cpu_to_le32(cmd->in_length),
>   	};
> +	mbox_cmd = (struct cxl_mbox_cmd) {
> +		.opcode = CXL_MBOX_OP_GET_LSA,
> +		.payload_in = &get_lsa,
> +		.size_in = sizeof(get_lsa),
> +		.size_out = cmd->in_length,
> +		.payload_out = cmd->out_buf,
> +	};
>   
> -	rc = cxl_mbox_send_cmd(cxlds, CXL_MBOX_OP_GET_LSA, &get_lsa,
> -			       sizeof(get_lsa), cmd->out_buf, cmd->in_length);
> +	rc = cxl_internal_send_cmd(cxlds, &mbox_cmd);
>   	cmd->status = 0;
>   
>   	return rc;
> @@ -143,6 +150,7 @@ static int cxl_pmem_set_config_data(struct cxl_dev_state *cxlds,
>   				    unsigned int buf_len)
>   {
>   	struct cxl_mbox_set_lsa *set_lsa;
> +	struct cxl_mbox_cmd mbox_cmd;
>   	int rc;
>   
>   	if (sizeof(*cmd) > buf_len)
> @@ -161,10 +169,13 @@ static int cxl_pmem_set_config_data(struct cxl_dev_state *cxlds,
>   		.offset = cpu_to_le32(cmd->in_offset),
>   	};
>   	memcpy(set_lsa->data, cmd->in_buf, cmd->in_length);
> +	mbox_cmd = (struct cxl_mbox_cmd) {
> +		.opcode = CXL_MBOX_OP_SET_LSA,
> +		.payload_in = set_lsa,
> +		.size_in = struct_size(set_lsa, data, cmd->in_length),
> +	};
>   
> -	rc = cxl_mbox_send_cmd(cxlds, CXL_MBOX_OP_SET_LSA, set_lsa,
> -			       struct_size(set_lsa, data, cmd->in_length),
> -			       NULL, 0);
> +	rc = cxl_internal_send_cmd(cxlds, &mbox_cmd);
>   
>   	/*
>   	 * Set "firmware" status (4-packed bytes at the end of the input
> diff --git a/drivers/cxl/security.c b/drivers/cxl/security.c
> index ebb78b8944f5..4ad4bda2d18e 100644
> --- a/drivers/cxl/security.c
> +++ b/drivers/cxl/security.c
> @@ -19,11 +19,17 @@ static unsigned long cxl_pmem_get_security_flags(struct nvdimm *nvdimm,
>   	struct cxl_get_security_output {
>   		__le32 flags;
>   	} out;
> +	struct cxl_mbox_cmd mbox_cmd;
>   	u32 sec_out;
>   	int rc;
>   
> -	rc = cxl_mbox_send_cmd(cxlds, CXL_MBOX_OP_GET_SECURITY_STATE, NULL, 0,
> -			       &out, sizeof(out));
> +	mbox_cmd = (struct cxl_mbox_cmd) {
> +		.opcode = CXL_MBOX_OP_GET_SECURITY_STATE,
> +		.size_out = sizeof(out),
> +		.payload_out = &out,
> +	};
> +
> +	rc = cxl_internal_send_cmd(cxlds, &mbox_cmd);
>   	if (rc < 0)
>   		return 0;
>   
> @@ -62,17 +68,23 @@ static int cxl_pmem_security_change_key(struct nvdimm *nvdimm,
>   	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_mbox_cmd mbox_cmd;
>   	struct cxl_set_pass set_pass;
> -	int rc;
>   
> -	set_pass.type = ptype == NVDIMM_MASTER ?
> -		CXL_PMEM_SEC_PASS_MASTER : CXL_PMEM_SEC_PASS_USER;
> +	set_pass = (struct cxl_set_pass) {
> +		.type = ptype == NVDIMM_MASTER ? CXL_PMEM_SEC_PASS_MASTER :
> +						 CXL_PMEM_SEC_PASS_USER,
> +	};
>   	memcpy(set_pass.old_pass, old_data->data, NVDIMM_PASSPHRASE_LEN);
>   	memcpy(set_pass.new_pass, new_data->data, NVDIMM_PASSPHRASE_LEN);
>   
> -	rc = cxl_mbox_send_cmd(cxlds, CXL_MBOX_OP_SET_PASSPHRASE,
> -			       &set_pass, sizeof(set_pass), NULL, 0);
> -	return rc;
> +	mbox_cmd = (struct cxl_mbox_cmd) {
> +		.opcode = CXL_MBOX_OP_SET_PASSPHRASE,
> +		.size_in = sizeof(set_pass),
> +		.payload_in = &set_pass,
> +	};
> +
> +	return cxl_internal_send_cmd(cxlds, &mbox_cmd);
>   }
>   
>   static int __cxl_pmem_security_disable(struct nvdimm *nvdimm,
> @@ -83,15 +95,21 @@ static int __cxl_pmem_security_disable(struct nvdimm *nvdimm,
>   	struct cxl_memdev *cxlmd = cxl_nvd->cxlmd;
>   	struct cxl_dev_state *cxlds = cxlmd->cxlds;
>   	struct cxl_disable_pass dis_pass;
> -	int rc;
> +	struct cxl_mbox_cmd mbox_cmd;
>   
> -	dis_pass.type = ptype == NVDIMM_MASTER ?
> -		CXL_PMEM_SEC_PASS_MASTER : CXL_PMEM_SEC_PASS_USER;
> +	dis_pass = (struct cxl_disable_pass) {
> +		.type = ptype == NVDIMM_MASTER ? CXL_PMEM_SEC_PASS_MASTER :
> +						 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);
> -	return rc;
> +	mbox_cmd = (struct cxl_mbox_cmd) {
> +		.opcode = CXL_MBOX_OP_DISABLE_PASSPHRASE,
> +		.size_in = sizeof(dis_pass),
> +		.payload_in = &dis_pass,
> +	};
> +
> +	return cxl_internal_send_cmd(cxlds, &mbox_cmd);
>   }
>   
>   static int cxl_pmem_security_disable(struct nvdimm *nvdimm,
> @@ -111,8 +129,11 @@ static int cxl_pmem_security_freeze(struct nvdimm *nvdimm)
>   	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_mbox_cmd mbox_cmd = {
> +		.opcode = CXL_MBOX_OP_FREEZE_SECURITY,
> +	};
>   
> -	return cxl_mbox_send_cmd(cxlds, CXL_MBOX_OP_FREEZE_SECURITY, NULL, 0, NULL, 0);
> +	return cxl_internal_send_cmd(cxlds, &mbox_cmd);
>   }
>   
>   static int cxl_pmem_security_unlock(struct nvdimm *nvdimm,
> @@ -122,11 +143,17 @@ static int cxl_pmem_security_unlock(struct nvdimm *nvdimm,
>   	struct cxl_memdev *cxlmd = cxl_nvd->cxlmd;
>   	struct cxl_dev_state *cxlds = cxlmd->cxlds;
>   	u8 pass[NVDIMM_PASSPHRASE_LEN];
> +	struct cxl_mbox_cmd mbox_cmd;
>   	int rc;
>   
>   	memcpy(pass, key_data->data, NVDIMM_PASSPHRASE_LEN);
> -	rc = cxl_mbox_send_cmd(cxlds, CXL_MBOX_OP_UNLOCK,
> -			       pass, NVDIMM_PASSPHRASE_LEN, NULL, 0);
> +	mbox_cmd = (struct cxl_mbox_cmd) {
> +		.opcode = CXL_MBOX_OP_UNLOCK,
> +		.size_in = NVDIMM_PASSPHRASE_LEN,
> +		.payload_in = pass,
> +	};
> +
> +	rc = cxl_internal_send_cmd(cxlds, &mbox_cmd);
>   	if (rc < 0)
>   		return rc;
>   
> @@ -140,14 +167,22 @@ static int cxl_pmem_security_passphrase_erase(struct nvdimm *nvdimm,
>   	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_mbox_cmd mbox_cmd;
>   	struct cxl_pass_erase erase;
>   	int rc;
>   
> -	erase.type = ptype == NVDIMM_MASTER ?
> -		CXL_PMEM_SEC_PASS_MASTER : CXL_PMEM_SEC_PASS_USER;
> +	erase = (struct cxl_pass_erase) {
> +		.type = ptype == NVDIMM_MASTER ? CXL_PMEM_SEC_PASS_MASTER :
> +						 CXL_PMEM_SEC_PASS_USER,
> +	};
>   	memcpy(erase.pass, key->data, NVDIMM_PASSPHRASE_LEN);
> -	rc = cxl_mbox_send_cmd(cxlds, CXL_MBOX_OP_PASSPHRASE_SECURE_ERASE,
> -			       &erase, sizeof(erase), NULL, 0);
> +	mbox_cmd = (struct cxl_mbox_cmd) {
> +		.opcode = CXL_MBOX_OP_PASSPHRASE_SECURE_ERASE,
> +		.size_in = sizeof(erase),
> +		.payload_in = &erase,
> +	};
> +
> +	rc = cxl_internal_send_cmd(cxlds, &mbox_cmd);
>   	if (rc < 0)
>   		return rc;
>   
> 

Re: [PATCH 2/4] cxl/mbox: Enable cxl_mbox_send_cmd() users to validate output size

[Jonathan Cameron]

On Mon, 05 Dec 2022 20:22:33 -0800
Dan Williams <dan.j.williams@intel.com> wrote:

> Internally cxl_mbox_send_cmd() converts all passed-in parameters to a
> 'struct cxl_mbox_cmd' instance and sends that to cxlds->mbox_send(). It
> then teases the possibilty that the caller can validate the output size.
> However, they cannot since the resulting output size is not conveyed to
> the called. Fix that by making the caller pass in a constructed 'struct
> cxl_mbox_cmd'. This prepares for a future patch to add output size
> validation on a per-command basis.
> 
> Given the change in signature, also change the name to differentiate it
> from the user command submission path that performs more validation
> before generating the 'struct cxl_mbox_cmd' instance to execute.
> 
> Signed-off-by: Dan Williams <dan.j.williams@intel.com>

I'm with Ira on ordering in structure alignment. Other trivial things inline.

Usual moans about slipping in unrelated tidy up.  All good stuff, but
would have preferred them separated out.  In interests of speed though
I'm fine with it as it stands

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

>  static int __cxl_pmem_security_disable(struct nvdimm *nvdimm,
> @@ -83,15 +95,21 @@ static int __cxl_pmem_security_disable(struct nvdimm *nvdimm,
>  	struct cxl_memdev *cxlmd = cxl_nvd->cxlmd;
>  	struct cxl_dev_state *cxlds = cxlmd->cxlds;
>  	struct cxl_disable_pass dis_pass;
> -	int rc;
> +	struct cxl_mbox_cmd mbox_cmd;
>  
> -	dis_pass.type = ptype == NVDIMM_MASTER ?
> -		CXL_PMEM_SEC_PASS_MASTER : CXL_PMEM_SEC_PASS_USER;
> +	dis_pass = (struct cxl_disable_pass) {
> +		.type = ptype == NVDIMM_MASTER ? CXL_PMEM_SEC_PASS_MASTER :
> +						 CXL_PMEM_SEC_PASS_USER,
> +	};

Unrelated change as far as I can see. Perhaps reasonable to keep a common
approach, but should really be in a precusor patch.


> @@ -140,14 +167,22 @@ static int cxl_pmem_security_passphrase_erase(struct nvdimm *nvdimm,
>  	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_mbox_cmd mbox_cmd;
>  	struct cxl_pass_erase erase;
>  	int rc;
>  
> -	erase.type = ptype == NVDIMM_MASTER ?
> -		CXL_PMEM_SEC_PASS_MASTER : CXL_PMEM_SEC_PASS_USER;
> +	erase = (struct cxl_pass_erase) {
> +		.type = ptype == NVDIMM_MASTER ? CXL_PMEM_SEC_PASS_MASTER :
> +						 CXL_PMEM_SEC_PASS_USER,
> +	};

Unrelated change?  Meh trivial though so fair enough rolling it in here.


>  	memcpy(erase.pass, key->data, NVDIMM_PASSPHRASE_LEN);
> -	rc = cxl_mbox_send_cmd(cxlds, CXL_MBOX_OP_PASSPHRASE_SECURE_ERASE,
> -			       &erase, sizeof(erase), NULL, 0);
> +	mbox_cmd = (struct cxl_mbox_cmd) {
> +		.opcode = CXL_MBOX_OP_PASSPHRASE_SECURE_ERASE,
> +		.size_in = sizeof(erase),
> +		.payload_in = &erase,
> +	};
> +
> +	rc = cxl_internal_send_cmd(cxlds, &mbox_cmd);
>  	if (rc < 0)
>  		return rc;
>  
> 

[PATCH 3/4] cxl/mbox: Add variable output size validation for internal commands

[Dan Williams]

cxl_internal_send_cmd() skips output size validation for variable output
commands which is not ideal. Most of the time internal usages want to
fail if the output size does not match what was requested. For other
commands where the caller cannot predict the size there is usually a
a header that conveys how much vaild data is in the payload. For those
cases add @min_out as a parameter to specify what the minimum response
payload needs to be for the caller to parse the rest of the payload.

In this patch only Get Supported Logs has that behavior, but going
forward records retrieval commands like Get Poison List and Get Event
Records can use @min_out to retrieve a variable amount of records.

Critically, this validation scheme skips the needs to interrogate the
cxl_mem_commands array which in turn frees up the implementation to
support internal command enabling without also enabling external / user
commands.

Signed-off-by: Dan Williams <dan.j.williams@intel.com>
---
 drivers/cxl/core/mbox.c |   23 ++++++++++++++---------
 drivers/cxl/cxlmem.h    |    2 ++
 2 files changed, 16 insertions(+), 9 deletions(-)

diff --git a/drivers/cxl/core/mbox.c b/drivers/cxl/core/mbox.c
index ed451ca60ce5..c36a3589377a 100644
--- a/drivers/cxl/core/mbox.c
+++ b/drivers/cxl/core/mbox.c
@@ -166,9 +166,7 @@ static const char *cxl_mem_opcode_to_name(u16 opcode)
 int cxl_internal_send_cmd(struct cxl_dev_state *cxlds,
 			  struct cxl_mbox_cmd *mbox_cmd)
 {
-	const struct cxl_mem_command *cmd =
-		cxl_mem_find_command(mbox_cmd->opcode);
-	size_t out_size;
+	size_t out_size, min_out;
 	int rc;
 
 	if (mbox_cmd->size_in > cxlds->payload_size ||
@@ -176,6 +174,7 @@ int cxl_internal_send_cmd(struct cxl_dev_state *cxlds,
 		return -E2BIG;
 
 	out_size = mbox_cmd->size_out;
+	min_out = mbox_cmd->min_out;
 	rc = cxlds->mbox_send(cxlds, mbox_cmd);
 	if (rc)
 		return rc;
@@ -183,14 +182,18 @@ int cxl_internal_send_cmd(struct cxl_dev_state *cxlds,
 	if (mbox_cmd->return_code != CXL_MBOX_CMD_RC_SUCCESS)
 		return cxl_mbox_cmd_rc2errno(mbox_cmd);
 
+	if (!out_size)
+		return 0;
+
 	/*
-	 * Variable sized commands can't be validated and so it's up to the
-	 * caller to do that if they wish.
+	 * Variable sized output needs to at least satisfy the caller's
+	 * minimum if not the fully requested size.
 	 */
-	if (cmd->info.size_out != CXL_VARIABLE_PAYLOAD) {
-		if (mbox_cmd->size_out != out_size)
-			return -EIO;
-	}
+	if (min_out == 0)
+		min_out = out_size;
+
+	if (mbox_cmd->size_out < min_out)
+		return -EIO;
 	return 0;
 }
 EXPORT_SYMBOL_NS_GPL(cxl_internal_send_cmd, CXL);
@@ -635,6 +638,8 @@ static struct cxl_mbox_get_supported_logs *cxl_get_gsl(struct cxl_dev_state *cxl
 		.opcode = CXL_MBOX_OP_GET_SUPPORTED_LOGS,
 		.size_out = cxlds->payload_size,
 		.payload_out = ret,
+		/* At least the record number field must be valid */
+		.min_out = 2,
 	};
 	rc = cxl_internal_send_cmd(cxlds, &mbox_cmd);
 	if (rc < 0) {
diff --git a/drivers/cxl/cxlmem.h b/drivers/cxl/cxlmem.h
index c447577f5ad5..ab138004f644 100644
--- a/drivers/cxl/cxlmem.h
+++ b/drivers/cxl/cxlmem.h
@@ -101,6 +101,7 @@ static inline struct cxl_ep *cxl_ep_load(struct cxl_port *port,
  *            outputs commands this is always expected to be deterministic. For
  *            variable sized output commands, it tells the exact number of bytes
  *            written.
+ * @min_out: (input) internal command output payload size validation
  * @return_code: (output) Error code returned from hardware.
  *
  * This is the primary mechanism used to send commands to the hardware.
@@ -115,6 +116,7 @@ struct cxl_mbox_cmd {
 	void *payload_out;
 	size_t size_in;
 	size_t size_out;
+	size_t min_out;
 	u16 return_code;
 };
 

Re: [PATCH 3/4] cxl/mbox: Add variable output size validation for internal commands

[Ira Weiny]

On Mon, Dec 05, 2022 at 08:22:39PM -0800, Dan Williams wrote:
> cxl_internal_send_cmd() skips output size validation for variable output
> commands which is not ideal. Most of the time internal usages want to
> fail if the output size does not match what was requested. For other
> commands where the caller cannot predict the size there is usually a
> a header that conveys how much vaild data is in the payload. For those
> cases add @min_out as a parameter to specify what the minimum response
> payload needs to be for the caller to parse the rest of the payload.
> 
> In this patch only Get Supported Logs has that behavior, but going
> forward records retrieval commands like Get Poison List and Get Event
> Records can use @min_out to retrieve a variable amount of records.
> 
> Critically, this validation scheme skips the needs to interrogate the

					       need

> cxl_mem_commands array which in turn frees up the implementation to
> support internal command enabling without also enabling external / user
> commands.
> 

Minor comment below.

Reviewed-by: Ira Weiny <ira.weiny@intel.com>

> Signed-off-by: Dan Williams <dan.j.williams@intel.com>
> ---
>  drivers/cxl/core/mbox.c |   23 ++++++++++++++---------
>  drivers/cxl/cxlmem.h    |    2 ++
>  2 files changed, 16 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/cxl/core/mbox.c b/drivers/cxl/core/mbox.c
> index ed451ca60ce5..c36a3589377a 100644
> --- a/drivers/cxl/core/mbox.c
> +++ b/drivers/cxl/core/mbox.c
> @@ -166,9 +166,7 @@ static const char *cxl_mem_opcode_to_name(u16 opcode)
>  int cxl_internal_send_cmd(struct cxl_dev_state *cxlds,
>  			  struct cxl_mbox_cmd *mbox_cmd)
>  {
> -	const struct cxl_mem_command *cmd =
> -		cxl_mem_find_command(mbox_cmd->opcode);
> -	size_t out_size;
> +	size_t out_size, min_out;
>  	int rc;
>  
>  	if (mbox_cmd->size_in > cxlds->payload_size ||
> @@ -176,6 +174,7 @@ int cxl_internal_send_cmd(struct cxl_dev_state *cxlds,
>  		return -E2BIG;
>  
>  	out_size = mbox_cmd->size_out;
> +	min_out = mbox_cmd->min_out;
>  	rc = cxlds->mbox_send(cxlds, mbox_cmd);
>  	if (rc)
>  		return rc;
> @@ -183,14 +182,18 @@ int cxl_internal_send_cmd(struct cxl_dev_state *cxlds,
>  	if (mbox_cmd->return_code != CXL_MBOX_CMD_RC_SUCCESS)
>  		return cxl_mbox_cmd_rc2errno(mbox_cmd);
>  
> +	if (!out_size)

	if (out_size == 0)
	???

> +		return 0;
> +
>  	/*
> -	 * Variable sized commands can't be validated and so it's up to the
> -	 * caller to do that if they wish.
> +	 * Variable sized output needs to at least satisfy the caller's
> +	 * minimum if not the fully requested size.
>  	 */
> -	if (cmd->info.size_out != CXL_VARIABLE_PAYLOAD) {
> -		if (mbox_cmd->size_out != out_size)
> -			return -EIO;
> -	}
> +	if (min_out == 0)

I prefer this logic but NIT is that they are both used.

Ira

> +		min_out = out_size;
> +
> +	if (mbox_cmd->size_out < min_out)
> +		return -EIO;
>  	return 0;
>  }
>  EXPORT_SYMBOL_NS_GPL(cxl_internal_send_cmd, CXL);
> @@ -635,6 +638,8 @@ static struct cxl_mbox_get_supported_logs *cxl_get_gsl(struct cxl_dev_state *cxl
>  		.opcode = CXL_MBOX_OP_GET_SUPPORTED_LOGS,
>  		.size_out = cxlds->payload_size,
>  		.payload_out = ret,
> +		/* At least the record number field must be valid */
> +		.min_out = 2,
>  	};
>  	rc = cxl_internal_send_cmd(cxlds, &mbox_cmd);
>  	if (rc < 0) {
> diff --git a/drivers/cxl/cxlmem.h b/drivers/cxl/cxlmem.h
> index c447577f5ad5..ab138004f644 100644
> --- a/drivers/cxl/cxlmem.h
> +++ b/drivers/cxl/cxlmem.h
> @@ -101,6 +101,7 @@ static inline struct cxl_ep *cxl_ep_load(struct cxl_port *port,
>   *            outputs commands this is always expected to be deterministic. For
>   *            variable sized output commands, it tells the exact number of bytes
>   *            written.
> + * @min_out: (input) internal command output payload size validation
>   * @return_code: (output) Error code returned from hardware.
>   *
>   * This is the primary mechanism used to send commands to the hardware.
> @@ -115,6 +116,7 @@ struct cxl_mbox_cmd {
>  	void *payload_out;
>  	size_t size_in;
>  	size_t size_out;
> +	size_t min_out;
>  	u16 return_code;
>  };
>  
> 

Re: [PATCH 3/4] cxl/mbox: Add variable output size validation for internal commands

[Dave Jiang]

On 12/5/2022 9:22 PM, Dan Williams wrote:
> cxl_internal_send_cmd() skips output size validation for variable output
> commands which is not ideal. Most of the time internal usages want to
> fail if the output size does not match what was requested. For other
> commands where the caller cannot predict the size there is usually a
> a header that conveys how much vaild data is in the payload. For those
> cases add @min_out as a parameter to specify what the minimum response
> payload needs to be for the caller to parse the rest of the payload.
> 
> In this patch only Get Supported Logs has that behavior, but going
> forward records retrieval commands like Get Poison List and Get Event
> Records can use @min_out to retrieve a variable amount of records.
> 
> Critically, this validation scheme skips the needs to interrogate the
> cxl_mem_commands array which in turn frees up the implementation to
> support internal command enabling without also enabling external / user
> commands.
> 
> Signed-off-by: Dan Williams <dan.j.williams@intel.com>

Reviewed-by: Dave Jiang <dave.jiang@intel.com>

> ---
>   drivers/cxl/core/mbox.c |   23 ++++++++++++++---------
>   drivers/cxl/cxlmem.h    |    2 ++
>   2 files changed, 16 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/cxl/core/mbox.c b/drivers/cxl/core/mbox.c
> index ed451ca60ce5..c36a3589377a 100644
> --- a/drivers/cxl/core/mbox.c
> +++ b/drivers/cxl/core/mbox.c
> @@ -166,9 +166,7 @@ static const char *cxl_mem_opcode_to_name(u16 opcode)
>   int cxl_internal_send_cmd(struct cxl_dev_state *cxlds,
>   			  struct cxl_mbox_cmd *mbox_cmd)
>   {
> -	const struct cxl_mem_command *cmd =
> -		cxl_mem_find_command(mbox_cmd->opcode);
> -	size_t out_size;
> +	size_t out_size, min_out;
>   	int rc;
>   
>   	if (mbox_cmd->size_in > cxlds->payload_size ||
> @@ -176,6 +174,7 @@ int cxl_internal_send_cmd(struct cxl_dev_state *cxlds,
>   		return -E2BIG;
>   
>   	out_size = mbox_cmd->size_out;
> +	min_out = mbox_cmd->min_out;
>   	rc = cxlds->mbox_send(cxlds, mbox_cmd);
>   	if (rc)
>   		return rc;
> @@ -183,14 +182,18 @@ int cxl_internal_send_cmd(struct cxl_dev_state *cxlds,
>   	if (mbox_cmd->return_code != CXL_MBOX_CMD_RC_SUCCESS)
>   		return cxl_mbox_cmd_rc2errno(mbox_cmd);
>   
> +	if (!out_size)
> +		return 0;
> +
>   	/*
> -	 * Variable sized commands can't be validated and so it's up to the
> -	 * caller to do that if they wish.
> +	 * Variable sized output needs to at least satisfy the caller's
> +	 * minimum if not the fully requested size.
>   	 */
> -	if (cmd->info.size_out != CXL_VARIABLE_PAYLOAD) {
> -		if (mbox_cmd->size_out != out_size)
> -			return -EIO;
> -	}
> +	if (min_out == 0)
> +		min_out = out_size;
> +
> +	if (mbox_cmd->size_out < min_out)
> +		return -EIO;
>   	return 0;
>   }
>   EXPORT_SYMBOL_NS_GPL(cxl_internal_send_cmd, CXL);
> @@ -635,6 +638,8 @@ static struct cxl_mbox_get_supported_logs *cxl_get_gsl(struct cxl_dev_state *cxl
>   		.opcode = CXL_MBOX_OP_GET_SUPPORTED_LOGS,
>   		.size_out = cxlds->payload_size,
>   		.payload_out = ret,
> +		/* At least the record number field must be valid */
> +		.min_out = 2,
>   	};
>   	rc = cxl_internal_send_cmd(cxlds, &mbox_cmd);
>   	if (rc < 0) {
> diff --git a/drivers/cxl/cxlmem.h b/drivers/cxl/cxlmem.h
> index c447577f5ad5..ab138004f644 100644
> --- a/drivers/cxl/cxlmem.h
> +++ b/drivers/cxl/cxlmem.h
> @@ -101,6 +101,7 @@ static inline struct cxl_ep *cxl_ep_load(struct cxl_port *port,
>    *            outputs commands this is always expected to be deterministic. For
>    *            variable sized output commands, it tells the exact number of bytes
>    *            written.
> + * @min_out: (input) internal command output payload size validation
>    * @return_code: (output) Error code returned from hardware.
>    *
>    * This is the primary mechanism used to send commands to the hardware.
> @@ -115,6 +116,7 @@ struct cxl_mbox_cmd {
>   	void *payload_out;
>   	size_t size_in;
>   	size_t size_out;
> +	size_t min_out;
>   	u16 return_code;
>   };
>   
> 

Re: [PATCH 3/4] cxl/mbox: Add variable output size validation for internal commands

[Jonathan Cameron]

On Mon, 05 Dec 2022 20:22:39 -0800
Dan Williams <dan.j.williams@intel.com> wrote:

> cxl_internal_send_cmd() skips output size validation for variable output
> commands which is not ideal. Most of the time internal usages want to
> fail if the output size does not match what was requested. For other
> commands where the caller cannot predict the size there is usually a
> a header that conveys how much vaild data is in the payload. For those
> cases add @min_out as a parameter to specify what the minimum response
> payload needs to be for the caller to parse the rest of the payload.
> 
> In this patch only Get Supported Logs has that behavior, but going
> forward records retrieval commands like Get Poison List and Get Event
> Records can use @min_out to retrieve a variable amount of records.
> 
> Critically, this validation scheme skips the needs to interrogate the
> cxl_mem_commands array which in turn frees up the implementation to
> support internal command enabling without also enabling external / user
> commands.
> 
> Signed-off-by: Dan Williams <dan.j.williams@intel.com>
Makes sense.
Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

Re: [PATCH 3/4] cxl/mbox: Add variable output size validation for internal commands

[Alison Schofield]

On Mon, Dec 05, 2022 at 08:22:39PM -0800, Dan Williams wrote:
> cxl_internal_send_cmd() skips output size validation for variable output
> commands which is not ideal. Most of the time internal usages want to
> fail if the output size does not match what was requested. For other
> commands where the caller cannot predict the size there is usually a
> a header that conveys how much vaild data is in the payload. For those
> cases add @min_out as a parameter to specify what the minimum response
> payload needs to be for the caller to parse the rest of the payload.
> 
> In this patch only Get Supported Logs has that behavior, but going
> forward records retrieval commands like Get Poison List and Get Event
> Records can use @min_out to retrieve a variable amount of records.
> 
> Critically, this validation scheme skips the needs to interrogate the
> cxl_mem_commands array which in turn frees up the implementation to
> support internal command enabling without also enabling external / user
> commands.

I tested (tripped upon) this while rebasing the Get Poison List patchset.
This commit log answers something I wondered - why a default min_out
value was not assigned in the cxl_mem_commands array. I get it now.

Tested-by: Alison Schofield <alison.schofield@intel.com>

> 
> Signed-off-by: Dan Williams <dan.j.williams@intel.com>
> ---
>  drivers/cxl/core/mbox.c |   23 ++++++++++++++---------
>  drivers/cxl/cxlmem.h    |    2 ++
>  2 files changed, 16 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/cxl/core/mbox.c b/drivers/cxl/core/mbox.c
> index ed451ca60ce5..c36a3589377a 100644
> --- a/drivers/cxl/core/mbox.c
> +++ b/drivers/cxl/core/mbox.c
> @@ -166,9 +166,7 @@ static const char *cxl_mem_opcode_to_name(u16 opcode)
>  int cxl_internal_send_cmd(struct cxl_dev_state *cxlds,
>  			  struct cxl_mbox_cmd *mbox_cmd)
>  {
> -	const struct cxl_mem_command *cmd =
> -		cxl_mem_find_command(mbox_cmd->opcode);
> -	size_t out_size;
> +	size_t out_size, min_out;
>  	int rc;
>  
>  	if (mbox_cmd->size_in > cxlds->payload_size ||
> @@ -176,6 +174,7 @@ int cxl_internal_send_cmd(struct cxl_dev_state *cxlds,
>  		return -E2BIG;
>  
>  	out_size = mbox_cmd->size_out;
> +	min_out = mbox_cmd->min_out;
>  	rc = cxlds->mbox_send(cxlds, mbox_cmd);
>  	if (rc)
>  		return rc;
> @@ -183,14 +182,18 @@ int cxl_internal_send_cmd(struct cxl_dev_state *cxlds,
>  	if (mbox_cmd->return_code != CXL_MBOX_CMD_RC_SUCCESS)
>  		return cxl_mbox_cmd_rc2errno(mbox_cmd);
>  
> +	if (!out_size)
> +		return 0;
> +
>  	/*
> -	 * Variable sized commands can't be validated and so it's up to the
> -	 * caller to do that if they wish.
> +	 * Variable sized output needs to at least satisfy the caller's
> +	 * minimum if not the fully requested size.
>  	 */
> -	if (cmd->info.size_out != CXL_VARIABLE_PAYLOAD) {
> -		if (mbox_cmd->size_out != out_size)
> -			return -EIO;
> -	}
> +	if (min_out == 0)
> +		min_out = out_size;
> +
> +	if (mbox_cmd->size_out < min_out)
> +		return -EIO;
>  	return 0;
>  }
>  EXPORT_SYMBOL_NS_GPL(cxl_internal_send_cmd, CXL);
> @@ -635,6 +638,8 @@ static struct cxl_mbox_get_supported_logs *cxl_get_gsl(struct cxl_dev_state *cxl
>  		.opcode = CXL_MBOX_OP_GET_SUPPORTED_LOGS,
>  		.size_out = cxlds->payload_size,
>  		.payload_out = ret,
> +		/* At least the record number field must be valid */
> +		.min_out = 2,
>  	};
>  	rc = cxl_internal_send_cmd(cxlds, &mbox_cmd);
>  	if (rc < 0) {
> diff --git a/drivers/cxl/cxlmem.h b/drivers/cxl/cxlmem.h
> index c447577f5ad5..ab138004f644 100644
> --- a/drivers/cxl/cxlmem.h
> +++ b/drivers/cxl/cxlmem.h
> @@ -101,6 +101,7 @@ static inline struct cxl_ep *cxl_ep_load(struct cxl_port *port,
>   *            outputs commands this is always expected to be deterministic. For
>   *            variable sized output commands, it tells the exact number of bytes
>   *            written.
> + * @min_out: (input) internal command output payload size validation
>   * @return_code: (output) Error code returned from hardware.
>   *
>   * This is the primary mechanism used to send commands to the hardware.
> @@ -115,6 +116,7 @@ struct cxl_mbox_cmd {
>  	void *payload_out;
>  	size_t size_in;
>  	size_t size_out;
> +	size_t min_out;
>  	u16 return_code;
>  };
>  
> 

[PATCH 4/4] cxl/security: Drop security command ioctl uapi

[Dan Williams]

CXL PMEM security operations are routed through the NVDIMM sysfs
interface. For this reason the corresponding commands are marked
"exclusive" to preclude collisions between the ioctl ABI and the sysfs
ABI. However, a better way to preclude that collision is to simply
remove the ioctl ABI (command-id definitions) for those operations.

Now that cxl_internal_send_cmd() (formerly cxl_mbox_send_cmd()) no
longer needs to talk the cxl_mem_commands array, all of the uapi
definitions for the security commands can be dropped.

These never appeared in a released kernel, so no regression risk.

Signed-off-by: Dan Williams <dan.j.williams@intel.com>
---
 drivers/cxl/core/mbox.c      |   17 -----------------
 include/uapi/linux/cxl_mem.h |    6 ------
 2 files changed, 23 deletions(-)

diff --git a/drivers/cxl/core/mbox.c b/drivers/cxl/core/mbox.c
index c36a3589377a..b03fba212799 100644
--- a/drivers/cxl/core/mbox.c
+++ b/drivers/cxl/core/mbox.c
@@ -65,12 +65,6 @@ static struct cxl_mem_command cxl_mem_commands[CXL_MEM_COMMAND_ID_MAX] = {
 	CXL_CMD(GET_SCAN_MEDIA_CAPS, 0x10, 0x4, 0),
 	CXL_CMD(SCAN_MEDIA, 0x11, 0, 0),
 	CXL_CMD(GET_SCAN_MEDIA, 0, CXL_VARIABLE_PAYLOAD, 0),
-	CXL_CMD(GET_SECURITY_STATE, 0, 0x4, 0),
-	CXL_CMD(SET_PASSPHRASE, 0x60, 0, 0),
-	CXL_CMD(DISABLE_PASSPHRASE, 0x40, 0, 0),
-	CXL_CMD(FREEZE_SECURITY, 0, 0, 0),
-	CXL_CMD(UNLOCK, 0x20, 0, 0),
-	CXL_CMD(PASSPHRASE_SECURE_ERASE, 0x40, 0, 0),
 };
 
 /*
@@ -717,17 +711,6 @@ int cxl_enumerate_cmds(struct cxl_dev_state *cxlds)
 		/* Found the required CEL */
 		rc = 0;
 	}
-
-	/*
-	 * Setup permanently kernel exclusive commands, i.e. the
-	 * mechanism is driven through sysfs, keyctl, etc...
-	 */
-	set_bit(CXL_MEM_COMMAND_ID_SET_PASSPHRASE, cxlds->exclusive_cmds);
-	set_bit(CXL_MEM_COMMAND_ID_DISABLE_PASSPHRASE, cxlds->exclusive_cmds);
-	set_bit(CXL_MEM_COMMAND_ID_UNLOCK, cxlds->exclusive_cmds);
-	set_bit(CXL_MEM_COMMAND_ID_PASSPHRASE_SECURE_ERASE,
-		cxlds->exclusive_cmds);
-
 out:
 	kvfree(gsl);
 	return rc;
diff --git a/include/uapi/linux/cxl_mem.h b/include/uapi/linux/cxl_mem.h
index 82bdad4ce5de..c71021a2a9ed 100644
--- a/include/uapi/linux/cxl_mem.h
+++ b/include/uapi/linux/cxl_mem.h
@@ -41,12 +41,6 @@
 	___C(GET_SCAN_MEDIA_CAPS, "Get Scan Media Capabilities"),         \
 	___C(SCAN_MEDIA, "Scan Media"),                                   \
 	___C(GET_SCAN_MEDIA, "Get Scan Media Results"),                   \
-	___C(GET_SECURITY_STATE, "Get Security State"),			  \
-	___C(SET_PASSPHRASE, "Set Passphrase"),				  \
-	___C(DISABLE_PASSPHRASE, "Disable Passphrase"),			  \
-	___C(FREEZE_SECURITY, "Freeze Security"),			  \
-	___C(UNLOCK, "Unlock"),						  \
-	___C(PASSPHRASE_SECURE_ERASE, "Passphrase Secure Erase"),	  \
 	___C(MAX, "invalid / last command")
 
 #define ___C(a, b) CXL_MEM_COMMAND_ID_##a

Re: [PATCH 4/4] cxl/security: Drop security command ioctl uapi

[Ira Weiny]

On Mon, Dec 05, 2022 at 08:22:44PM -0800, Dan Williams wrote:
> CXL PMEM security operations are routed through the NVDIMM sysfs
> interface. For this reason the corresponding commands are marked
> "exclusive" to preclude collisions between the ioctl ABI and the sysfs
> ABI. However, a better way to preclude that collision is to simply
> remove the ioctl ABI (command-id definitions) for those operations.
> 
> Now that cxl_internal_send_cmd() (formerly cxl_mbox_send_cmd()) no
> longer needs to talk the cxl_mem_commands array, all of the uapi
> definitions for the security commands can be dropped.
> 
> These never appeared in a released kernel, so no regression risk.
> 

Reviewed-by: Ira Weiny <ira.weiny@intel.com>

> Signed-off-by: Dan Williams <dan.j.williams@intel.com>
> ---
>  drivers/cxl/core/mbox.c      |   17 -----------------
>  include/uapi/linux/cxl_mem.h |    6 ------
>  2 files changed, 23 deletions(-)
> 
> diff --git a/drivers/cxl/core/mbox.c b/drivers/cxl/core/mbox.c
> index c36a3589377a..b03fba212799 100644
> --- a/drivers/cxl/core/mbox.c
> +++ b/drivers/cxl/core/mbox.c
> @@ -65,12 +65,6 @@ static struct cxl_mem_command cxl_mem_commands[CXL_MEM_COMMAND_ID_MAX] = {
>  	CXL_CMD(GET_SCAN_MEDIA_CAPS, 0x10, 0x4, 0),
>  	CXL_CMD(SCAN_MEDIA, 0x11, 0, 0),
>  	CXL_CMD(GET_SCAN_MEDIA, 0, CXL_VARIABLE_PAYLOAD, 0),
> -	CXL_CMD(GET_SECURITY_STATE, 0, 0x4, 0),
> -	CXL_CMD(SET_PASSPHRASE, 0x60, 0, 0),
> -	CXL_CMD(DISABLE_PASSPHRASE, 0x40, 0, 0),
> -	CXL_CMD(FREEZE_SECURITY, 0, 0, 0),
> -	CXL_CMD(UNLOCK, 0x20, 0, 0),
> -	CXL_CMD(PASSPHRASE_SECURE_ERASE, 0x40, 0, 0),
>  };
>  
>  /*
> @@ -717,17 +711,6 @@ int cxl_enumerate_cmds(struct cxl_dev_state *cxlds)
>  		/* Found the required CEL */
>  		rc = 0;
>  	}
> -
> -	/*
> -	 * Setup permanently kernel exclusive commands, i.e. the
> -	 * mechanism is driven through sysfs, keyctl, etc...
> -	 */
> -	set_bit(CXL_MEM_COMMAND_ID_SET_PASSPHRASE, cxlds->exclusive_cmds);
> -	set_bit(CXL_MEM_COMMAND_ID_DISABLE_PASSPHRASE, cxlds->exclusive_cmds);
> -	set_bit(CXL_MEM_COMMAND_ID_UNLOCK, cxlds->exclusive_cmds);
> -	set_bit(CXL_MEM_COMMAND_ID_PASSPHRASE_SECURE_ERASE,
> -		cxlds->exclusive_cmds);
> -
>  out:
>  	kvfree(gsl);
>  	return rc;
> diff --git a/include/uapi/linux/cxl_mem.h b/include/uapi/linux/cxl_mem.h
> index 82bdad4ce5de..c71021a2a9ed 100644
> --- a/include/uapi/linux/cxl_mem.h
> +++ b/include/uapi/linux/cxl_mem.h
> @@ -41,12 +41,6 @@
>  	___C(GET_SCAN_MEDIA_CAPS, "Get Scan Media Capabilities"),         \
>  	___C(SCAN_MEDIA, "Scan Media"),                                   \
>  	___C(GET_SCAN_MEDIA, "Get Scan Media Results"),                   \
> -	___C(GET_SECURITY_STATE, "Get Security State"),			  \
> -	___C(SET_PASSPHRASE, "Set Passphrase"),				  \
> -	___C(DISABLE_PASSPHRASE, "Disable Passphrase"),			  \
> -	___C(FREEZE_SECURITY, "Freeze Security"),			  \
> -	___C(UNLOCK, "Unlock"),						  \
> -	___C(PASSPHRASE_SECURE_ERASE, "Passphrase Secure Erase"),	  \
>  	___C(MAX, "invalid / last command")
>  
>  #define ___C(a, b) CXL_MEM_COMMAND_ID_##a
> 

Re: [PATCH 4/4] cxl/security: Drop security command ioctl uapi

[Dave Jiang]

On 12/5/2022 9:22 PM, Dan Williams wrote:
> CXL PMEM security operations are routed through the NVDIMM sysfs
> interface. For this reason the corresponding commands are marked
> "exclusive" to preclude collisions between the ioctl ABI and the sysfs
> ABI. However, a better way to preclude that collision is to simply
> remove the ioctl ABI (command-id definitions) for those operations.
> 
> Now that cxl_internal_send_cmd() (formerly cxl_mbox_send_cmd()) no
> longer needs to talk the cxl_mem_commands array, all of the uapi
> definitions for the security commands can be dropped.
> 
> These never appeared in a released kernel, so no regression risk.
> 
> Signed-off-by: Dan Williams <dan.j.williams@intel.com>

Reviewed-by: Dave Jiang <dave.jiang@intel.com>

> ---
>   drivers/cxl/core/mbox.c      |   17 -----------------
>   include/uapi/linux/cxl_mem.h |    6 ------
>   2 files changed, 23 deletions(-)
> 
> diff --git a/drivers/cxl/core/mbox.c b/drivers/cxl/core/mbox.c
> index c36a3589377a..b03fba212799 100644
> --- a/drivers/cxl/core/mbox.c
> +++ b/drivers/cxl/core/mbox.c
> @@ -65,12 +65,6 @@ static struct cxl_mem_command cxl_mem_commands[CXL_MEM_COMMAND_ID_MAX] = {
>   	CXL_CMD(GET_SCAN_MEDIA_CAPS, 0x10, 0x4, 0),
>   	CXL_CMD(SCAN_MEDIA, 0x11, 0, 0),
>   	CXL_CMD(GET_SCAN_MEDIA, 0, CXL_VARIABLE_PAYLOAD, 0),
> -	CXL_CMD(GET_SECURITY_STATE, 0, 0x4, 0),
> -	CXL_CMD(SET_PASSPHRASE, 0x60, 0, 0),
> -	CXL_CMD(DISABLE_PASSPHRASE, 0x40, 0, 0),
> -	CXL_CMD(FREEZE_SECURITY, 0, 0, 0),
> -	CXL_CMD(UNLOCK, 0x20, 0, 0),
> -	CXL_CMD(PASSPHRASE_SECURE_ERASE, 0x40, 0, 0),
>   };
>   
>   /*
> @@ -717,17 +711,6 @@ int cxl_enumerate_cmds(struct cxl_dev_state *cxlds)
>   		/* Found the required CEL */
>   		rc = 0;
>   	}
> -
> -	/*
> -	 * Setup permanently kernel exclusive commands, i.e. the
> -	 * mechanism is driven through sysfs, keyctl, etc...
> -	 */
> -	set_bit(CXL_MEM_COMMAND_ID_SET_PASSPHRASE, cxlds->exclusive_cmds);
> -	set_bit(CXL_MEM_COMMAND_ID_DISABLE_PASSPHRASE, cxlds->exclusive_cmds);
> -	set_bit(CXL_MEM_COMMAND_ID_UNLOCK, cxlds->exclusive_cmds);
> -	set_bit(CXL_MEM_COMMAND_ID_PASSPHRASE_SECURE_ERASE,
> -		cxlds->exclusive_cmds);
> -
>   out:
>   	kvfree(gsl);
>   	return rc;
> diff --git a/include/uapi/linux/cxl_mem.h b/include/uapi/linux/cxl_mem.h
> index 82bdad4ce5de..c71021a2a9ed 100644
> --- a/include/uapi/linux/cxl_mem.h
> +++ b/include/uapi/linux/cxl_mem.h
> @@ -41,12 +41,6 @@
>   	___C(GET_SCAN_MEDIA_CAPS, "Get Scan Media Capabilities"),         \
>   	___C(SCAN_MEDIA, "Scan Media"),                                   \
>   	___C(GET_SCAN_MEDIA, "Get Scan Media Results"),                   \
> -	___C(GET_SECURITY_STATE, "Get Security State"),			  \
> -	___C(SET_PASSPHRASE, "Set Passphrase"),				  \
> -	___C(DISABLE_PASSPHRASE, "Disable Passphrase"),			  \
> -	___C(FREEZE_SECURITY, "Freeze Security"),			  \
> -	___C(UNLOCK, "Unlock"),						  \
> -	___C(PASSPHRASE_SECURE_ERASE, "Passphrase Secure Erase"),	  \
>   	___C(MAX, "invalid / last command")
>   
>   #define ___C(a, b) CXL_MEM_COMMAND_ID_##a
> 

Re: [PATCH 4/4] cxl/security: Drop security command ioctl uapi

[Jonathan Cameron]

On Mon, 05 Dec 2022 20:22:44 -0800
Dan Williams <dan.j.williams@intel.com> wrote:

> CXL PMEM security operations are routed through the NVDIMM sysfs
> interface. For this reason the corresponding commands are marked
> "exclusive" to preclude collisions between the ioctl ABI and the sysfs
> ABI. However, a better way to preclude that collision is to simply
> remove the ioctl ABI (command-id definitions) for those operations.
> 
> Now that cxl_internal_send_cmd() (formerly cxl_mbox_send_cmd()) no
> longer needs to talk the cxl_mem_commands array, all of the uapi
> definitions for the security commands can be dropped.
> 
> These never appeared in a released kernel, so no regression risk.
> 
> Signed-off-by: Dan Williams <dan.j.williams@intel.com>

Makes sense

Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> ---
>  drivers/cxl/core/mbox.c      |   17 -----------------
>  include/uapi/linux/cxl_mem.h |    6 ------
>  2 files changed, 23 deletions(-)
> 
> diff --git a/drivers/cxl/core/mbox.c b/drivers/cxl/core/mbox.c
> index c36a3589377a..b03fba212799 100644
> --- a/drivers/cxl/core/mbox.c
> +++ b/drivers/cxl/core/mbox.c
> @@ -65,12 +65,6 @@ static struct cxl_mem_command cxl_mem_commands[CXL_MEM_COMMAND_ID_MAX] = {
>  	CXL_CMD(GET_SCAN_MEDIA_CAPS, 0x10, 0x4, 0),
>  	CXL_CMD(SCAN_MEDIA, 0x11, 0, 0),
>  	CXL_CMD(GET_SCAN_MEDIA, 0, CXL_VARIABLE_PAYLOAD, 0),
> -	CXL_CMD(GET_SECURITY_STATE, 0, 0x4, 0),
> -	CXL_CMD(SET_PASSPHRASE, 0x60, 0, 0),
> -	CXL_CMD(DISABLE_PASSPHRASE, 0x40, 0, 0),
> -	CXL_CMD(FREEZE_SECURITY, 0, 0, 0),
> -	CXL_CMD(UNLOCK, 0x20, 0, 0),
> -	CXL_CMD(PASSPHRASE_SECURE_ERASE, 0x40, 0, 0),
>  };
>  
>  /*
> @@ -717,17 +711,6 @@ int cxl_enumerate_cmds(struct cxl_dev_state *cxlds)
>  		/* Found the required CEL */
>  		rc = 0;
>  	}
> -
> -	/*
> -	 * Setup permanently kernel exclusive commands, i.e. the
> -	 * mechanism is driven through sysfs, keyctl, etc...
> -	 */
> -	set_bit(CXL_MEM_COMMAND_ID_SET_PASSPHRASE, cxlds->exclusive_cmds);
> -	set_bit(CXL_MEM_COMMAND_ID_DISABLE_PASSPHRASE, cxlds->exclusive_cmds);
> -	set_bit(CXL_MEM_COMMAND_ID_UNLOCK, cxlds->exclusive_cmds);
> -	set_bit(CXL_MEM_COMMAND_ID_PASSPHRASE_SECURE_ERASE,
> -		cxlds->exclusive_cmds);
> -
>  out:
>  	kvfree(gsl);
>  	return rc;
> diff --git a/include/uapi/linux/cxl_mem.h b/include/uapi/linux/cxl_mem.h
> index 82bdad4ce5de..c71021a2a9ed 100644
> --- a/include/uapi/linux/cxl_mem.h
> +++ b/include/uapi/linux/cxl_mem.h
> @@ -41,12 +41,6 @@
>  	___C(GET_SCAN_MEDIA_CAPS, "Get Scan Media Capabilities"),         \
>  	___C(SCAN_MEDIA, "Scan Media"),                                   \
>  	___C(GET_SCAN_MEDIA, "Get Scan Media Results"),                   \
> -	___C(GET_SECURITY_STATE, "Get Security State"),			  \
> -	___C(SET_PASSPHRASE, "Set Passphrase"),				  \
> -	___C(DISABLE_PASSPHRASE, "Disable Passphrase"),			  \
> -	___C(FREEZE_SECURITY, "Freeze Security"),			  \
> -	___C(UNLOCK, "Unlock"),						  \
> -	___C(PASSPHRASE_SECURE_ERASE, "Passphrase Secure Erase"),	  \
>  	___C(MAX, "invalid / last command")
>  
>  #define ___C(a, b) CXL_MEM_COMMAND_ID_##a
>