Re: [PATCH v4 03/11] perf/cxl: Fix the counter overflow delta fixup

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

Pre-existing issues:
- [High] Unvalidated `counter_width` from the CXL PMU capability register causes undefined behavior in `GENMASK_ULL`.
--

--- Patch [3]: [PATCH v4 03/11] perf/cxl: Fix the counter overflow delta fixup ---
commit 6424601c16fd758ca929ead4bf2082b98255eee8
Author: Dave Jiang <[email protected]>

perf/cxl: Fix the counter overflow delta fixup

This commit fixes the counter overflow delta fixup by conditioning the fixup
on new_cnt >= prev_cnt instead of relying on the overflow status alone. It
also uses mask + 1 for the period to avoid undefined behavior with a 64-bit
counter width shift.

Closes: https://sashiko.dev/#/patchset/[email protected]?part=1

> --- a/drivers/perf/cxl_pmu.c
> +++ b/drivers/perf/cxl_pmu.c

[ ... ]

> @@ -697,12 +697,16 @@ static void __cxl_pmu_read(struct perf_event *event, bool overflow)
>  	} while (local64_cmpxchg(&hwc->prev_count, prev_cnt, new_cnt) != prev_cnt);
>  
>  	/*
> -	 * If we know an overflow occur then take that into account.
> -	 * Note counter is not reset as that would lose events
> +	 * The mask discards the bit that says the counter wrapped, so a delta of
> +	 * one whole period reads back as 0 - the same as no events at all. Only
> +	 * the overflow status separates them, and new_cnt >= prev_cnt is that
> +	 * case, so add the period back. mask + 1 is 2^counter_width, which comes
> +	 * out as 0 for a 64-bit counter and avoids an undefined 1 << 64.
>  	 */
> -	delta = (new_cnt - prev_cnt) & GENMASK_ULL(info->counter_width - 1, 0);
> -	if (overflow && delta < GENMASK_ULL(info->counter_width - 1, 0))
> -		delta += (1UL << info->counter_width);
> +	mask = GENMASK_ULL(info->counter_width - 1, 0);

[Severity: High]
This isn't a bug introduced by this patch, but could this macro expansion
result in undefined behavior if the hardware provides an invalid counter
width?

The counter_width is extracted directly from a hardware capability register
during initialization:

drivers/perf/cxl_pmu.c:cxl_pmu_parse_caps() {
    ...
    info->counter_width = FIELD_GET(CXL_PMU_CAP_COUNTER_WIDTH_MSK, val);
    ...
}

If a faulty or malicious CXL device provides a counter_width of 0 or greater
than 64, the GENMASK_ULL() macro here in __cxl_pmu_read() will evaluate with
a negative or out-of-bounds shift amount. Since GENMASK_ULL(h, l) produces
undefined behavior when h < l or h >= 64, this could trigger a kernel splat
under UBSAN and potentially lead to a panic.

Should we validate the capability register bounds when parsing it to protect
against untrusted MMIO input?

> +	delta = (new_cnt - prev_cnt) & mask;
> +	if (overflow && new_cnt >= prev_cnt)
> +		delta += mask + 1;
>  
>  	local64_add(delta, &event->count);
>  }

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