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

[Anisa Su <anisa.su887@gmail.com>]

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
>