[PATCH] cxl/mbox: Handle variable size output while still signed

[PATCH] cxl/mbox: Handle variable size output while still signed

[alison.schofield]

From: Alison Schofield <alison.schofield@intel.com>

A mailbox command output size value of (-1) means that a device
may return a variable sized output for that command. The device
defines a maximum payload size, so the driver will allocate enough
memory to receive that maximum payload for these commands.

A recent code refactoring moved the check for variable sized output
to occur after the signedness is discarded from the output size.
Move the check to occur while still operating on the signed output
size variable.

Smatch warn: unsigned 'out_size' is never less than 0

Fixes: be0d0ce77aa3 ("cxl/mbox: Move build of user mailbox cmd to a helper funct
ions")
Reported-by: kernel test robot <lkp@intel.com>
Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
Signed-off-by: Alison Schofield <alison.schofield@intel.com>
---
 drivers/cxl/core/mbox.c | 15 +++++++--------
 1 file changed, 7 insertions(+), 8 deletions(-)

diff --git a/drivers/cxl/core/mbox.c b/drivers/cxl/core/mbox.c
index 8a8388599a85..987703ae1fe3 100644
--- a/drivers/cxl/core/mbox.c
+++ b/drivers/cxl/core/mbox.c
@@ -258,6 +258,7 @@ static int cxl_mbox_cmd_ctor(struct cxl_mbox_cmd *mbox,
 	*mbox = (struct cxl_mbox_cmd) {
 		.opcode = opcode,
 		.size_in = in_size,
+		.size_out = out_size,
 	};
 
 	if (in_size) {
@@ -274,12 +275,6 @@ static int cxl_mbox_cmd_ctor(struct cxl_mbox_cmd *mbox,
 		}
 	}
 
-	/* Prepare to handle a full payload for variable sized output */
-	if (out_size < 0)
-		mbox->size_out = cxlds->payload_size;
-	else
-		mbox->size_out = out_size;
-
 	if (mbox->size_out) {
 		mbox->payload_out = kvzalloc(mbox->size_out, GFP_KERNEL);
 		if (!mbox->payload_out) {
@@ -320,7 +315,6 @@ static int cxl_to_mem_cmd_raw(struct cxl_mem_command *mem_cmd,
 		.info = {
 			.id = CXL_MEM_COMMAND_ID_RAW,
 			.size_in = send_cmd->in.size,
-			.size_out = send_cmd->out.size,
 		},
 		.opcode = send_cmd->raw.opcode
 	};
@@ -365,7 +359,6 @@ static int cxl_to_mem_cmd(struct cxl_mem_command *mem_cmd,
 			.id = info->id,
 			.flags = info->flags,
 			.size_in = send_cmd->in.size,
-			.size_out = send_cmd->out.size,
 		},
 		.opcode = c->opcode
 	};
@@ -417,6 +410,12 @@ static int cxl_validate_cmd_from_user(struct cxl_mbox_cmd *mbox_cmd,
 	if (rc)
 		return rc;
 
+	/* Prepare to handle a full payload for variable sized output */
+	if (send_cmd->out.size < 0)
+		mem_cmd.info.size_out = cxlds->payload_size;
+	else
+		mem_cmd.info.size_out = send_cmd->out.size;
+
 	/* Sanitize and construct a cxl_mbox_cmd */
 	return cxl_mbox_cmd_ctor(mbox_cmd, cxlds, mem_cmd.opcode,
 				 mem_cmd.info.size_in, mem_cmd.info.size_out,

base-commit: 7dc1d11d7abae52aada5340fb98885f0ddbb7c37
-- 
2.31.1

Re: [PATCH] cxl/mbox: Handle variable size output while still signed

[Dan Williams]

On Fri, Apr 8, 2022 at 8:11 PM <alison.schofield@intel.com> wrote:
>
> From: Alison Schofield <alison.schofield@intel.com>
>
> A mailbox command output size value of (-1) means that a device
> may return a variable sized output for that command. The device
> defines a maximum payload size, so the driver will allocate enough
> memory to receive that maximum payload for these commands.
>
> A recent code refactoring moved the check for variable sized output
> to occur after the signedness is discarded from the output size.
> Move the check to occur while still operating on the signed output
> size variable.
>
> Smatch warn: unsigned 'out_size' is never less than 0
>
> Fixes: be0d0ce77aa3 ("cxl/mbox: Move build of user mailbox cmd to a helper funct
> ions")
> Reported-by: kernel test robot <lkp@intel.com>
> Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
> Signed-off-by: Alison Schofield <alison.schofield@intel.com>
> ---
>  drivers/cxl/core/mbox.c | 15 +++++++--------
>  1 file changed, 7 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/cxl/core/mbox.c b/drivers/cxl/core/mbox.c
> index 8a8388599a85..987703ae1fe3 100644
> --- a/drivers/cxl/core/mbox.c
> +++ b/drivers/cxl/core/mbox.c
> @@ -258,6 +258,7 @@ static int cxl_mbox_cmd_ctor(struct cxl_mbox_cmd *mbox,
>         *mbox = (struct cxl_mbox_cmd) {
>                 .opcode = opcode,
>                 .size_in = in_size,
> +               .size_out = out_size,
>         };
>
>         if (in_size) {
> @@ -274,12 +275,6 @@ static int cxl_mbox_cmd_ctor(struct cxl_mbox_cmd *mbox,
>                 }
>         }
>
> -       /* Prepare to handle a full payload for variable sized output */
> -       if (out_size < 0)
> -               mbox->size_out = cxlds->payload_size;
> -       else
> -               mbox->size_out = out_size;
> -

Why not?

diff --git a/drivers/cxl/core/mbox.c b/drivers/cxl/core/mbox.c
index 8a8388599a85..6c6f6a157485 100644
--- a/drivers/cxl/core/mbox.c
+++ b/drivers/cxl/core/mbox.c
@@ -253,7 +253,7 @@ static bool cxl_payload_from_user_allowed(u16
opcode, void *payload_in)

 static int cxl_mbox_cmd_ctor(struct cxl_mbox_cmd *mbox,
                             struct cxl_dev_state *cxlds, u16 opcode,
-                            size_t in_size, size_t out_size, u64 in_payload)
+                            ssize_t in_size, ssize_t out_size, u64 in_payload)
 {
        *mbox = (struct cxl_mbox_cmd) {
                .opcode = opcode,


...and then the rest of this patch becomes unnecessary.