[PATCH] cxl/mbox: bound the device-supplied command return code

[PATCH] cxl/mbox: bound the device-supplied command return code

[Gaobin Huang]

cxl_pci_mbox_send() takes the command return code straight out of the
mailbox status register:

	mbox_cmd->return_code =
		FIELD_GET(CXLDEV_MBOX_STATUS_RET_CODE_MASK, status_reg);

The field is 16 bits wide and nothing validates it.  It is read the same
way from the background command status register when a background
operation completes (CXLDEV_MBOX_BG_CMD_COMMAND_RC_MASK, also 16 bits).
Both consumers then index a table with it:

	return cxl_mbox_cmd_rctable[mbox_cmd->return_code].err;

and that table, built from CMD_CMD_RC_TABLE, has 31 entries (SUCCESS(0)
through EXTLIST(0x1e)).  A device that reports 0x1f or above therefore
reads past the end of a global array.

cxl_mbox_cmd_rc2errno() is called for every non-success, non-background
return code, which makes this reachable during probe with no host action:
answering Get Supported Logs (0x0400) with return code 0x1f is enough.

Reproduced with a QEMU Type-3 device that reports a chosen return code,
against this tree:

	BUG: KASAN: global-out-of-bounds in cxl_internal_send_cmd+0x11d/0x190
	Read of size 4 at addr ffffffffa5212c10 by task kworker/u8:2/37
	Call Trace:
	 cxl_internal_send_cmd+0x11d/0x190
	 cxl_enumerate_cmds+0x12c/0x980
	 cxl_pci_probe+0x84a/0x11e0

Return codes that are in the table take the ordinary error path and
report nothing, which is what makes the out-of-range case attributable
rather than just "the device reported an error".

Validate once, in a helper that both accessors use, rather than at the
two register read sites: the same bound then covers the foreground and
the background path, and any future consumer of the field.  An undefined
code is still a failure, and -ENXIO is what every non-success entry in
the table already maps to, so callers cannot tell the difference in a way
that matters.

Fixes: 92fcc1abab09 ("cxl/mbox: Improve handling of mbox_cmd hw return codes")
Signed-off-by: Gaobin Huang <huanggaobin23@semi.ac.cn>
---
 drivers/cxl/cxlmem.h | 30 ++++++++++++++++++++++++++++--
 1 file changed, 28 insertions(+), 2 deletions(-)

diff --git a/drivers/cxl/cxlmem.h b/drivers/cxl/cxlmem.h
index c401e3a1a..7f89dc4fc 100644
--- a/drivers/cxl/cxlmem.h
+++ b/drivers/cxl/cxlmem.h
@@ -6,6 +6,7 @@
 #include <linux/pci.h>
 #include <linux/cdev.h>
 #include <linux/uuid.h>
+#include <linux/array_size.h>
 #include <linux/node.h>
 #include <cxl/event.h>
 #include <cxl/mailbox.h>
@@ -205,14 +206,39 @@ static const
 struct cxl_mbox_cmd_rc cxl_mbox_cmd_rctable[] ={ CMD_CMD_RC_TABLE };
 #undef C
 
+/*
+ * The return code comes straight out of a device register: the low 16 bits of
+ * CXL_DEV_MAILBOX_STS.ERRNO for a foreground command, and the same width of
+ * CXL_DEV_BG_CMD_STS.RET_CODE for a background one.  Nothing validates it, so
+ * a device that reports an undefined code would index past the end of the
+ * table above with both of the accessors below.
+ */
+static inline const struct cxl_mbox_cmd_rc *
+cxl_mbox_cmd_rc_entry(struct cxl_mbox_cmd *mbox_cmd)
+{
+	if (mbox_cmd->return_code >= ARRAY_SIZE(cxl_mbox_cmd_rctable))
+		return NULL;
+
+	return &cxl_mbox_cmd_rctable[mbox_cmd->return_code];
+}
+
 static inline const char *cxl_mbox_cmd_rc2str(struct cxl_mbox_cmd *mbox_cmd)
 {
-	return cxl_mbox_cmd_rctable[mbox_cmd->return_code].desc;
+	const struct cxl_mbox_cmd_rc *rc = cxl_mbox_cmd_rc_entry(mbox_cmd);
+
+	return rc ? rc->desc : "undefined command return code";
 }
 
 static inline int cxl_mbox_cmd_rc2errno(struct cxl_mbox_cmd *mbox_cmd)
 {
-	return cxl_mbox_cmd_rctable[mbox_cmd->return_code].err;
+	const struct cxl_mbox_cmd_rc *rc = cxl_mbox_cmd_rc_entry(mbox_cmd);
+
+	/*
+	 * Unknown codes are still failures, and -ENXIO is what every
+	 * non-success entry in the table maps to, so the caller cannot tell
+	 * an undefined code from a defined error in a way that matters.
+	 */
+	return rc ? rc->err : -ENXIO;
 }
 
 /*
base-commit: 999811aca000b0d3d1c838c60dc9db7c72eb0c73
-- 
2.34.1

Re: [PATCH] cxl/mbox: bound the device-supplied command return code

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Low] cxl: incorrect comment about return code mappings
--

commit 2dcefe093dad494dcbd18a284c00e32e26f8041e
Author: Gaobin Huang <huanggaobin23@semi.ac.cn>

cxl/mbox: bound the device-supplied command return code

Validates the device-supplied command return code from the mailbox status
register before indexing the command return code table. This prevents an
out-of-bounds read if a device reports an undefined code.

> diff --git a/drivers/cxl/cxlmem.h b/drivers/cxl/cxlmem.h
> index c401e3a1af06f..7f89dc4fcf050 100644
> --- a/drivers/cxl/cxlmem.h
> +++ b/drivers/cxl/cxlmem.h

[ ... ]

>  static inline int cxl_mbox_cmd_rc2errno(struct cxl_mbox_cmd *mbox_cmd)
>  {
> -	return cxl_mbox_cmd_rctable[mbox_cmd->return_code].err;
> +	const struct cxl_mbox_cmd_rc *rc = cxl_mbox_cmd_rc_entry(mbox_cmd);
> +
> +	/*
> +	 * Unknown codes are still failures, and -ENXIO is what every
> +	 * non-success entry in the table maps to, so the caller cannot tell
> +	 * an undefined code from a defined error in a way that matters.
> +	 */

[Severity: Low]
This isn't a bug, but in cxl_mbox_cmd_rc2errno(), is this comment accurate
regarding the table mappings?

While most non-success entries map to -ENXIO, PADDR maps to -EFAULT and
POISONLMT maps to -EBUSY. The fallback to -ENXIO is safe, but the comment
and the commit message claim that every non-success entry maps to -ENXIO,
which could be confusing.

> +	return rc ? rc->err : -ENXIO;
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260929060747.3550005-1-huanggaobin23@semi.ac.cn?part=1