Re: [PATCH v2] cxl/mbox: bound the Get Supported Logs entry count by the payload

[Alison Schofield]

On Thu, Sep 17, 2026 at 06:46:03PM +0800, Gaobin Huang wrote:

Hi Gaobin,

Nit: CXL subsystem begins subjects w uppercase, ie:
cxl/mbox: Bound the Get...


> cxl_enumerate_cmds() iterates gsl->entries entries of gsl->entry[] using
> the count the device put in the Get Supported Logs response.  The response
> is only checked with .min_out = 2, so a device may report more entries
> than it delivered and the driver reads past the end of the buffer.  This
> is probe time, so it happens on every boot of a machine with such a
> device, without any host action.
> 
> cxl_get_gsl() can already tell the caller how much arrived, because
> __cxl_pci_mbox_send_cmd() records the copied byte count in
> mbox_cmd.size_out.  Derive the number of entries that fit from it and stop
> the loop there.
> Guard the subtraction too: min_out is smaller than the response header, so
> a two byte response would otherwise wrap the size_t arithmetic and leave
> the bound with nothing to do.
> 
> Like 1/2, this is hardening against a device that does not honour the
> protocol rather than a fix for a regression: an honest device never reports
> more entries than it returned, so no existing hardware is affected.

Note that once this patch is applied, the text below the scissors lines
disappears and referencing 1/2 loses meaning. Also, I don't think you
can claim no existing hardware is affected.

> 
> Reproduced on the tree this series is based on with a QEMU Type-3 device
> that reports 0xffff entries while writing one.  The 2048 byte buffer holds
> 102 entries, so a claim of 102 is still inside it and 103 is not:
> 
> 	BUG: KASAN: slab-out-of-bounds in cxl_enumerate_cmds+0x1e1/0x870
> 	Read of size 4 at addr ffff8880036de810 by task kworker/u8:4/48
> 	 which belongs to the cache kmalloc-2k of size 2048
> 	The buggy address is located 16 bytes to the right of
> 
> The Read of size 4 is gsl->entry[i].size.  With the bound in place the
> same device logs
> 
> 	GSL: device claimed 65535 entries but the payload holds 1
> 
> and enumeration continues with the entries that are present.

Commit logs do not require this level of forensics. Something like this
would have give your reviewers a crisp snapshot of the problem, impact, and
resolution:

	The Get Supported Logs response includes a device supplied entry count.
	cxl_enumerate_cmds() uses that count without validating it against the
	returned payload length. A malformed response can cause an out-of-bounds
	read during device probe.
	
	Validate the entry count against the returned payload length before using
	the entries.
	
	Reproduced with QEMU modified to return an inconsistent entry count.
	KASAN reported an out-of-bounds read in cxl_enumerate_cmds().

Notice I use the word validate, which gets me to a question about how
this is implemented and the note below. Below you seem to say you model
after get_supported_features() but that is not what I see in this patch.

Are we intentionally bounding the enumeration and continuing with a partial
response, or should we be validating the device supplied count and rejecting
an inconsistent response, as get_supported_features does?

> 
> This is the cross-check get_supported_features() already performs in
> drivers/cxl/core/features.c, where a device supplied count is compared
> against the retrieved length before the entries are used.
> 
> Signed-off-by: Gaobin Huang <huanggaobin23@xxxxxxxxxx>
> ---
> v1 -> v2.
> 
> 1/2 of the v1 series (the event record count and cxl_clear_event_record) is
> dropped.  Anisa Su posted a fix for the same bug in the same function on
> 2026-08-31:
> 
>   [PATCH v2 2/4] cxl/events: Validate the record count reported by the device
>   https://lore.kernel.org/linux-cxl/20260901002912.958-3-anisa.su@xxxxxxxxxxx/
> 
> It bounds the count with the same expression (struct_size() against
> mbox_cmd.size_out) and carries the same Fixes: commit (6ebe28f9ec72), so this
> is a duplicate, and hers is the better of the two: it fails the command rather
> than clamping, which also bounds the per-log loop.  Clamping leaves nr_rec
> non-zero, so `} while (nr_rec);` keeps issuing Get and Clear Event Records to a
> device that never clears them -- a hang, which is worse than the read it fixes.
> No reason to post both.
> 
> The patch kept here (formerly 2/2) is not in her series: it is the Get
> Supported Logs entry count during command enumeration, a different function and
> a different response.
> 
> Changes from v1 to this patch, from Jonathan Cameron's review:
> - drop the Fixes: tag; this is hardening against a device that does not honour
>   the protocol, not a fix for a regression, and the commit message says so now
>   rather than leaving it to be inferred.
> - use struct_offset(gsl, entry) and name it gsl_hdr_size so it is not read as a
>   pointer to the header.
> - fail the command when the response is shorter than the header instead of
>   deriving a zero bound from the subtraction.  That also removes the ternary
>   and the underflow it was guarding against, so the comment about wrapping went
>   with it.  -EIO because that is what cxl_internal_send_cmd() returns for a
>   payload size mismatch.
> - add the missing blank line before return ret in cxl_get_gsl().
> 
> Anisa Su is on the Cc list, as she asked on the v1 thread.
> 
>  drivers/cxl/core/mbox.c | 39 +++++++++++++++++++++++++++++++++++----
>  1 file changed, 35 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/cxl/core/mbox.c b/drivers/cxl/core/mbox.c
> index 55828a836..e7daa23de 100644
> --- a/drivers/cxl/core/mbox.c
> +++ b/drivers/cxl/core/mbox.c
> @@ -794,7 +794,8 @@ static void cxl_walk_cel(struct cxl_memdev_state *mds, size_t size, u8 *cel)
>  	set_features_cap(cxl_mbox, ro_cmds, wr_cmds);
>  }
>  
> -static struct cxl_mbox_get_supported_logs *cxl_get_gsl(struct cxl_memdev_state *mds)
> +static struct cxl_mbox_get_supported_logs *cxl_get_gsl(struct cxl_memdev_state *mds,
> +						       size_t *len)
>  {
>  	struct cxl_mailbox *cxl_mbox = &mds->cxlds.cxl_mbox;
>  	struct cxl_mbox_get_supported_logs *ret;
> @@ -818,6 +819,7 @@ static struct cxl_mbox_get_supported_logs *cxl_get_gsl(struct cxl_memdev_state *
>  		return ERR_PTR(rc);
>  	}
>  
> +	*len = mbox_cmd.size_out;	/* bytes actually received */

The comment seems unnecessary. size_out already describes what this is.

>  
>  	return ret;
>  }
> @@ -849,18 +851,47 @@ int cxl_enumerate_cmds(struct cxl_memdev_state *mds)
>  	struct cxl_mbox_get_supported_logs *gsl;
>  	struct device *dev = mds->cxlds.dev;
>  	struct cxl_mem_command *cmd;
> +	size_t gsl_len, gsl_hdr_size, max_entries;
>  	int i, rc;
>  
> -	gsl = cxl_get_gsl(mds);
> +	gsl = cxl_get_gsl(mds, &gsl_len);
>  	if (IS_ERR(gsl))
>  		return PTR_ERR(gsl);
>  
> +	/*
> +	 * The device chooses the reported payload length and min_out only
> +	 * requires the entry count field on its own (2 bytes), so a response
> +	 * shorter than the header is reachable.  There is nothing to enumerate
> +	 * in that case: fail rather than derive a bound from an underflowed
> +	 * subtraction.
> +	 */
> +	gsl_hdr_size = struct_offset(gsl, entry);
> +	if (gsl_len < gsl_hdr_size) {
> +		dev_err(dev,
> +		"GSL: response of %zu bytes is too short for the header\n",
> +		gsl_len);
> +		return -EIO;
> +	}


I don't think the comment is needed. The check and error message make the
requirement clear. As written, it is good self-documenting code :)


> +
> +	max_entries = (gsl_len - gsl_hdr_size) / sizeof(gsl->entry[0]);
> +
>  	rc = -ENOENT;
>  	for (i = 0; i < le16_to_cpu(gsl->entries); i++) {
> -		u32 size = le32_to_cpu(gsl->entry[i].size);
> -		uuid_t uuid = gsl->entry[i].uuid;
> +		u32 size;
> +		uuid_t uuid;
>  		u8 *log;
>  
> +		if (i >= max_entries) {
> +			dev_warn_ratelimited(dev,
> +					     "GSL: device claimed %u entries but the payload holds %zu\n",
> +					     le16_to_cpu(gsl->entries),
> +					     max_entries);
> +			break;
> +		}

Question as above. Why not reject the malformed response here?
If there is a reason to salvage entries that fit, explain that in the
commit log.

If the intent is to validate the device supplied count against max_entries,
it seems clearer to validate the count once before entering the loop.


> +
> +		size = le32_to_cpu(gsl->entry[i].size);
> +		uuid = gsl->entry[i].uuid;
> +
>  		dev_dbg(dev, "Found LOG type %pU of size %d", &uuid, size);
>  
>  		if (!uuid_equal(&uuid, &log_uuid[CEL_UUID]))
> 
> base-commit: 999811aca000b0d3d1c838c60dc9db7c72eb0c73
> -- 
> 2.34.1
>