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

[Richard Cheng <icheng@nvidia.com>]

On Thu, Sep 17, 2026 at 06:46:03PM +0800, Gaobin Huang wrote:
> 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.
>

Hi Gaobin,

I have some comments and issue, maybe they can help.
 
> 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.
> 
> 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@semi.ac.cn>
> ---
> 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@samsung.com/
> 
> 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 */
>

I would suggest to keep the signature and do the check here instead.
cxl_get_gsl() issues the command, owns the buffer and already has the kvfree()/
ERR_PTR error path. Then the caller doesn't need to know the header size or
the entry size, and gsl->entries can be trusted by whoever uses the struct.

.min_out = 2 is the only min_out in the tree that's not a full header.
Raising it to struct_size(ret, etnry, 0) lets cxl_internal_send_cmd() reject
the short response with -EIO, so the "too short for the header" part can
go away.


>  	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;
> +	}
> +

The only kvfree() is at "out" section, directly return here would leak
gsl.

> +	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;
> +		}
> +

Agree with Alison and Jonathan, don't clamp here.
CXL r4.0 Section 8.2.10.5.1 Get Supported Logs, Table 8-249 Get Supported Logs
Output Payload, defines "Number of Supported Logs Entries" as "the number of
Supported Log Entries returned in the output payload".
It's not total, so a counter larger than what arrived is a malformed response,
not a partial list.

About -EIO v.s. -ENXIO, I would go with -EIO.
cxl_internal_send_cmd() documents -EIO as "Unexpacted output size",
-ENXIO as "device reported an error", and this is a size mismatch so I think
the former suits better.

Best regards,
Richard Cheng.

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