From: Dave Jiang <dave.jiang@intel.com>
To: Dan Williams <dan.j.williams@intel.com>, linux-cxl@vger.kernel.org
Cc: ira.weiny@intel.com
Subject: Re: [PATCH 2/4] cxl/mbox: Enable cxl_mbox_send_cmd() users to validate output size
Date: Tue, 6 Dec 2022 09:35:33 -0700 [thread overview]
Message-ID: <d337e198-b275-280b-ea12-d2e6e278c4cb@intel.com> (raw)
In-Reply-To: <167030055370.4044561.17788093375112783036.stgit@dwillia2-xfh.jf.intel.com>
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;
>
>
next prev parent reply other threads:[~2022-12-06 16:36 UTC|newest]
Thread overview: 18+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-12-06 4:22 [PATCH 0/4] cxl/mbox: Output payload validation reworks Dan Williams
2022-12-06 4:22 ` [PATCH 1/4] cxl/security: Fix Get Security State output payload endian handling Dan Williams
2022-12-06 6:07 ` Ira Weiny
2022-12-06 16:21 ` Dave Jiang
2022-12-08 10:52 ` Jonathan Cameron
2022-12-06 4:22 ` [PATCH 2/4] cxl/mbox: Enable cxl_mbox_send_cmd() users to validate output size Dan Williams
2022-12-06 6:27 ` Ira Weiny
2022-12-06 16:35 ` Dave Jiang [this message]
2022-12-08 11:01 ` Jonathan Cameron
2022-12-06 4:22 ` [PATCH 3/4] cxl/mbox: Add variable output size validation for internal commands Dan Williams
2022-12-06 6:36 ` Ira Weiny
2022-12-06 16:53 ` Dave Jiang
2022-12-08 11:03 ` Jonathan Cameron
2022-12-08 21:24 ` Alison Schofield
2022-12-06 4:22 ` [PATCH 4/4] cxl/security: Drop security command ioctl uapi Dan Williams
2022-12-06 6:38 ` Ira Weiny
2022-12-06 16:56 ` Dave Jiang
2022-12-08 10:51 ` Jonathan Cameron
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=d337e198-b275-280b-ea12-d2e6e278c4cb@intel.com \
--to=dave.jiang@intel.com \
--cc=dan.j.williams@intel.com \
--cc=ira.weiny@intel.com \
--cc=linux-cxl@vger.kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox