Re: [PATCH] cxl/region: Fix out of bounds access in cxl_cancel_auto_attach()

[Li Ming]

在 2026/5/21 14:52, Alison Schofield 写道:
> On Wed, May 20, 2026 at 07:59:21AM -0700, Dave Jiang wrote:
> > On 5/20/26 5:30 AM, Li Ming wrote:
> > > 在 2026/5/20 01:18, Dave Jiang 写道:
> > > > On 5/19/26 6:23 AM, Li Ming wrote:
> > > > > In cxl_cancel_auto_attach(), it assumes cxled->pos is a valid index for
> > > > > accessing p->targets[]. However, cxled->pos can be set to -ENXIO in
> > > > It can be set to other error codes I think? I would just s/-ENXIO/negative errno/
> > > Sure, Will do that.
> > > > > cxl_region_sort_targets() if cxl_calc_interleave_pos() fails. This
> > > > > causes the driver to use a negative index to access p->targets[],
> > > > > resulting in out-of-bounds access.
> > > > >
> > > > > Fix it by walking p->targets[] instead of using cxled->pos directly.
> > > > Does the comment in cxl_region_sort_targets() need to be updated with the new changes?
> > > I'm not sure how to update the comment in cxl_region_sort_targets(). Any suggestion?
> > idk if we should just drop it entirely since the comment is no longer true. At least that second part. Alison?
> I'd like to see it replaced w this so we continue to have the
> debug info, but stop the lie that led to this issue.
>
> /*
>   * Record that sorting failed, but still continue to calc
>   * cxled->pos so that cxl_calc_interleave_pos() emits its
>   * dev_dbg() for every member, which is useful for auto
>   * discovery debug.
>   */
>
> snip
>
> > > > > +static int cxl_region_remove_target(struct device *dev, void *data)
> > > > >    {
> > > > > -    const struct cxl_endpoint_decoder *cxled = data;
> > > > > +    struct cxl_endpoint_decoder *cxled = data;
> > > > >        struct cxl_region_params *p;
> > > > >        struct cxl_region *cxlr;
> > > > > +    int i;
> > > > >          if (!is_cxl_region(dev))
> > > > >            return 0;
> > > > >          cxlr = to_cxl_region(dev);
> > > > >        p = &cxlr->params;
> > > > > -    return p->targets[cxled->pos] == cxled;
> > > > > +    for (i = 0; i < p->nr_targets; i++) {
> > > > > +        if (p->targets[i] == cxled) {
> > > > > +            p->nr_targets--;
> > > > > +            cxled->state = CXL_DECODER_STATE_AUTO;
> > > > > +            cxled->pos = -1;
> > > > > +            p->targets[i] = NULL;
> > > > > +
> > > > > +            return 1;
> > > > > +        }
> > > > > +    }
> Sashiko review looks like it is calling out a valid 'hole' issue above.
> Does the array need to be compacted when we remove an entry that is not
> the last. That would keep nr_targets same as 'first free slot', so
> there are no NULL holes. I think that fix goes in a separate patch.

Good catch, how about using p->interleave_ways instead of p->nr_targets 
as the loop condition? I think it can solve the problem.

BTW, where did you get the Sashiko review? I didn't get any email from it.


Ming