Re: [PATCH v3 1/2] cxl/cdat: Fix uninitialized stack use in bandwidth gathering

[Alison Schofield <alison.schofield@intel.com>]

On Thu, Aug 27, 2026 at 05:49:47PM -0700, Alison Schofield wrote:
> On Wed, Aug 12, 2026 at 04:30:34PM +0800, Guixin Liu wrote:
> > cxl_endpoint_gather_bandwidth() declares three access_coordinate arrays on
> > the stack - pci_coord, sw_coord and ep_coord - without initializing them,
> > and relies on its helpers to fill every member. None of them does.
> > cxl_pci_get_bandwidth() and cxl_port_get_switch_dport_bandwidth() assign
> > only read_bandwidth and write_bandwidth, and __cxl_coordinates_combine()
> > assigns an output bandwidth only when both input bandwidths are non-zero.
> > Every member a producer declines to set is read back as whatever was on the
> > stack. cxl_switch_gather_bandwidth() has the same defect in its coords
> > array, which it hands to cxl_pci_get_bandwidth() and then to
> > cxl_coordinates_combine().
> > 
> > Both cases occur on real topologies. The latency members are never written
> > by any of those producers, yet __cxl_coordinates_combine() sums them
> > unconditionally, so the latency reads are undefined on every call. And a
> > device whose CDAT DSLBIS reports no bandwidth for an access class leaves
> > perf->cdat_coord zero for that class, which is exactly the condition that
> > makes __cxl_coordinates_combine() skip the bandwidth assignment and leave
> > the ep_coord entry untouched.
> > 
> > The ep_coord case escapes the function: cxl_bandwidth_add() accumulates it
> > into the per-upstream-port aggregate that is published through the region's
> > access coordinate sysfs attributes, so stack contents are reported to
> > userspace as a bandwidth figure. The latency sums are discarded by
> > cxl_bandwidth_add() rather than published, but they are still computed from
> > uninitialized memory.
> > 
> > Zero initialize the four arrays. Zero is already the value this code uses
> > for "not reported" - both the __cxl_coordinates_combine() guard and
> > coordinates_valid() test for it - so a member no producer sets now reads
> > back as unknown rather than as a plausible number.
> 
> 
> Hi Guixin Liu,
> 
> Please see my comments on the einj_inject patch about commit message
> clarity and level of detail. The same issue applies here.
> https://lore.kernel.org/linux-cxl/apDXSzSZVcjQ027J@aschofie-mobl2.lan/
> 
> For this patch, I think the essential story is:

Oops, I just criss-crossed your 2 patches. This goes with Patch 2/2.
Similar feedback on both patches.
Thanks,

-- Alison




> 
> Background:
> cxl_endpoint_get_perf_coordinates() returns success for RCDs without
> calculating coordinates.
> 
> Issue:
> The output array is left uninitialized on that path.
> 
> Impact:
> Callers can consume and expose garbage access-coordinate values.
> 
> Resolution:
> Initialize the coordinates to zero before returning for an RCD.
> 
> That is enough to explain why the patch is needed. I do not need the
> full path through CDAT parsing, __cxl_coordinates_combine(), QoS class
> selection, cxl_dpa_perf, and sysfs to understand the bug.
> 
> Also, as with the other patch, please say how this was found and how it
> was tested.
> 
> -- Alison
> 
> 
> > 
> > Fixes: a5ab0de0ebaa ("cxl: Calculate region bandwidth of targets with shared upstream link")
> > Signed-off-by: Guixin Liu <kanie@linux.alibaba.com>
> > ---
> >  drivers/cxl/core/cdat.c | 8 ++++----
> >  1 file changed, 4 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/cxl/core/cdat.c b/drivers/cxl/core/cdat.c
> > index 5c9f07262513..df9261ba84e5 100644
> > --- a/drivers/cxl/core/cdat.c
> > +++ b/drivers/cxl/core/cdat.c
> > @@ -633,9 +633,9 @@ static int cxl_endpoint_gather_bandwidth(struct cxl_region *cxlr,
> >  	struct cxl_port *endpoint = to_cxl_port(cxled->cxld.dev.parent);
> >  	struct cxl_port *parent_port = to_cxl_port(endpoint->dev.parent);
> >  	struct cxl_port *gp_port = to_cxl_port(parent_port->dev.parent);
> > -	struct access_coordinate pci_coord[ACCESS_COORDINATE_MAX];
> > -	struct access_coordinate sw_coord[ACCESS_COORDINATE_MAX];
> > -	struct access_coordinate ep_coord[ACCESS_COORDINATE_MAX];
> > +	struct access_coordinate pci_coord[ACCESS_COORDINATE_MAX] = { };
> > +	struct access_coordinate sw_coord[ACCESS_COORDINATE_MAX] = { };
> > +	struct access_coordinate ep_coord[ACCESS_COORDINATE_MAX] = { };
> >  	struct cxl_memdev *cxlmd = cxled_to_memdev(cxled);
> >  	struct cxl_dev_state *cxlds = cxlmd->cxlds;
> >  	struct pci_dev *pdev = to_pci_dev(cxlds->dev);
> > @@ -757,7 +757,7 @@ static struct xarray *cxl_switch_gather_bandwidth(struct cxl_region *cxlr,
> >  {
> >  	struct xarray *res_xa __free(free_perf_xa) =
> >  		kzalloc_obj(*res_xa);
> > -	struct access_coordinate coords[ACCESS_COORDINATE_MAX];
> > +	struct access_coordinate coords[ACCESS_COORDINATE_MAX] = { };
> >  	struct cxl_perf_ctx *ctx, *us_ctx;
> >  	unsigned long index, us_index;
> >  	int dev_count = 0;
> > -- 
> > 2.43.7
> > 
>