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

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

[Gaobin Huang]

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.

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

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

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] cxl/mbox: memory leak of gsl buffer on truncated payload error path
--

commit 7ecb0885e04d5e150bd0769634b9a3391c017244
Author: Gaobin Huang <huanggaobin23@semi.ac.cn>
cxl/mbox: bound the Get Supported Logs entry count by the payload

This commit adds bounds checking for the number of entries in the Get
Supported Logs response against the actual payload size. It prevents
out-of-bounds reads during device probe by ensuring the device doesn't
report more entries than it actually returns.

> diff --git a/drivers/cxl/core/mbox.c b/drivers/cxl/core/mbox.c
> --- a/drivers/cxl/core/mbox.c
> +++ b/drivers/cxl/core/mbox.c
[ ... ]
> @@ -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;

[Severity: Medium]
Does this code leak the `gsl` buffer allocated by cxl_get_gsl()?

cxl_get_gsl() dynamically allocates the `gsl` buffer using kvmalloc() and
returns it. By directly returning -EIO when the payload is shorter than the
header, we bypass the `goto out;` block which contains the necessary
kvfree(gsl) call. Since this occurs on a device probe error path, it leaks
the allocated memory.

As an additional observation, leaving `.min_out = 2` in cxl_get_gsl() forces
higher layers to manually re-validate the payload length here. Setting
`.min_out = 8` (or using struct_size()) in cxl_get_gsl() would allow the CXL
mailbox core to handle this validation natively without requiring these
manual checks.

> +	}

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

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@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 */

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
> 

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

[Jonathan Cameron]

> >  	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.
Seconded. Error out as early as it is convenient to do validation.
Here that is as Alison says before the loop starts.

Thanks,

Jonathan

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

[黄高彬]

Hi Alison,

Thanks for the review, and sorry for the slow turn-around.

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

Validating and rejecting, and Richard's review on this thread moved the check
to where it belongs.  There is no reason to salvage the entries that fit:
CXL r4.0 Table 8-249 defines the count as the number of entries returned in
this payload, not a running total, so a count the payload cannot hold is a
malformed response rather than a partial list.  v3 therefore validates inside
cxl_get_gsl(), which issues the command, owns the buffer and can return the
error itself:

	mbox_cmd = (struct cxl_mbox_cmd) {
		.opcode = CXL_MBOX_OP_GET_SUPPORTED_LOGS,
		.size_out = cxl_mbox->payload_size,
		.payload_out = ret,
		/* At least the header must be valid */
		.min_out = struct_size(ret, entry, 0),
	};
	rc = cxl_internal_send_cmd(cxl_mbox, &mbox_cmd);
	if (rc < 0) {
		kvfree(ret);
		return ERR_PTR(rc);
	}

	max_entries = (mbox_cmd.size_out - struct_size(ret, entry, 0)) /
		      sizeof(ret->entry[0]);
	if (le16_to_cpu(ret->entries) > max_entries) {
		dev_err(mds->cxlds.dev,
			"GSL: device claimed %u entries but the payload holds %zu\n",
			le16_to_cpu(ret->entries), max_entries);
		kvfree(ret);
		return ERR_PTR(-EIO);
	}

cxl_enumerate_cmds() is unchanged now, and no caller can see an entry without
a count it can trust.  Raising min_out to the header also lets
cxl_internal_send_cmd() reject a response too short to hold the count, so the
manual length check -- and the subtraction it had to guard -- are gone.

That also makes the last paragraph of v2's commit message true.  v2 claimed to
be the cross-check get_supported_features() performs, but it bounded the loop
and carried on, which is not what that function does.  Thank you for catching
the contradiction.

One consequence to be explicit about: cxl_enumerate_cmds() is called from
cxl_pci_probe(), so a device whose count does not fit the payload now fails to
probe instead of coming up with the entries that fit.  That is the loud failure
you both asked for.

> 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 :)

The comment is gone.  So is the one on the *len assignment -- that assignment
is gone too, since cxl_get_gsl() now does the validation itself.

> Nit: CXL subsystem begins subjects w uppercase

Capitalized: "cxl/mbox: Bound the Get Supported Logs entry count by the
payload".

> Also, I don't think you can claim no existing hardware is affected.

Dropped -- I cannot rule that out, so the commit message only says what the
response may contain and what the driver does about it.

> Note that once this patch is applied, the text below the scissors lines
> disappears and referencing 1/2 loses meaning.

The changelog now describes v2 -> v3 only, and nothing else refers to the
dropped patch.

> 	The Get Supported Logs response includes a device supplied entry count.
> 	...
> 	Reproduced with QEMU modified to return an inconsistent entry count.
> 	KASAN reported an out-of-bounds read in cxl_enumerate_cmds().

Adopted.  The 102/103 sweep and the boundary arithmetic are gone; the repro is
one sentence.

> Seconded. Error out as early as it is convenient to do validation.
> Here that is as Alison says before the loop starts.

Done, as above.

sashiko also reported the leak on v2, and Richard pointed it out
independently; the early return it concerned is gone with the restructure.

v3:
https://lore.kernel.org/all/20260929060610.3549718-1-huanggaobin23@semi.ac.cn/

Gaobin

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

[Richard Cheng]

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
> 

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

[黄高彬]

Hi Richard,

Thank you, and sorry for the slow turn-around -- both suggestions are in v3,
and the spec pointer settles the clamping-versus-rejecting question rather
than just making rejecting safer.

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

Agreed, and that is where v3 puts it.  cxl_enumerate_cmds() is now unchanged
from mainline:

	mbox_cmd = (struct cxl_mbox_cmd) {
		.opcode = CXL_MBOX_OP_GET_SUPPORTED_LOGS,
		.size_out = cxl_mbox->payload_size,
		.payload_out = ret,
		/* At least the header must be valid */
		.min_out = struct_size(ret, entry, 0),
	};
	rc = cxl_internal_send_cmd(cxl_mbox, &mbox_cmd);
	if (rc < 0) {
		kvfree(ret);
		return ERR_PTR(rc);
	}

	max_entries = (mbox_cmd.size_out - struct_size(ret, entry, 0)) /
		      sizeof(ret->entry[0]);
	if (le16_to_cpu(ret->entries) > max_entries) {
		dev_err(mds->cxlds.dev,
			"GSL: device claimed %u entries but the payload holds %zu\n",
			le16_to_cpu(ret->entries), max_entries);
		kvfree(ret);
		return ERR_PTR(-EIO);
	}

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

Taken.  The subtraction cannot underflow once min_out guarantees the header,
so the guard around it is gone too, and it matches the two users already in
this file (struct_size(payload, records, 0) and struct_size(po, record, 0)).

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

That was mine, in v2.  Moving the check into cxl_get_gsl() removes the path
instead of repairing it.  sashiko reported the same leak on v2.

> Agree with Alison and Jonathan, don't clamp here.
> CXL r4.0 Section 8.2.10.5.1 Get Supported Logs, Table 8-249 ... 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.

Thank you for the citation.  It is in the commit message now, because it is
what makes rejecting the right answer rather than merely the safer one.

> About -EIO v.s. -ENXIO, I would go with -EIO.

-EIO it is, and it is now literally cxl_internal_send_cmd()'s payload size
mismatch error rather than a choice made here.

v3:
https://lore.kernel.org/all/20260929060610.3549718-1-huanggaobin23@semi.ac.cn/

Gaobin