Re: [PATCH v2] cxl/cdat: Fix uninitialized stack use in endpoint bandwidth gathering

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

Pre-existing issues:
- [High] Uninitialized stack memory use in cxl_switch_gather_bandwidth() during access coordinate combination.
- [High] Uninitialized stack memory leak into QoS class calculation for Restricted CXL Devices (RCDs).
- [Medium] Pointer aliasing in `cxl_coordinates_combine()` causes bandwidth calculations to incorrectly ignore zero bandwidths from downstream hops.
--

--- Patch [1]: [PATCH v2] cxl/cdat: Fix uninitialized stack use in endpoint bandwidth gathering ---
commit 1627473a040a741d578311d820959582436220a3
Author: Guixin Liu <[email protected]>

cxl/cdat: Fix uninitialized stack use in endpoint bandwidth gathering

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.

Link: https://lore.kernel.org/linux-cxl/[email protected]/

> diff --git a/drivers/cxl/core/cdat.c b/drivers/cxl/core/cdat.c
> index 5c9f072625135..3c6a1537f89b8 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);

[Severity: High]
This is a pre-existing issue, but cxl_switch_gather_bandwidth() has a similar
uninitialized stack array:

drivers/cxl/core/cdat.c:cxl_switch_gather_bandwidth() {
    ...
    struct access_coordinate coords[ACCESS_COORDINATE_MAX];
    ...
    rc = cxl_pci_get_bandwidth(pdev, coords);
    ...
    cxl_coordinates_combine(coords, coords, ctx->coord);
    ...
}

cxl_pci_get_bandwidth() leaves latency members uninitialized, and then
cxl_coordinates_combine() unconditionally sums them.

Should coords be zero-initialized here as well to prevent reading
uninitialized memory?

[Severity: High]
This is a pre-existing issue, but uninitialized stack memory can leak into QoS
calculations for Restricted CXL Devices (RCDs):

drivers/cxl/core/cdat.c:cxl_port_perf_data_calculate() {
    struct access_coordinate ep_c[ACCESS_COORDINATE_MAX];
    ...
    rc = cxl_endpoint_get_perf_coordinates(port, ep_c);
    ...
    cxl_coordinates_combine(dent->coord, dent->cdat_coord, ep_c);
    ...
}

For RCDs, cxl_endpoint_get_perf_coordinates() returns 0 early without
modifying the array:

drivers/cxl/core/port.c:cxl_endpoint_get_perf_coordinates() {
    ...
    if (cxlmd->cxlds->rcd)
        return 0;
    ...
}

This causes cxl_coordinates_combine() to merge uninitialized stack data into
the region's coordinates, which is later passed to QoS class calculation and
exported to userspace sysfs.

Should ep_c also be zero-initialized?

[Severity: Medium]
This is a pre-existing issue, but does pointer aliasing in
cxl_coordinates_combine() cause bandwidth calculations to ignore zero
bandwidths from downstream hops?

drivers/cxl/core/cdat.c:__cxl_coordinates_combine() {
    if (c1->write_bandwidth && c2->write_bandwidth)
        out->write_bandwidth = min(c1->write_bandwidth,
                                   c2->write_bandwidth);
}

When called with aliased pointers in cxl_endpoint_gather_bandwidth() and
cxl_switch_gather_bandwidth():

    cxl_coordinates_combine(ep_coord, ep_coord, sw_coord);

If sw_coord has 0 bandwidth, the operation is skipped, and ep_coord retains
its previous non-zero bandwidth instead of being updated to 0.

Should the bandwidth be updated when one of the inputs has 0 bandwidth, or is
retaining the previous value intended?

>  	struct cxl_dev_state *cxlds = cxlmd->cxlds;
>  	struct pci_dev *pdev = to_pci_dev(cxlds->dev);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1