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

在 2026/8/12 14:22, [email protected] 写道:
> 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?
Yes, will be changed too in v3.
>
> [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?
Yes, changed too in v3.
>
> [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?
It is intended.

Best Regards,
Guixin Liu
>>   	struct cxl_dev_state *cxlds = cxlmd->cxlds;
>>   	struct pci_dev *pdev = to_pci_dev(cxlds->dev);