diff mbox series

[v2] cxl/mbox: Restore signedness to a mailbox payload variable

Message ID 20220413191311.1242361-1-alison.schofield@intel.com
State New, archived
Headers show
Series [v2] cxl/mbox: Restore signedness to a mailbox payload variable | expand

Commit Message

Alison Schofield April 13, 2022, 7:13 p.m. UTC 
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 to that command. The device
defines a maximum payload size, so the driver will allocate enough
memory to receive that maximum payload for variable size mailbox
commands.

A recent code refactoring discarded the signedness on 'out_size'
before checking it, thereby breaking the handling of mailbox
commands with variable sized output. Pass along both in_size and
out_size with signedness, and only discard the signedness when
building the cxl_mbox_cmd structure.

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

Changes in v2:
- Pass the payload sizes as signed rather than move the check (Dan)
- Signedness is now discarded on ssize_t to size_t assignment.
- Update commit msg. Was 'Handle variable size output while still
  signed'
- Update commit log.

 drivers/cxl/core/mbox.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)


base-commit: 7dc1d11d7abae52aada5340fb98885f0ddbb7c37

Comments

Ben Widawsky April 13, 2022, 8:29 p.m. UTC | #1 
On 22-04-13 12:13:11, 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 to that command. The device
> defines a maximum payload size, so the driver will allocate enough
> memory to receive that maximum payload for variable size mailbox
> commands.
> 
> A recent code refactoring discarded the signedness on 'out_size'
> before checking it, thereby breaking the handling of mailbox
> commands with variable sized output. Pass along both in_size and
> out_size with signedness, and only discard the signedness when
> building the cxl_mbox_cmd structure.
> 
> 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>

Per internal discussion, do we want to change in_size also? I'm in favor of
fixing the ABI.

[snip]
Alison Schofield April 13, 2022, 9:39 p.m. UTC | #2 
On Wed, Apr 13, 2022 at 01:29:02PM -0700, Ben Widawsky wrote:
> On 22-04-13 12:13:11, 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 to that command. The device
> > defines a maximum payload size, so the driver will allocate enough
> > memory to receive that maximum payload for variable size mailbox
> > commands.
> > 
> > A recent code refactoring discarded the signedness on 'out_size'
> > before checking it, thereby breaking the handling of mailbox
> > commands with variable sized output. Pass along both in_size and
> > out_size with signedness, and only discard the signedness when
> > building the cxl_mbox_cmd structure.
> > 
> > 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>
> 
> Per internal discussion, do we want to change in_size also? I'm in favor of
> fixing the ABI.
> 
Thanks for reviewing!
OK - next version will removed the signedness from the ABI.
Compare to UINT_MAX for the variable size option.

> [snip]
>
diff mbox series

Patch

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,