[PATCH 0/2] cxl/mbox: bound device supplied counts by the received payload

[PATCH 0/2] cxl/mbox: bound device supplied counts by the received payload

[Gaobin Huang]

The CXL mailbox layer uses counts the device writes inside a response to
index flexible arrays in that same response, without ever comparing them
against the number of bytes the device actually delivered.  Two of those
walks can leave the buffer:

  Patch 1  Get Event Records.  record_count bounds the walk over
           records[], in cxl_mem_get_records_log() and again in
           cxl_clear_event_record().

  Patch 2  Get Supported Logs.  entries bounds the walk over entry[],
           during command enumeration, on every probe.

The number to compare against is already in hand:
__cxl_pci_mbox_send_cmd() stores how many bytes it copied into the driver
buffer in mbox_cmd.size_out.  Both paths derive their bound from it, so
this adds no new trust in the device, it just uses what the driver already
knows.

It matters because the count is read from the same response whose length
the device also reports: a device that reports success and a full length
while writing less leaves the kernel reading the rest of its mailbox
buffer as the count.  Firmware that takes an error path can do that
without any malice, and patch 2 runs at probe time, so no host action is
involved.

The cross-check is not new to this driver.  get_supported_features() in
drivers/cxl/core/features.c already compares the device supplied count
against the retrieved length before touching the entries.

Both were found by fuzzing a QEMU Type-3 device that lies in those two
responses, with a KASAN guest kernel; the reports are in the patches.

Gaobin Huang (2):
  cxl/mbox: clamp the event record count to the received payload
  cxl/mbox: bound the Get Supported Logs entry count by the payload

 drivers/cxl/core/mbox.c | 54 +++++++++++++++++++++++++++++++++++------
 1 file changed, 46 insertions(+), 8 deletions(-)


base-commit: 999811aca000b0d3d1c838c60dc9db7c72eb0c73
-- 
2.34.1

[PATCH 1/2] cxl/mbox: clamp the event record count to the received payload

[Gaobin Huang]

cxl_mem_get_records_log() walks payload->records[] using the record_count
the device wrote into that same payload, and cxl_clear_event_record()
repeats the walk with the same unvalidated value, reading record handles
past the end of the mailbox buffer.  The handle it reads is then handed
back to the device in the Clear Event Records payload.

The command is issued with .size_out = cxl_mbox->payload_size and only

	.min_out = struct_size(payload, records, 0)

enforced, so the count is never compared against what the device actually
returned.  __cxl_pci_mbox_send_cmd() already knows that number: it stores
the bytes it copied into the driver buffer in mbox_cmd.size_out.  Derive
the record bound from it, and pass the validated count to
cxl_clear_event_record() instead of letting it re-read the raw field.

Seen with a QEMU Type-3 device that returns one record in a 2048 byte
buffer while claiming more, so records[16] is the first access outside it:

	BUG: KASAN: slab-out-of-bounds in cxl_clear_event_record+0x1ad/0x2e0
	Read of size 2 at addr ffff888003306834 by task irq/27-0000:35:/58
	 which belongs to the cache kmalloc-2k of size 2048
	The buggy address is located 52 bytes to the right of
	 allocated 2048-byte region

A sweep of the claimed count agrees: 16 stays inside the allocation and 17
does not.  With the clamp in place the same device logs

	Event log '4': device claimed 4096 records but the payload holds 1

and the log is drained normally.

Fixes: 6ebe28f9ec72 ("cxl/mem: Read, trace, and clear events on driver load")
Signed-off-by: Gaobin Huang <huanggaobin23@semi.ac.cn>
---
 drivers/cxl/core/mbox.c | 22 +++++++++++++++++++---
 1 file changed, 19 insertions(+), 3 deletions(-)

diff --git a/drivers/cxl/core/mbox.c b/drivers/cxl/core/mbox.c
index 55828a836..a8f51bf0f 100644
--- a/drivers/cxl/core/mbox.c
+++ b/drivers/cxl/core/mbox.c
@@ -992,11 +992,13 @@ static void __cxl_event_trace_record(struct cxl_memdev *cxlmd,
 
 static int cxl_clear_event_record(struct cxl_memdev_state *mds,
 				  enum cxl_event_log_type log,
-				  struct cxl_get_event_payload *get_pl)
+				  struct cxl_get_event_payload *get_pl,
+				  u16 nr_rec)
 {
 	struct cxl_mailbox *cxl_mbox = &mds->cxlds.cxl_mbox;
 	struct cxl_mbox_clear_event_payload *payload;
-	u16 total = le16_to_cpu(get_pl->record_count);
+	/* count validated by cxl_mem_get_records_log(), not re-read here */
+	u16 total = nr_rec;
 	u8 max_handles = CXL_CLEAR_EVENT_MAX_HANDLES;
 	size_t pl_size = struct_size(payload, handles, max_handles);
 	struct cxl_mbox_cmd mbox_cmd;
@@ -1070,6 +1072,7 @@ static void cxl_mem_get_records_log(struct cxl_memdev_state *mds,
 	struct cxl_get_event_payload *payload;
 	u8 log_type = type;
 	u16 nr_rec;
+	size_t max_recs;
 
 	mutex_lock(&mds->event.log_lock);
 	payload = mds->event.buf;
@@ -1093,7 +1096,20 @@ static void cxl_mem_get_records_log(struct cxl_memdev_state *mds,
 			break;
 		}
 
+		/*
+		 * The record count is device-supplied.  Never walk records[]
+		 * past the payload the device actually returned.
+		 */
+		max_recs = (mbox_cmd.size_out -
+			    offsetof(struct cxl_get_event_payload, records)) /
+			   sizeof(struct cxl_event_record_raw);
 		nr_rec = le16_to_cpu(payload->record_count);
+		if (nr_rec > max_recs) {
+			dev_warn_ratelimited(dev,
+					     "Event log '%d': device claimed %u records but the payload holds %zu\n",
+					     type, nr_rec, max_recs);
+			nr_rec = max_recs;
+		}
 		if (!nr_rec)
 			break;
 
@@ -1104,7 +1120,7 @@ static void cxl_mem_get_records_log(struct cxl_memdev_state *mds,
 		if (payload->flags & CXL_GET_EVENT_FLAG_OVERFLOW)
 			trace_cxl_overflow(cxlmd, type, payload);
 
-		rc = cxl_clear_event_record(mds, type, payload);
+		rc = cxl_clear_event_record(mds, type, payload, nr_rec);
 		if (rc) {
 			dev_err_ratelimited(dev,
 				"Event log '%d': Failed to clear events : %d",
-- 
2.34.1

[PATCH 2/2] 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.

Seen with a QEMU Type-3 device that reports 0xffff entries while writing
one, in a 2048 byte buffer:

	BUG: KASAN: slab-out-of-bounds in cxl_enumerate_cmds+0x1e3/0x980
	Read of size 4 at addr ffff888003707810 by task kworker/u8:2/38
	cxl_enumerate_cmds+0x1e3/0x980
	cxl_pci_probe+0x84a/0x11e0
	 which belongs to the cache kmalloc-2k of size 2048
	The buggy address is located 16 bytes to the right of the
	 allocated region

The Read of size 4 is gsl->entry[i].size.  The buffer can hold 102
entries, so claiming 102 is still inside it and 103 is not, which is what
the sweep shows.  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.

Fixes: 472b1ce6e9d6 ("cxl/mem: Enable commands via CEL")
Signed-off-by: Gaobin Huang <huanggaobin23@semi.ac.cn>
---
 drivers/cxl/core/mbox.c | 32 +++++++++++++++++++++++++++-----
 1 file changed, 27 insertions(+), 5 deletions(-)

diff --git a/drivers/cxl/core/mbox.c b/drivers/cxl/core/mbox.c
index a8f51bf0f..c94439554 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,7 +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 +850,39 @@ 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, 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 may return a
+	 * response as short as the entry count field on its own (min_out is
+	 * 2), so derive the entry count without underflowing.
+	 */
+	gsl_hdr = offsetof(struct cxl_mbox_get_supported_logs, entry);
+	max_entries = gsl_len > gsl_hdr ?
+		      (gsl_len - gsl_hdr) / sizeof(gsl->entry[0]) : 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]))
-- 
2.34.1

Re: [PATCH 1/2] cxl/mbox: clamp the event record count to the received payload

[Jonathan Cameron]

On Mon, 14 Sep 2026 20:57:00 +0800
Gaobin Huang <huanggaobin23@semi.ac.cn> wrote:

> cxl_mem_get_records_log() walks payload->records[] using the record_count
> the device wrote into that same payload, and cxl_clear_event_record()
> repeats the walk with the same unvalidated value, reading record handles
> past the end of the mailbox buffer.  The handle it reads is then handed
> back to the device in the Clear Event Records payload.
> 
> The command is issued with .size_out = cxl_mbox->payload_size and only
> 
> 	.min_out = struct_size(payload, records, 0)
> 
> enforced, so the count is never compared against what the device actually
> returned.  __cxl_pci_mbox_send_cmd() already knows that number: it stores
> the bytes it copied into the driver buffer in mbox_cmd.size_out.  Derive
> the record bound from it, and pass the validated count to
> cxl_clear_event_record() instead of letting it re-read the raw field.
> 
> Seen with a QEMU Type-3 device that returns one record in a 2048 byte
> buffer while claiming more, so records[16] is the first access outside it:
> 
> 	BUG: KASAN: slab-out-of-bounds in cxl_clear_event_record+0x1ad/0x2e0
> 	Read of size 2 at addr ffff888003306834 by task irq/27-0000:35:/58
> 	 which belongs to the cache kmalloc-2k of size 2048
> 	The buggy address is located 52 bytes to the right of
> 	 allocated 2048-byte region
> 
> A sweep of the claimed count agrees: 16 stays inside the allocation and 17
> does not.  With the clamp in place the same device logs
> 
> 	Event log '4': device claimed 4096 records but the payload holds 1
> 
> and the log is drained normally.
> 

Hi Gaobin Huang,

> Fixes: 6ebe28f9ec72 ("cxl/mem: Read, trace, and clear events on driver load")

If I follow the above correctly this hardening against a buggy device?
If that's the case drop the fixes tag.  I'm happy to see this sort
of change go as long as we make it clear what it is.

> Signed-off-by: Gaobin Huang <huanggaobin23@semi.ac.cn>

Minor suggestion inline.

Jonathan

> ---
>  drivers/cxl/core/mbox.c | 22 +++++++++++++++++++---
>  1 file changed, 19 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/cxl/core/mbox.c b/drivers/cxl/core/mbox.c
> index 55828a836..a8f51bf0f 100644
> --- a/drivers/cxl/core/mbox.c
> +++ b/drivers/cxl/core/mbox.c
> @@ -992,11 +992,13 @@ static void __cxl_event_trace_record(struct cxl_memdev *cxlmd,
>  
>  static int cxl_clear_event_record(struct cxl_memdev_state *mds,
>  				  enum cxl_event_log_type log,
> -				  struct cxl_get_event_payload *get_pl)
> +				  struct cxl_get_event_payload *get_pl,
> +				  u16 nr_rec)
>  {
>  	struct cxl_mailbox *cxl_mbox = &mds->cxlds.cxl_mbox;
>  	struct cxl_mbox_clear_event_payload *payload;
> -	u16 total = le16_to_cpu(get_pl->record_count);
> +	/* count validated by cxl_mem_get_records_log(), not re-read here */
> +	u16 total = nr_rec;
>  	u8 max_handles = CXL_CLEAR_EVENT_MAX_HANDLES;
>  	size_t pl_size = struct_size(payload, handles, max_handles);
>  	struct cxl_mbox_cmd mbox_cmd;
> @@ -1070,6 +1072,7 @@ static void cxl_mem_get_records_log(struct cxl_memdev_state *mds,
>  	struct cxl_get_event_payload *payload;
>  	u8 log_type = type;
>  	u16 nr_rec;
> +	size_t max_recs;
>  
>  	mutex_lock(&mds->event.log_lock);
>  	payload = mds->event.buf;
> @@ -1093,7 +1096,20 @@ static void cxl_mem_get_records_log(struct cxl_memdev_state *mds,
>  			break;
>  		}
>  
> +		/*
> +		 * The record count is device-supplied.  Never walk records[]
> +		 * past the payload the device actually returned.
> +		 */
> +		max_recs = (mbox_cmd.size_out -
> +			    offsetof(struct cxl_get_event_payload, records)) /
> +			   sizeof(struct cxl_event_record_raw);

Feels a bit like there should be a function to reverse struct_size() but sadly not.
We do have struct_offset() however so given we are using this access into
payloads, I think we can write this as:

		max_rec = (mbox_cmd.size_out - struct_offset(payload, records) /
			   sizeof(*payload->records);

>  		nr_rec = le16_to_cpu(payload->record_count);
> +		if (nr_rec > max_recs) {
> +			dev_warn_ratelimited(dev,
> +					     "Event log '%d': device claimed %u records but the payload holds %zu\n",
> +					     type, nr_rec, max_recs);
> +			nr_rec = max_recs;
> +		}
>  		if (!nr_rec)
>  			break;
>  
> @@ -1104,7 +1120,7 @@ static void cxl_mem_get_records_log(struct cxl_memdev_state *mds,
>  		if (payload->flags & CXL_GET_EVENT_FLAG_OVERFLOW)
>  			trace_cxl_overflow(cxlmd, type, payload);
>  
> -		rc = cxl_clear_event_record(mds, type, payload);
> +		rc = cxl_clear_event_record(mds, type, payload, nr_rec);
>  		if (rc) {
>  			dev_err_ratelimited(dev,
>  				"Event log '%d': Failed to clear events : %d",

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

[Jonathan Cameron]

On Mon, 14 Sep 2026 20:57:01 +0800
Gaobin Huang <huanggaobin23@semi.ac.cn> 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.
> 
> Seen with a QEMU Type-3 device that reports 0xffff entries while writing
> one, in a 2048 byte buffer:
> 
> 	BUG: KASAN: slab-out-of-bounds in cxl_enumerate_cmds+0x1e3/0x980
> 	Read of size 4 at addr ffff888003707810 by task kworker/u8:2/38
> 	cxl_enumerate_cmds+0x1e3/0x980
> 	cxl_pci_probe+0x84a/0x11e0
> 	 which belongs to the cache kmalloc-2k of size 2048
> 	The buggy address is located 16 bytes to the right of the
> 	 allocated region
> 
> The Read of size 4 is gsl->entry[i].size.  The buffer can hold 102
> entries, so claiming 102 is still inside it and 103 is not, which is what
> the sweep shows.  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.
> 
> Fixes: 472b1ce6e9d6 ("cxl/mem: Enable commands via CEL")

Similar to previous.  So far we aren't treating buggy devices as
something we must harden against.  Where it is simple though we can
do so.

> Signed-off-by: Gaobin Huang <huanggaobin23@semi.ac.cn>
> ---
>  drivers/cxl/core/mbox.c | 32 +++++++++++++++++++++++++++-----
>  1 file changed, 27 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/cxl/core/mbox.c b/drivers/cxl/core/mbox.c
> index a8f51bf0f..c94439554 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,7 +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 */

blank line here.

>  	return ret;
>  }
>  
> @@ -849,18 +850,39 @@ 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, 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 may return a
> +	 * response as short as the entry count field on its own (min_out is
> +	 * 2), so derive the entry count without underflowing.
> +	 */
> +	gsl_hdr = offsetof(struct cxl_mbox_get_supported_logs, entry);

	gsl_hdr_size = struct_offset(gsl, entry);

Note the size is to make it clear this isnt a pointer to the gsl header.

> +	max_entries = gsl_len > gsl_hdr ?
> +		      (gsl_len - gsl_hdr) / sizeof(gsl->entry[0]) : 0;

I'd error out first on it not being big enough for the header.
Nothing else useful is going to happen and we know it will return
an error anyway.

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

Re: [PATCH 1/2] cxl/mbox: clamp the event record count to the received payload

[Anisa Su]

On Mon, Sep 14, 2026 at 08:57:00PM +0800, Gaobin Huang wrote:
> cxl_mem_get_records_log() walks payload->records[] using the record_count
> the device wrote into that same payload, and cxl_clear_event_record()
> repeats the walk with the same unvalidated value, reading record handles
> past the end of the mailbox buffer.  The handle it reads is then handed
> back to the device in the Clear Event Records payload.
> 
> The command is issued with .size_out = cxl_mbox->payload_size and only
> 
> 	.min_out = struct_size(payload, records, 0)
> 
> enforced, so the count is never compared against what the device actually
> returned.  __cxl_pci_mbox_send_cmd() already knows that number: it stores
> the bytes it copied into the driver buffer in mbox_cmd.size_out.  Derive
> the record bound from it, and pass the validated count to
> cxl_clear_event_record() instead of letting it re-read the raw field.
> 
> Seen with a QEMU Type-3 device that returns one record in a 2048 byte
> buffer while claiming more, so records[16] is the first access outside it:
> 
> 	BUG: KASAN: slab-out-of-bounds in cxl_clear_event_record+0x1ad/0x2e0
> 	Read of size 2 at addr ffff888003306834 by task irq/27-0000:35:/58
> 	 which belongs to the cache kmalloc-2k of size 2048
> 	The buggy address is located 52 bytes to the right of
> 	 allocated 2048-byte region
> 
> A sweep of the claimed count agrees: 16 stays inside the allocation and 17
> does not.  With the clamp in place the same device logs
> 
> 	Event log '4': device claimed 4096 records but the payload holds 1
> 
> and the log is drained normally.
> 
> Fixes: 6ebe28f9ec72 ("cxl/mem: Read, trace, and clear events on driver load")
> Signed-off-by: Gaobin Huang <huanggaobin23@semi.ac.cn>
Hi Gaobin,

I sent a similar patch a part of a series while ago and received the general
feedback that the driver does not need special handling for a buggy device:
https://lore.kernel.org/linux-cxl/20260901002912.958-1-anisa.su@samsung.com/T/#m38ed8d6482902f857bceef314133a67937cc012f

However I see from Jonathan's reply that this patch should move forward
with the "Fixes" tag dropped.
If so, I would recommend shortening the commit message to something like
this:
==================
cxl_mem_get_records_log() takes the record count straight out of the Get
Event Records payload and uses it to read from payload->records[].
An oversized count reads past that buffer. Both
__cxl_event_trace_record() and cxl_clear_event_record() use the record
count to iterate oer payload->records[].

Bound the count by mbox_cmd.size_out which is already sanitized by
 __cxl_pci_mbox_send_cmd(). Report a bad record count as an error rather than a
drained log, so a device repeating the bad count does not keep the event
thread re-reading it.

==================
And could you CC me on v2? There are some minor conflicts with the DCD
prep patch I'm working on, so I will need to rebase those.

Thanks,
Anisa
> ---
>  drivers/cxl/core/mbox.c | 22 +++++++++++++++++++---
>  1 file changed, 19 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/cxl/core/mbox.c b/drivers/cxl/core/mbox.c
> index 55828a836..a8f51bf0f 100644
> --- a/drivers/cxl/core/mbox.c
> +++ b/drivers/cxl/core/mbox.c
> @@ -992,11 +992,13 @@ static void __cxl_event_trace_record(struct cxl_memdev *cxlmd,
>  
>  static int cxl_clear_event_record(struct cxl_memdev_state *mds,
>  				  enum cxl_event_log_type log,
> -				  struct cxl_get_event_payload *get_pl)
> +				  struct cxl_get_event_payload *get_pl,
> +				  u16 nr_rec)
>  {
>  	struct cxl_mailbox *cxl_mbox = &mds->cxlds.cxl_mbox;
>  	struct cxl_mbox_clear_event_payload *payload;
> -	u16 total = le16_to_cpu(get_pl->record_count);
> +	/* count validated by cxl_mem_get_records_log(), not re-read here */
> +	u16 total = nr_rec;
>  	u8 max_handles = CXL_CLEAR_EVENT_MAX_HANDLES;
>  	size_t pl_size = struct_size(payload, handles, max_handles);
>  	struct cxl_mbox_cmd mbox_cmd;
> @@ -1070,6 +1072,7 @@ static void cxl_mem_get_records_log(struct cxl_memdev_state *mds,
>  	struct cxl_get_event_payload *payload;
>  	u8 log_type = type;
>  	u16 nr_rec;
> +	size_t max_recs;
>  
>  	mutex_lock(&mds->event.log_lock);
>  	payload = mds->event.buf;
> @@ -1093,7 +1096,20 @@ static void cxl_mem_get_records_log(struct cxl_memdev_state *mds,
>  			break;
>  		}
>  
> +		/*
> +		 * The record count is device-supplied.  Never walk records[]
> +		 * past the payload the device actually returned.
> +		 */
> +		max_recs = (mbox_cmd.size_out -
> +			    offsetof(struct cxl_get_event_payload, records)) /
> +			   sizeof(struct cxl_event_record_raw);
>  		nr_rec = le16_to_cpu(payload->record_count);
> +		if (nr_rec > max_recs) {
> +			dev_warn_ratelimited(dev,
> +					     "Event log '%d': device claimed %u records but the payload holds %zu\n",
> +					     type, nr_rec, max_recs);
> +			nr_rec = max_recs;
> +		}
>  		if (!nr_rec)
>  			break;
>  
> @@ -1104,7 +1120,7 @@ static void cxl_mem_get_records_log(struct cxl_memdev_state *mds,
>  		if (payload->flags & CXL_GET_EVENT_FLAG_OVERFLOW)
>  			trace_cxl_overflow(cxlmd, type, payload);
>  
> -		rc = cxl_clear_event_record(mds, type, payload);
> +		rc = cxl_clear_event_record(mds, type, payload, nr_rec);
>  		if (rc) {
>  			dev_err_ratelimited(dev,
>  				"Event log '%d': Failed to clear events : %d",
> -- 
> 2.34.1
> 

Re: Re: [PATCH 1/2] cxl/mbox: clamp the event record count to the received payload

[黄高彬]

> If I follow the above correctly this hardening against a buggy device?
> If that's the case drop the fixes tag.  I'm happy to see this sort
> of change go as long as we make it clear what it is.

You have followed it correctly: this is hardening against a device that does
not honour the protocol, not a fix for a regression.  An honest device never
reports more records than it returned.

Having gone looking, I am dropping this patch rather than posting it again.
Anisa Su fixed 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/

Same expression for the bound (struct_size() against mbox_cmd.size_out), same
Fixes: commit (6ebe28f9ec72), and hers is the better of the two: it fails the
command instead of 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 being fixed.  Hers also comes with a pass bound for that loop, which
this one did not.  So this is a duplicate and I withdraw it.

Sorry for the noise.

> Feels a bit like there should be a function to reverse struct_size() but sadly not.
> We do have struct_offset() however so given we are using this access into
> payloads, I think we can write this as:
>
> 		max_rec = (mbox_cmd.size_out - struct_offset(payload, records) /
> 			   sizeof(*payload->records);

Thank you -- it is applied to the Get Supported Logs patch, which is not part
of her series and continues as

  [PATCH v2] cxl/mbox: bound the Get Supported Logs entry count by the payload
  https://lore.kernel.org/all/20260917104603.2658529-1-huanggaobin23@semi.ac.cn/

One reading note in case it was a sketch rather than a paste: as written the
parenthesis closes after sizeof(*payload->records), so the division would bind
to struct_offset() alone, and the paren opened before mbox_cmd.size_out is
never closed.  I grouped the subtraction.  No new include is needed:
struct_size() a few lines above comes from the same linux/overflow.h.

Gaobin

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

[黄高彬]

> Similar to previous.  So far we aren't treating buggy devices as
> something we must harden against.  Where it is simple though we can
> do so.

Same framing as 1/2: this is hardening, and the Fixes: tag is dropped with the
commit message saying so rather than leaving it to be inferred.

The patch is now posted on its own as [PATCH v2] -- the event record patch from
the v1 series is withdrawn, because Anisa Su has the same fix in flight from
2026-08-31:

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

> +	*len = mbox_cmd.size_out;	/* bytes actually received */
> 
> blank line here.

Added.

> +	gsl_hdr = offsetof(struct cxl_mbox_get_supported_logs, entry);
> 
> 	gsl_hdr_size = struct_offset(gsl, entry);
> 
> Note the size is to make it clear this isnt a pointer to the gsl header.

Agreed on both; it is struct_offset(gsl, entry) named gsl_hdr_size now.

> +	max_entries = gsl_len > gsl_hdr ?
> +		      (gsl_len - gsl_hdr) / sizeof(gsl->entry[0]) : 0;
> 
> I'd error out first on it not being big enough for the header.
> Nothing else useful is going to happen and we know it will return
> an error anyway.

Done.  That removes the ternary and the underflow it was guarding, so the
comment about wrapping went with it:

	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]);

I used -EIO because that is what cxl_internal_send_cmd() returns for a payload
size mismatch; say if you would rather have -ENXIO and I will change it.  The
case is reachable: min_out is 2 and the header is 8 bytes, so a response of 2
to 7 bytes lands there.

Thanks,
Gaobin

Re: Re: [PATCH 1/2] cxl/mbox: clamp the event record count to the received payload

[黄高彬]

> I sent a similar patch a part of a series while ago and received the general
> feedback that the driver does not need special handling for a buggy device:
> https://lore.kernel.org/linux-cxl/20260901002912.958-1-anisa.su@samsung.com/T/#m38ed8d6482902f857bceef314133a67937cc012f

Thank you for the pointer -- I had not found your series, and having read it I
am withdrawing our patch.  [PATCH v2 2/4] cxl/events: Validate the record count
reported by the device is the same bug in the same function, bounds the count
with the same expression, and carries the same Fixes: commit (6ebe28f9ec72).
It is also the better of the two, which I did not appreciate until I wrote both
loops out side by side:

  - yours fails the command, so the walk stops at the first sign of a bad count;
  - ours clamped nr_rec to what the payload holds and carried on, which leaves
    nr_rec non-zero and lets `} while (nr_rec);` keep issuing Get and Clear
    Event Records to a device that never actually clears them.  Against a device
    that keeps claiming records it does not deliver, that is a hang;
  - your series bounds the per-log loop as well (CXL_EVENT_LOG_MAX_PASSES),
    which ours had nothing for.

Anything that fixes the read but leaves a device able to spin the kernel is not
worth posting, so v2 of our series drops it.

> Report a bad record count as an error rather than a drained log, so a device
> repeating the bad count does not keep the event thread re-reading it.

This is the part I got wrong.  I had argued that clamping "terminates" because a
count of zero takes the existing `if (!nr_rec) break;`.  That only covers the
device that delivers nothing; a device that delivers one record and claims
65535 every time never reaches it.

> And could you CC me on v2? There are some minor conflicts with the DCD
> prep patch I'm working on, so I will need to rebase those.

You are Cc'd on the patch we kept:

  [PATCH v2] cxl/mbox: bound the Get Supported Logs entry count by the payload
  https://lore.kernel.org/all/20260917104603.2658529-1-huanggaobin23@semi.ac.cn/

It is not in your series: it is the Get Supported Logs entry count during
command enumeration, a different function and a different response, so it
should not conflict with your DCD preparatory work.

Two things that may be of use:

- we have an independent reproduction from a QEMU Type-3 device that lies about
  the record count per response, with the boundary measured: a claim of 16 stays
  inside the 2048 byte buffer and 17 is the first that does not, KASAN in
  cxl_clear_event_record.  Happy to send it as supporting evidence, or as a test
  case for your patch.

- one optional suggestion: cxl_clear_event_record() still re-reads
  get_pl->record_count to decide how many handles to send.  Passing the count
  you validated would remove the invariant the two functions have to agree
  about, though with a single static caller it is not load bearing today.  I
  mention it only because it is the one thing our version did differently that
  was not worse.

Gaobin