* Re: [PATCH] cxl/region: Validate partition index before array access
2026-04-09 15:44 [PATCH] cxl/region: Validate partition index before array access KobaK
@ 2026-04-09 16:58 ` Dave Jiang
2026-04-09 18:26 ` KobaK
2026-04-11 23:16 ` Alison Schofield
2 siblings, 0 replies; 4+ messages in thread
From: Dave Jiang @ 2026-04-09 16:58 UTC (permalink / raw)
To: KobaK, Dan Williams
Cc: Davidlohr Bueso, Jonathan Cameron, Alison Schofield, Vishal Verma,
Ira Weiny, Li Ming, linux-cxl, linux-kernel
On 4/9/26 8:44 AM, KobaK wrote:
> From: Koba Ko <kobak@nvidia.com>
>
> Check partition index bounds before accessing cxlds->part[] to prevent
> out-of-bounds access when part is -1 or invalid.
>
> The partition index is read from cxled->part without validation. If it's
> negative or exceeds nr_partitions, accessing cxlds->part[part].mode will
> cause out-of-bounds array access.
>
> Fixes: 5ec67596e368 ("cxl/region: Drop goto pattern of construct_region()")
> Signed-off-by: Koba Ko <kobak@nvidia.com>
Was this issue encountered during testing or just by inspection (or AI analysis)? I'm just curious on how this condition is triggered and if a regression unit test needs to be added.
DJ
> ---
> drivers/cxl/core/region.c | 8 ++++++++
> 1 file changed, 8 insertions(+)
>
> diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
> index edc267c6cf77a..6be46636db7ee 100644
> --- a/drivers/cxl/core/region.c
> +++ b/drivers/cxl/core/region.c
> @@ -3712,6 +3712,14 @@ static struct cxl_region *construct_region(struct cxl_root_decoder *cxlrd,
> int rc, part = READ_ONCE(cxled->part);
> struct cxl_region *cxlr;
>
> + if (part < 0 || part >= cxlds->nr_partitions) {
> + dev_err(cxlmd->dev.parent,
> + "%s:%s: invalid partition index %d (max %u)\n",
> + dev_name(&cxlmd->dev), dev_name(&cxled->cxld.dev),
> + part, cxlds->nr_partitions);
> + return ERR_PTR(-ENXIO);
> + }
> +
> do {
> cxlr = __create_region(cxlrd, cxlds->part[part].mode,
> atomic_read(&cxlrd->region_id),
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH] cxl/region: Validate partition index before array access
2026-04-09 15:44 [PATCH] cxl/region: Validate partition index before array access KobaK
2026-04-09 16:58 ` Dave Jiang
2026-04-09 18:26 ` KobaK
@ 2026-04-11 23:16 ` Alison Schofield
2 siblings, 0 replies; 4+ messages in thread
From: Alison Schofield @ 2026-04-11 23:16 UTC (permalink / raw)
To: KobaK
Cc: Dave Jiang, Dan Williams, Davidlohr Bueso, Jonathan Cameron,
Vishal Verma, Ira Weiny, Li Ming, linux-cxl, linux-kernel
On Thu, Apr 09, 2026 at 11:44:45PM +0800, KobaK wrote:
> From: Koba Ko <kobak@nvidia.com>
>
> Check partition index bounds before accessing cxlds->part[] to prevent
> out-of-bounds access when part is -1 or invalid.
>
> The partition index is read from cxled->part without validation. If it's
> negative or exceeds nr_partitions, accessing cxlds->part[part].mode will
> cause out-of-bounds array access.
>
> Fixes: 5ec67596e368 ("cxl/region: Drop goto pattern of construct_region()")
> Signed-off-by: Koba Ko <kobak@nvidia.com>
This bounds check need is overstated. All writers of cxled->part either
initialize it to -1 or assign it from a bounded walk of cxlds->nr_partitions.
There is no path in that code that produces a positive out-of-range index,
and the only invalid state is part == -1. So, let's focus on the -1 case.
(If we did want to defensively guard against out-of-range indices, that
would need to be applied consistently across all accesses of cxlds->part[part],
not just in construct_region(). I don't think that is needed.)
The Fixes: Tag was a bit perplexing. The cited commit does touch any lines
related to partition indexing, at least based on "git show 5ec67596e368".
However, a close look suggests something else is going on.
Commit be5cbd084027 ("cxl: Kill enum cxl_decoder_mode") which previously
reworked this code included the needed check:
+ int rc, part = READ_ONCE(cxled->part);+
+ if (part < 0)
+ return ERR_PTR(-EBUSY);
And that check is indeed preserved in the final lore posting of the patch
referenced in the Fixes: tag:
https://lore.kernel.org/all/20250221013205.126419-1-ming.li@zohomail.com/
The version that landed in the tree however is different:
5ec67596e368 ("cxl/region: Drop goto pattern of construct_region()")
This suggests the check was not removed by that commit directly but instead
lost during a merge.
At this point, I suggest a v2 of 'this' patch that simply restores the
missing check, no added range check, no debug messaging, simply:
+ if (part < 0)
+ return ERR_PTR(-EBUSY);
It would be useful for Ming and DaveJ to take a close look at the merge
to confirm if anything else is off.
> ---
> drivers/cxl/core/region.c | 8 ++++++++
> 1 file changed, 8 insertions(+)
>
> diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
> index edc267c6cf77a..6be46636db7ee 100644
> --- a/drivers/cxl/core/region.c
> +++ b/drivers/cxl/core/region.c
> @@ -3712,6 +3712,14 @@ static struct cxl_region *construct_region(struct cxl_root_decoder *cxlrd,
> int rc, part = READ_ONCE(cxled->part);
> struct cxl_region *cxlr;
>
> + if (part < 0 || part >= cxlds->nr_partitions) {
> + dev_err(cxlmd->dev.parent,
> + "%s:%s: invalid partition index %d (max %u)\n",
> + dev_name(&cxlmd->dev), dev_name(&cxled->cxld.dev),
> + part, cxlds->nr_partitions);
> + return ERR_PTR(-ENXIO);
> + }
> +
> do {
> cxlr = __create_region(cxlrd, cxlds->part[part].mode,
> atomic_read(&cxlrd->region_id),
> --
> 2.43.0
>
^ permalink raw reply [flat|nested] 4+ messages in thread