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

在 2026/8/12 16:44, [email protected] 写道:
> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
>
> Pre-existing issues:
> - [High] Zero-initializing `ep_coord` codifies a logic error where valid PCI bandwidth is discarded if CDAT bandwidth is unreported (0), contradicting subsystem conventions.
> - [High] The cxl_port_perf_data_calculate() function declares an uninitialized access_coordinate array on the stack, which is leaked to userspace via sysfs for Restricted CXL Devices (RCDs).
> --
>
> --- Patch [1]: [PATCH v3 1/2] cxl/cdat: Fix uninitialized stack use in bandwidth gathering ---
> commit ecbce27f92f7e67349081889c3ca38eb48761104
> Author: Guixin Liu <[email protected]>
> Subject: cxl/cdat: Fix uninitialized stack use in bandwidth gathering
>
> This commit fixes uninitialized stack usage in cxl_endpoint_gather_bandwidth()
> and cxl_switch_gather_bandwidth() by zero-initializing the access_coordinate
> arrays. It prevents uninitialized kernel stack data from being exposed as
> bandwidth and latency figures.
>
>> diff --git a/drivers/cxl/core/cdat.c b/drivers/cxl/core/cdat.c
>> index 5c9f072625135..df9261ba84e5a 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] = { };
> [Severity: High]
> This isn't a bug introduced by this patch, but does zero-initializing
> ep_coord codify a logic error where valid PCI bandwidth is discarded?
>
> If an endpoint CDAT does not report bandwidth, the bandwidth fields in
> perf->cdat_coord will be 0. When cxl_coordinates_combine() is called later
> in cxl_endpoint_gather_bandwidth():
>
> 	rc = cxl_pci_get_bandwidth(pdev, pci_coord);
> 	if (rc < 0)
> 		return rc;
>
> 	/*
> 	 * Min of upstream link bandwidth and Endpoint CDAT bandwidth from
> 	 * DSLBIS.
> 	 */
> 	cxl_coordinates_combine(ep_coord, pci_coord, perf->cdat_coord);
>
> The __cxl_coordinates_combine() helper requires both inputs to be non-zero
> to assign the output:
>
> 	if (c1->write_bandwidth && c2->write_bandwidth)
> 		out->write_bandwidth = min(c1->write_bandwidth,
> 					   c2->write_bandwidth);
>
> Because the bandwidth in perf->cdat_coord is 0, ep_coord remains untouched
> (which is now 0). This completely discards the valid upstream link
> bandwidth in pci_coord, resulting in an erroneous region aggregate
> bandwidth of 0.
>
> [Severity: High]
> This is a pre-existing issue, but while fixing uninitialized stack
> variables, did you notice a similar leak in cxl_port_perf_data_calculate()
> in drivers/cxl/core/cdat.c?
>
> It declares an uninitialized array:
>
> 	struct access_coordinate ep_c[ACCESS_COORDINATE_MAX];
> 	...
> 	rc = cxl_endpoint_get_perf_coordinates(port, ep_c);
>
> If the device is a Restricted CXL Device (RCD),
> cxl_endpoint_get_perf_coordinates() in drivers/cxl/core/port.c returns 0
> early without initializing the array:
>
> 	/*
> 	 * Skip calculation for RCD. Expectation is HMAT already covers RCD case
> 	 * since RCH does not support hotplug.
> 	 */
> 	if (cxlmd->cxlds->rcd)
> 		return 0;
>
> cxl_port_perf_data_calculate() then passes this uninitialized array to
> cxl_coordinates_combine(), which can leak kernel stack memory to userspace
> via world-readable sysfs region attributes.
On the RCD leak in cxl_port_perf_data_calculate(): that is what patch 2 of
this series fixes. cxl_endpoint_get_perf_coordinates() now zeroes the
caller's array before the early return for RCDs, so ep_c[] is no longer read
before anything writes it. The fix is in the exported helper rather than in
the caller because the helper documents @coord as an output parameter, and
cxl_test discards its return value.

On ep_coord being discarded when the CDAT reports no bandwidth: the guard
that skips the assignment, "if (c1->write_bandwidth && c2->write_bandwidth)"
in __cxl_coordinates_combine(), is pre-existing and untouched by this patch.
What the patch changes is only what remains in the output when that guard
skips: stack residue before, zero now. A zero bandwidth is how that helper
already spells "not reported" - that is why the guard exists, since min()
against an unknown value is meaningless. Making the combine step fall back
to one input instead would change the meaning of the helper for all of its
callers, which is a separate discussion from not reading uninitialized
stack. The cover letter says the same about the aliasing question raised on
v2.

Best Regards,
Guixin Liu