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

[Alison Schofield <alison.schofield@intel.com>]

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
>