[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

[Alison Schofield]

On Tue, Sep 29, 2026 at 02:07:47PM +0800, Gaobin Huang wrote:
> 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.
> 


Hi Gaobin,

Thanks for the patch!

I'm taking this into a series that adds CXL KUnit tests, including a regression
test for this table. You'll get CC'd when it posts and I'll need you ACK it
since it has your original authorship with changes from me added.

(Sashiko is right here. PADDR and POISONLMT don't map to -ENXIO, so the comment
and changelog claim isn't accurate. -ENXIO is still the right fallback for an
unknown code.)

It looks like the return code you saw is valid for CXL 4.0 and just not added to
CXL driver yet. I don't think anyone else has sent a patch for that yet,
so you can if you like.

This is what I'm looking at:
CXL 4.0, Table 8-208 "CXL-defined Command Return Codes" (section 8.2.9.4.5.1):
001Fh and 0020h

-- Alison




> 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: Re: [PATCH] cxl/mbox: bound the device-supplied command return code

[黄高彬]

Hi Alison,

Thank you for picking it up, and for the corrections.

> (Sashiko is right here. PADDR and POISONLMT don't map to -ENXIO, so the
> comment and changelog claim isn't accurate. -ENXIO is still the right
> fallback for an unknown code.)

You are both right: the claim should be that an unknown code is still a failure
and that -ENXIO keeps it in the usual device-error class -- not that every
non-success entry maps to -ENXIO.  Please use that wording in the version you
carry; I will not repost the patch on its own.

> I'm taking this into a series that adds CXL KUnit tests, including a
> regression test for this table. You'll get CC'd when it posts and I'll need
> you ACK it since it has your original authorship with changes from me added.

Understood -- I will ACK as soon as it posts.

> It looks like the return code you saw is valid for CXL 4.0 and just not added
> to CXL driver yet. ... CXL 4.0, Table 8-208 ... 001Fh and 0020h
> I don't think anyone else has sent a patch for that yet, so you can if you
> like.

Two things there, and I would rather not collide with your series:

- The KASAN reproduction used 0x1f, which stops being out of range once 001Fh is
  in the table.  Out of range is any code past the end of the table, so with
  001Fh and 0020h added it is 0021h upwards -- the bound itself is unchanged.
- Adding the two entries is a separate change from the bound.  Do you want it in
  your series next to the KUnit test, so the boundary and the test move
  together, or as a standalone patch from me after yours?  Either is fine.  If
  you would rather have it from me, I will take the names and errno mappings
  from Table 8-208 and send it separately.

Thanks,
Gaobin

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

[Alison Schofield]

On Wed, Oct 14, 2026 at 10:48:51PM +0800, 黄高彬 wrote:
> Hi Alison,
> 
> Thank you for picking it up, and for the corrections.
> 
> > (Sashiko is right here. PADDR and POISONLMT don't map to -ENXIO, so the
> > comment and changelog claim isn't accurate. -ENXIO is still the right
> > fallback for an unknown code.)
> 
> You are both right: the claim should be that an unknown code is still a failure
> and that -ENXIO keeps it in the usual device-error class -- not that every
> non-success entry maps to -ENXIO.  Please use that wording in the version you
> carry; I will not repost the patch on its own.

The version in the v1 cxl-kunit patchset[1] already avoids that claim. It reports
unknown codes as -ENXIO, "the errno used for most device-reported errors".


> 
> > I'm taking this into a series that adds CXL KUnit tests, including a
> > regression test for this table. You'll get CC'd when it posts and I'll need
> > you ACK it since it has your original authorship with changes from me added.
> 
> Understood -- I will ACK as soon as it posts.
> 
> > It looks like the return code you saw is valid for CXL 4.0 and just not added
> > to CXL driver yet. ... CXL 4.0, Table 8-208 ... 001Fh and 0020h
> > I don't think anyone else has sent a patch for that yet, so you can if you
> > like.
> 
> Two things there, and I would rather not collide with your series:
> 
> - The KASAN reproduction used 0x1f, which stops being out of range once 001Fh is
>   in the table.  Out of range is any code past the end of the table, so with
>   001Fh and 0020h added it is 0021h upwards -- the bound itself is unchanged.
> - Adding the two entries is a separate change from the bound.  Do you want it in
>   your series next to the KUnit test, so the boundary and the test move
>   together, or as a standalone patch from me after yours?  Either is fine.  If
>   you would rather have it from me, I will take the names and errno mappings
>   from Table 8-208 and send it separately.

Standalone patch from you please. It doesn't need to wait on the series.
The table can be extended without touching the test.

The test has 2 cases that read the table:
- Walk every code in the table and check each errno return. It loops up
  to the table size, so the new entries your patch adds will get picked
  up automatically.

- Checks codes past the end of the table. Its first code is the table 
  size itself, so today it's 001Fh and w your patch it'll be 0021h.

The test only needs a change if a new code maps to an errno other than
-ENXIO.

This conversation made me aware that the testing page could say more
about maintenance of the unit tests. Take a look here [2]


[1] https://lore.kernel.org/linux-cxl/3ed8f5db3f21c937d8783f31ee6faca3703ba72a.1791949910.git.alison.schofield@intel.com/
[2] https://lore.kernel.org/linux-cxl/as_1dGrwsPYTlZrp@aschofie-mobl2.lan/


-- Alison


> 
> Thanks,
> Gaobin