[PATCH RESEND] cxl/mbox: validate the DCD extent list counts against the payload

[PATCH RESEND] cxl/mbox: validate the DCD extent list counts against the payload

[Gaobin Huang]

__cxl_process_extent_list() trusts two counts from the Get DC Extent List
response: returned_extent_count bounds the loop over the flexible
extents->extent[] array, and total_extent_count decides when the enclosing
do/while is finished.  Neither is compared against what the device
actually returned, and the command is only issued with .min_out = 1.

A device that reports more extents than it delivered walks the loop past
the end of the mailbox buffer.  A device that reports a large total while
returning nothing makes the loop spin forever, because total_read never
reaches total_expected; a stable generation number and total also keep the
existing -EAGAIN check from firing.  The caller cannot recover from that
one: __cxl_process_extent_list() never returns, so the retry loop around
it never runs.

Derive the bound from mbox_cmd.size_out, clamp the claim to it, and fail
with -EIO when the device stops making progress.  min_out is smaller than
the response header, so the subtraction needs the same underflow guard.

Seen with QEMU emulating a device that lies.  The response header is 16
bytes and an extent is 40, so a 2048 byte mailbox buffer holds 50:

	BUG: KASAN: slab-out-of-bounds in cxl_validate_extent+0xca/0x310
	Read of size 2 at addr ffff888005758800 by task sh/1
	 cxl_validate_extent+0xca/0x310
	 cxl_process_extent_list+0x2c1/0x430
	 cxl_region_probe+0xb2b/0xc40
	 which belongs to the cache kmalloc-2k of size 2048

The Read of size 2 is extent->shared_extn_seq.  extent[50] starts at
16 + 50 * 40 = 2016 and the field is 32 bytes into the record, so that
read is the first byte outside the buffer; a claimed count of 50 stays
inside and 51 does not, as the sweep shows.  Reporting
total_extent_count = 100000 with returned_extent_count = 0 instead spins
region bring-up until the guest stops answering console commands; with the
fix it logs "Extent list: no progress after 0/100000" and fails.

This is in the dynamic capacity device series under review (branch
dcd-v6-2025-04-13 of weiny2/linux-kernel, based on 6.15-rc2), so the fix
belongs in that series before it is merged.

Signed-off-by: Gaobin Huang <huanggaobin23@semi.ac.cn>
---
Resend note: the patch is byte-for-byte unchanged.  v1 went to
ira.weiny@intel.com, which no longer exists, so the author it is addressed to
never received it.  The v2 revision of the linux-cxl patch (drop the Fixes:
tag, struct_offset(), fail on a response too short for the header) does not
apply to this one: this code is not in mainline, so there is no commit to point
a Fixes: at, and the bound here already guards the subtraction against wrap.

 drivers/cxl/core/mbox.c | 30 ++++++++++++++++++++++++++++++
 1 file changed, 30 insertions(+)

diff --git a/drivers/cxl/core/mbox.c b/drivers/cxl/core/mbox.c
index 0b51a5d..c12ff86 100644
--- a/drivers/cxl/core/mbox.c
+++ b/drivers/cxl/core/mbox.c
@@ -1764,6 +1764,7 @@ static int __cxl_process_extent_list(struct cxl_endpoint_decoder *cxled)
 	struct device *dev = mds->cxlds.dev;
 	struct cxl_mbox_cmd mbox_cmd;
 	u32 max_extent_count;
+	size_t extents_hdr, max_returned;
 	int latched_rc = 0;
 	bool first = true;
 
@@ -1808,7 +1809,24 @@ static int __cxl_process_extent_list(struct cxl_endpoint_decoder *cxled)
 			first = false;
 		}
 
+		/*
+		 * The returned count is device-supplied: never index
+		 * extent[] past the payload the device actually returned.
+		 * The device also chooses the reported length and may return a
+		 * response shorter than the header (min_out is 1), so derive the
+		 * bound without underflowing.
+		 */
+		extents_hdr = offsetof(struct cxl_mbox_get_extent_out, extent);
+		max_returned = mbox_cmd.size_out > extents_hdr ?
+			       (mbox_cmd.size_out - extents_hdr) /
+			       sizeof(struct cxl_extent) : 0;
 		nr_returned = le32_to_cpu(extents->returned_extent_count);
+		if (nr_returned > max_returned) {
+			dev_warn_ratelimited(dev,
+					     "Extent list: device claimed %u extents but the payload holds %zu\n",
+					     nr_returned, max_returned);
+			nr_returned = max_returned;
+		}
 		total_read += nr_returned;
 		current_total = le32_to_cpu(extents->total_extent_count);
 		current_gen_num = le32_to_cpu(extents->generation_num);
@@ -1823,6 +1841,18 @@ static int __cxl_process_extent_list(struct cxl_endpoint_decoder *cxled)
 			return -EAGAIN;
 		}
 
+		/*
+		 * A device that keeps claiming more extents without ever
+		 * delivering any would otherwise spin this loop forever
+		 * (the outer retry cannot help: this call never returns).
+		 */
+		if (!nr_returned && total_expected > total_read) {
+			dev_warn_ratelimited(dev,
+					     "Extent list: no progress after %u/%u extents; aborting\n",
+					     total_read, total_expected);
+			return -EIO;
+		}
+
 		for (int i = 0; i < nr_returned ; i++) {
 			struct cxl_extent *extent = &extents->extent[i];
 
-- 
2.34.1

Re: [PATCH RESEND] cxl/mbox: validate the DCD extent list counts against the payload

[Jonathan Cameron]

On Thu, 17 Sep 2026 18:46:56 +0800
Gaobin Huang <huanggaobin23@semi.ac.cn> wrote:

> __cxl_process_extent_list() trusts two counts from the Get DC Extent List
> response: returned_extent_count bounds the loop over the flexible
> extents->extent[] array, and total_extent_count decides when the enclosing
> do/while is finished.  Neither is compared against what the device
> actually returned, and the command is only issued with .min_out = 1.
> 
> A device that reports more extents than it delivered walks the loop past
> the end of the mailbox buffer.  A device that reports a large total while
> returning nothing makes the loop spin forever, because total_read never
> reaches total_expected; a stable generation number and total also keep the
> existing -EAGAIN check from firing.  The caller cannot recover from that
> one: __cxl_process_extent_list() never returns, so the retry loop around
> it never runs.
> 
> Derive the bound from mbox_cmd.size_out, clamp the claim to it, and fail
> with -EIO when the device stops making progress.  min_out is smaller than
> the response header, so the subtraction needs the same underflow guard.
> 
> Seen with QEMU emulating a device that lies.  The response header is 16
> bytes and an extent is 40, so a 2048 byte mailbox buffer holds 50:
> 
> 	BUG: KASAN: slab-out-of-bounds in cxl_validate_extent+0xca/0x310
> 	Read of size 2 at addr ffff888005758800 by task sh/1
> 	 cxl_validate_extent+0xca/0x310
> 	 cxl_process_extent_list+0x2c1/0x430
> 	 cxl_region_probe+0xb2b/0xc40
> 	 which belongs to the cache kmalloc-2k of size 2048
> 
> The Read of size 2 is extent->shared_extn_seq.  extent[50] starts at
> 16 + 50 * 40 = 2016 and the field is 32 bytes into the record, so that
> read is the first byte outside the buffer; a claimed count of 50 stays
> inside and 51 does not, as the sweep shows.  Reporting
> total_extent_count = 100000 with returned_extent_count = 0 instead spins
> region bring-up until the guest stops answering console commands; with the
> fix it logs "Extent list: no progress after 0/100000" and fails.
> 
> This is in the dynamic capacity device series under review (branch
> dcd-v6-2025-04-13 of weiny2/linux-kernel, based on 6.15-rc2), so the fix
> belongs in that series before it is merged.
> 
> Signed-off-by: Gaobin Huang <huanggaobin23@semi.ac.cn>
I'd probably just have sent a reply to Anisa's series with a snippet of what
needed changing.  Anyhow, as it is a patch, the above needs cutting down a lot.
The first part about reading too far or looping forever is sufficent without
the details.

Otherwise, similar to Alison's feedback on the other patch. If it's
wrong just error out the moment you know that. No reason to carry on
or clamp value etc.  Broken hardware should fail in a nice obvious fashion
so people notice!

Jonathan

> ---
> Resend note: the patch is byte-for-byte unchanged.  v1 went to
> ira.weiny@intel.com, which no longer exists, so the author it is addressed to
> never received it.  The v2 revision of the linux-cxl patch (drop the Fixes:
> tag, struct_offset(), fail on a response too short for the header) does not
> apply to this one: this code is not in mainline, so there is no commit to point
> a Fixes: at, and the bound here already guards the subtraction against wrap.
> 
>  drivers/cxl/core/mbox.c | 30 ++++++++++++++++++++++++++++++
>  1 file changed, 30 insertions(+)
> 
> diff --git a/drivers/cxl/core/mbox.c b/drivers/cxl/core/mbox.c
> index 0b51a5d..c12ff86 100644
> --- a/drivers/cxl/core/mbox.c
> +++ b/drivers/cxl/core/mbox.c
> @@ -1764,6 +1764,7 @@ static int __cxl_process_extent_list(struct cxl_endpoint_decoder *cxled)
>  	struct device *dev = mds->cxlds.dev;
>  	struct cxl_mbox_cmd mbox_cmd;
>  	u32 max_extent_count;
> +	size_t extents_hdr, max_returned;
>  	int latched_rc = 0;
>  	bool first = true;
>  
> @@ -1808,7 +1809,24 @@ static int __cxl_process_extent_list(struct cxl_endpoint_decoder *cxled)
>  			first = false;
>  		}
>  
> +		/*
> +		 * The returned count is device-supplied: never index
> +		 * extent[] past the payload the device actually returned.
> +		 * The device also chooses the reported length and may return a
> +		 * response shorter than the header (min_out is 1), so derive the
> +		 * bound without underflowing.
> +		 */
> +		extents_hdr = offsetof(struct cxl_mbox_get_extent_out, extent);
> +		max_returned = mbox_cmd.size_out > extents_hdr ?
> +			       (mbox_cmd.size_out - extents_hdr) /
> +			       sizeof(struct cxl_extent) : 0;
>  		nr_returned = le32_to_cpu(extents->returned_extent_count);
> +		if (nr_returned > max_returned) {
> +			dev_warn_ratelimited(dev,
> +					     "Extent list: device claimed %u extents but the payload holds %zu\n",
> +					     nr_returned, max_returned);
> +			nr_returned = max_returned;
> +		}
>  		total_read += nr_returned;
>  		current_total = le32_to_cpu(extents->total_extent_count);
>  		current_gen_num = le32_to_cpu(extents->generation_num);
> @@ -1823,6 +1841,18 @@ static int __cxl_process_extent_list(struct cxl_endpoint_decoder *cxled)
>  			return -EAGAIN;
>  		}
>  
> +		/*
> +		 * A device that keeps claiming more extents without ever
> +		 * delivering any would otherwise spin this loop forever
> +		 * (the outer retry cannot help: this call never returns).
> +		 */
> +		if (!nr_returned && total_expected > total_read) {
> +			dev_warn_ratelimited(dev,
> +					     "Extent list: no progress after %u/%u extents; aborting\n",
> +					     total_read, total_expected);
> +			return -EIO;
> +		}
> +
>  		for (int i = 0; i < nr_returned ; i++) {
>  			struct cxl_extent *extent = &extents->extent[i];
>  

Re: [PATCH RESEND] cxl/mbox: validate the DCD extent list counts against the payload

[Anisa Su]

On Thu, Sep 17, 2026 at 06:46:56PM +0800, Gaobin Huang wrote:
> __cxl_process_extent_list() trusts two counts from the Get DC Extent List
> response: returned_extent_count bounds the loop over the flexible
> extents->extent[] array, and total_extent_count decides when the enclosing
> do/while is finished.  Neither is compared against what the device
> actually returned, and the command is only issued with .min_out = 1.
> 
> A device that reports more extents than it delivered walks the loop past
> the end of the mailbox buffer.  A device that reports a large total while
> returning nothing makes the loop spin forever, because total_read never
> reaches total_expected; a stable generation number and total also keep the
> existing -EAGAIN check from firing.  The caller cannot recover from that
> one: __cxl_process_extent_list() never returns, so the retry loop around
> it never runs.
> 
> Derive the bound from mbox_cmd.size_out, clamp the claim to it, and fail
> with -EIO when the device stops making progress.  min_out is smaller than
> the response header, so the subtraction needs the same underflow guard.
> 
Hello Gaobin,

As Jonathan mentioned in his reply, specific debug info does not need to be
included in the commit message. I would suggest adding this kind of information
to a cover letter.

> Seen with QEMU emulating a device that lies.  The response header is 16
> bytes and an extent is 40, so a 2048 byte mailbox buffer holds 50:
> 
> 	BUG: KASAN: slab-out-of-bounds in cxl_validate_extent+0xca/0x310
> 	Read of size 2 at addr ffff888005758800 by task sh/1
> 	 cxl_validate_extent+0xca/0x310
> 	 cxl_process_extent_list+0x2c1/0x430
> 	 cxl_region_probe+0xb2b/0xc40
> 	 which belongs to the cache kmalloc-2k of size 2048
> 
> The Read of size 2 is extent->shared_extn_seq.  extent[50] starts at
> 16 + 50 * 40 = 2016 and the field is 32 bytes into the record, so that
> read is the first byte outside the buffer; a claimed count of 50 stays
> inside and 51 does not, as the sweep shows.  Reporting
> total_extent_count = 100000 with returned_extent_count = 0 instead spins
> region bring-up until the guest stops answering console commands; with the
> fix it logs "Extent list: no progress after 0/100000" and fails.
> 
> This is in the dynamic capacity device series under review (branch
> dcd-v6-2025-04-13 of weiny2/linux-kernel, based on 6.15-rc2), so the fix
> belongs in that series before it is merged.
> 

Ira has dropped the DCD series and I have continued to work on it. The last
revision is here:
https://lore.kernel.org/linux-cxl/20260625112638.550691-1-anisa.su@samsung.com/

Sashiko reported the same issue on the previous revision:
https://lore.kernel.org/linux-cxl/20260625183203.3042C1F000E9@smtp.kernel.org/
and I have already patched it in my working tree.

I plan to post the next revision after the DCD Prep Series is complete:
https://lore.kernel.org/linux-cxl/20260918203049.7273-1-anisa.su@samsung.com/T/#t

You are welcome to review both. FYI, if you would like to keep up-to-date with
community news, there is a monthly meeting at 11AM PST every 3rd Tuesday of the
month. So the next one is Tuesday October 20 11AM PST. If you would like to
attend, I can forward the invite to you.

> Signed-off-by: Gaobin Huang <huanggaobin23@semi.ac.cn>
> ---
> Resend note: the patch is byte-for-byte unchanged.  v1 went to
> ira.weiny@intel.com, which no longer exists, so the author it is addressed to
> never received it.  The v2 revision of the linux-cxl patch (drop the Fixes:
> tag, struct_offset(), fail on a response too short for the header) does not
> apply to this one: this code is not in mainline, so there is no commit to point
> a Fixes: at, and the bound here already guards the subtraction against wrap.
> 
If the original send failed and did not make it to the mailing list, there's no
need to prefix this patch with RESEND.

Thanks,
Anisa
>  drivers/cxl/core/mbox.c | 30 ++++++++++++++++++++++++++++++
>  1 file changed, 30 insertions(+)
> 
> diff --git a/drivers/cxl/core/mbox.c b/drivers/cxl/core/mbox.c
> index 0b51a5d..c12ff86 100644
> --- a/drivers/cxl/core/mbox.c
> +++ b/drivers/cxl/core/mbox.c
> @@ -1764,6 +1764,7 @@ static int __cxl_process_extent_list(struct cxl_endpoint_decoder *cxled)
>  	struct device *dev = mds->cxlds.dev;
>  	struct cxl_mbox_cmd mbox_cmd;
>  	u32 max_extent_count;
> +	size_t extents_hdr, max_returned;
>  	int latched_rc = 0;
>  	bool first = true;
>  
> @@ -1808,7 +1809,24 @@ static int __cxl_process_extent_list(struct cxl_endpoint_decoder *cxled)
>  			first = false;
>  		}
>  
> +		/*
> +		 * The returned count is device-supplied: never index
> +		 * extent[] past the payload the device actually returned.
> +		 * The device also chooses the reported length and may return a
> +		 * response shorter than the header (min_out is 1), so derive the
> +		 * bound without underflowing.
> +		 */
> +		extents_hdr = offsetof(struct cxl_mbox_get_extent_out, extent);
> +		max_returned = mbox_cmd.size_out > extents_hdr ?
> +			       (mbox_cmd.size_out - extents_hdr) /
> +			       sizeof(struct cxl_extent) : 0;
>  		nr_returned = le32_to_cpu(extents->returned_extent_count);
> +		if (nr_returned > max_returned) {
> +			dev_warn_ratelimited(dev,
> +					     "Extent list: device claimed %u extents but the payload holds %zu\n",
> +					     nr_returned, max_returned);
> +			nr_returned = max_returned;
> +		}
>  		total_read += nr_returned;
>  		current_total = le32_to_cpu(extents->total_extent_count);
>  		current_gen_num = le32_to_cpu(extents->generation_num);
> @@ -1823,6 +1841,18 @@ static int __cxl_process_extent_list(struct cxl_endpoint_decoder *cxled)
>  			return -EAGAIN;
>  		}
>  
> +		/*
> +		 * A device that keeps claiming more extents without ever
> +		 * delivering any would otherwise spin this loop forever
> +		 * (the outer retry cannot help: this call never returns).
> +		 */
> +		if (!nr_returned && total_expected > total_read) {
> +			dev_warn_ratelimited(dev,
> +					     "Extent list: no progress after %u/%u extents; aborting\n",
> +					     total_read, total_expected);
> +			return -EIO;
> +		}
> +
>  		for (int i = 0; i < nr_returned ; i++) {
>  			struct cxl_extent *extent = &extents->extent[i];
>  
> -- 
> 2.34.1
> 

Re: Re: [PATCH RESEND] cxl/mbox: validate the DCD extent list counts against the payload

[黄高彬]

Hi Anisa,

Thank you for the pointer, and sorry for the slow reply -- I had not found
your series.  Since you have the fix in your tree, I am withdrawing this patch
rather than carrying a second copy of it, and I will not repost it.

> Sashiko reported the same issue on the previous revision ... and I have
> already patched it in my working tree.

If it is useful, we have a reproduction from a QEMU Type-3 device that lies in
the Get DC Extent List response, with the boundary measured rather than
inferred.  The response header is 16 bytes and an extent is 40, so a 2048 byte
mailbox buffer holds 50: returned_extent_count = 50 stays inside it, 51 is the
first that reads past the end (KASAN in cxl_validate_extent, Read of size 2,
extent->shared_extn_seq), and 4096 fails the same way.  A device reporting
total_extent_count = 100000 with returned_extent_count = 0 instead spins region
bring-up until the guest stops answering console commands -- that one is not
covered by a count check, because zero extents fit any payload.

Our version ended up doing what I think yours does: reject a count that does
not fit the payload with -EIO, and fail with -EIO when a response makes no
progress.  Yours supersedes it.  Happy to send the sweep as test cases, or to
run it against your series once it is posted.

> If the original send failed and did not make it to the mailing list, there's
> no need to prefix this patch with RESEND.

Noted, thank you.  The list did receive the first posting; what bounced was a
maintainer address in the Cc list, so the RESEND prefix was misleading.  There
is nothing to re-post now in any case.

> I plan to post the next revision after the DCD Prep Series is complete:
> https://lore.kernel.org/linux-cxl/20260918203049.7273-1-anisa.su@samsung.com/T/#t
> You are welcome to review both.

I will follow both.  Jonathan's point earlier in this thread was right as well:
a snippet in the series thread would have been a better shape than a standalone
patch.

One more thing, since we were testing the older `dcd-v6-2025-04-13` branch: a
device that returns one partition and then zero while still claiming more
available makes `cxl_dev_dc_identify()` loop without progress (7.4 million
queries on our QEMU device, and the guest never reaches a shell).  v11 already
guards that with the `rc == 0` check, so there is nothing to report -- just
confirming that the case we can reproduce is covered, and that the zero-progress
shape is reachable rather than theoretical.

> ... monthly meeting at 11AM PST every 3rd Tuesday of the month.  So the next
> one is Tuesday October 20 11AM PST.  If you would like to attend, I can
> forward the invite to you.

Thank you for the offer -- I will pass on the invite for now, but I will keep
following the published notes (https://pmem.io/ndctl/collab/).  If something on
the call needs the author of this series, I am happy to join for that.

Gaobin