Re: [Patch v3 1/4] perf/x86: Clamp x86_pmu.lbr_nr to MAX_LBR_ENTRIES

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] perf/x86: late x86_pmu.lbr_nr clamp in init_hw_perf_events() risks #GP
--

commit 7f534b76717bd0a7a33fa3404815780334bcf485
Author: Dapeng Mi <dapeng1.mi@linux.intel.com>

perf/x86: Clamp x86_pmu.lbr_nr to MAX_LBR_ENTRIES

This commit clamps x86_pmu.lbr_nr to MAX_LBR_ENTRIES in
init_hw_perf_events() to prevent out-of-bounds accesses to the cached
LBR stack.

> diff --git a/arch/x86/events/core.c b/arch/x86/events/core.c
> index 4df792b139e05..82f6b66db7e2f 100644
> --- a/arch/x86/events/core.c
> +++ b/arch/x86/events/core.c
> @@ -2645,6 +2645,8 @@ static int __init init_hw_perf_events(void)
>  	if (!x86_pmu.config_mask)
>  		x86_pmu.config_mask = X86_RAW_EVENT_MASK;
>  
> +	x86_pmu.lbr_nr = min(x86_pmu.lbr_nr, MAX_LBR_ENTRIES);
> +
>  	perf_events_lapic_init();
>  	register_nmi_handler(NMI_LOCAL, perf_event_nmi_handler, 0, "PMI");

[Severity: High]
Does clamping x86_pmu.lbr_nr after intel_pmu_init() and amd_pmu_init()
return risk an unhandled #GP in intel_pmu_arch_lbr_reset() and leave
the vendor PMU initialization inconsistent with the clamped depth?

For Intel Architectural LBR, supported depths are enumerated as a bitmask
in eax.split.lbr_depth_mask (where each bit indicates support for a
multiple of 8 up to 64). During init_hw_perf_events() -> intel_pmu_init()
-> intel_pmu_arch_lbr_init(), the code selects the highest supported bit
and validates that unclamped depth against MSR_ARCH_LBR_DEPTH:

arch/x86/events/intel/lbr.c:intel_pmu_arch_lbr_init() {
	...
	lbr_nr = fls(eax.split.lbr_depth_mask) * 8;
	if (!lbr_nr)
		goto clear_arch_lbr;

	/* Apply the max depth of Arch LBR */
	if (wrmsrq_safe(MSR_ARCH_LBR_DEPTH, lbr_nr))
		goto clear_arch_lbr;
	...
}

If a CPU or hypervisor advertises a depth greater than 32 in
lbr_depth_mask without also setting the bit for depth 32, clamping
x86_pmu.lbr_nr to MAX_LBR_ENTRIES (32) here leaves x86_pmu.lbr_nr set to
an unsupported depth that was never tested with wrmsrq_safe(). When
cpuhp_setup_state() subsequently runs x86_pmu_starting_cpu() ->
intel_pmu_cpu_starting() -> intel_pmu_lbr_reset() ->
intel_pmu_arch_lbr_reset():

arch/x86/events/intel/lbr.c:intel_pmu_arch_lbr_reset() {
	/* Write to ARCH_LBR_DEPTH MSR, all LBR entries are reset to 0 */
	wrmsrq(MSR_ARCH_LBR_DEPTH, x86_pmu.lbr_nr);
}

won't this unchecked wrmsrq() of 32 to MSR_ARCH_LBR_DEPTH raise a fatal
#GP at boot?

Additionally, because this clamp runs after vendor initialization has
completed:

1. intel_pmu_arch_lbr_init() has already validated the XSAVE size via
   is_arch_lbr_xsave_available() and allocated pmu->task_ctx_cache using
   the unclamped lbr_nr.

2. intel_pmu_init() has already probed MSRs up to the unclamped
   x86_pmu.lbr_nr and logged the unclamped depth:

arch/x86/events/intel/core.c:intel_pmu_init() {
	...
	for (i = 0; i < x86_pmu.lbr_nr; i++) {
		if (!(check_msr(x86_pmu.lbr_from + i, 0xffffUL) &&
		      check_msr(x86_pmu.lbr_to + i, 0xffffUL)))
			x86_pmu.lbr_nr = 0;
	}

	if (x86_pmu.lbr_nr) {
		intel_pmu_lbr_init();

		pr_cont("%d-deep LBR, ", x86_pmu.lbr_nr);
	...
}

3. amd_pmu_lbr_init() has likewise already logged the unclamped depth:

arch/x86/events/amd/lbr.c:amd_pmu_lbr_init() {
	...
	x86_pmu.lbr_nr = ebx.split.lbr_v2_stack_sz;

	pr_cont("%d-deep LBR, ", x86_pmu.lbr_nr);
	...
}

Should the MAX_LBR_ENTRIES limit be enforced at the point where
x86_pmu.lbr_nr (and lbr_depth_mask in intel_pmu_arch_lbr_init()) is
determined inside the vendor initialization paths instead?

>  
 완벽

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261019070836.2972403-1-dapeng1.mi@linux.intel.com?part=1