[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

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but EDAC memory repair state storage is improperly gated behind a tracepoint condition, breaking memory repair when tracing is disabled.
--

commit 4e03b22d8d72d66a9d7ecd79d1ff3eb2082c4880
Author: Gaobin Huang <huanggaobin23@semi.ac.cn>

cxl/mbox: clamp the event record count to the received payload

This commit derives the event record bound from the mailbox command's
size_out to clamp the record count, preventing out-of-bounds reads when
the device returns a payload smaller than claimed.

> diff --git a/drivers/cxl/core/mbox.c b/drivers/cxl/core/mbox.c
> index 55828a836c01a..a8f51bf0ffc7c 100644
> --- a/drivers/cxl/core/mbox.c
> +++ b/drivers/cxl/core/mbox.c

[Severity: High]
This isn't a bug introduced by this patch, but while reviewing this file I
noticed EDAC memory repair state storage is improperly gated behind a
tracepoint condition in cxl_event_trace_record().

drivers/cxl/core/mbox.c:cxl_event_trace_record() {
    ...
    if (trace_cxl_general_media_enabled() || trace_cxl_dram_enabled()) {
        ...
        if (event_type == CXL_CPER_EVENT_GEN_MEDIA) {
            if (cxl_store_rec_gen_media((struct cxl_memdev *)cxlmd, evt))
    ...
}

Because memory error records are only stored when tracepoints are active,
doesn't this break EDAC memory repair (like PPR or memory sparing) when
tracing is disabled, since the records will be silently dropped and
operations requiring them will abort with -EINVAL?

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260914125701.1160136-1-huanggaobin23@semi.ac.cn?part=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