Re: [Patch v4 1/5] perf/x86: Disable LBR if x86_pmu.lbr_nr exceeds MAX_LBR_ENTRIES

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] perf/x86: init_hw_perf_events() disables LBR too late after vendor PMU init
--

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

perf/x86: Disable LBR if x86_pmu.lbr_nr exceeds MAX_LBR_ENTRIES

This commit disables last branch record support in init_hw_perf_events()
if x86_pmu.lbr_nr exceeds MAX_LBR_ENTRIES to prevent out-of-bounds
accesses in the cached branch stack handling path.

> diff --git a/arch/x86/events/core.c b/arch/x86/events/core.c
> index 4df792b139e05..002a04c149c29 100644
> --- a/arch/x86/events/core.c
> +++ b/arch/x86/events/core.c
> @@ -2645,6 +2645,12 @@ static int __init init_hw_perf_events(void)
>  	if (!x86_pmu.config_mask)
>  		x86_pmu.config_mask = X86_RAW_EVENT_MASK;
>  
> +	if (x86_pmu.lbr_nr > MAX_LBR_ENTRIES) {
> +		pr_warn("LBR entries %d exceeds max %d, disable LBR.\n",
> +			x86_pmu.lbr_nr, MAX_LBR_ENTRIES);
> +		x86_pmu.lbr_nr = 0;
> +	}
> +
>  	perf_events_lapic_init();
>  	register_nmi_handler(NMI_LOCAL, perf_event_nmi_handler, 0, "PMI");

[Severity: Medium]
Does zeroing x86_pmu.lbr_nr here in init_hw_perf_events() happen too late,
after intel_pmu_init() and amd_pmu_init() have already configured hardware
and driver state using the unvalidated x86_pmu.lbr_nr value?

In arch/x86/events/intel/lbr.c:intel_pmu_arch_lbr_init(), if lbr_nr exceeds
MAX_LBR_ENTRIES, initialization proceeds instead of taking the clear_arch_lbr
path:

	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;
	...
	x86_pmu.lbr_counters = ecx.split.lbr_counters;
	x86_pmu.lbr_nr = lbr_nr;

	if (!!x86_pmu.lbr_counters)
		x86_pmu.flags |= PMU_FL_BR_CNTR | PMU_FL_DYN_CONSTRAINT;

Because clear_arch_lbr is skipped, X86_FEATURE_ARCH_LBR remains enabled,
MSR_ARCH_LBR_DEPTH is written, pmu->task_ctx_cache is allocated via
create_lbr_kmem_cache(), and x86_pmu.lbr_ctl_map and static branches are
configured.

Subsequently, arch/x86/events/intel/lbr.c:reserve_lbr_buffers() checks the
feature bit and task_ctx_cache rather than x86_pmu.lbr_nr:

	if (!cpu_feature_enabled(X86_FEATURE_ARCH_LBR))
		return;

	for_each_possible_cpu(cpu) {
		cpuc = per_cpu_ptr(&cpu_hw_events, cpu);
		kmem_cache = x86_get_pmu(cpu)->task_ctx_cache;
		if (!kmem_cache || cpuc->lbr_xsave)
			continue;

		cpuc->lbr_xsave = kmem_cache_alloc_node(kmem_cache,
							GFP_KERNEL | __GFP_ZERO,
							cpu_to_node(cpu));
	}

Wouldn't this still allocate per-cpu cpuc->lbr_xsave buffers on hardware
reservation even after x86_pmu.lbr_nr is set to 0?

Similarly, in arch/x86/events/intel/core.c:lbr_is_visible(), leaving
PMU_FL_BR_CNTR set in x86_pmu.flags keeps branch_counter_nr and
branch_counter_width visible in sysfs when x86_pmu.lbr_nr is 0:

	/* branches */
	if (i == 0)
		return x86_pmu.lbr_nr ? attr->mode : 0;

	return (x86_pmu.flags & PMU_FL_BR_CNTR) ? attr->mode : 0;

In addition, both arch/x86/events/intel/core.c:intel_pmu_init() and
arch/x86/events/amd/core.c:amd_core_pmu_init() check x86_pmu.lbr_nr before
init_hw_perf_events() clears it:

	if (x86_pmu.lbr_nr) {
		intel_pmu_lbr_init();

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

		/* only support branch_stack snapshot for perfmon >= v2 */
		if (x86_pmu.disable_all == intel_pmu_disable_all) {
			if (boot_cpu_has(X86_FEATURE_ARCH_LBR)) {
				static_call_update(perf_snapshot_branch_stack,
						   intel_pmu_snapshot_arch_branch_stack);
			} else {
				static_call_update(perf_snapshot_branch_stack,
						   intel_pmu_snapshot_branch_stack);
			}
		}
	}

And in amd_core_pmu_init():

	if (!amd_pmu_lbr_init()) {
		/* LBR requires flushing on context switch */
		x86_pmu.sched_task = amd_pmu_lbr_sched_task;
		static_call_update(amd_pmu_branch_hw_config, amd_pmu_lbr_hw_config);
		static_call_update(amd_pmu_branch_reset, amd_pmu_lbr_reset);
		static_call_update(amd_pmu_branch_add, amd_pmu_lbr_add);
		static_call_update(amd_pmu_branch_del, amd_pmu_lbr_del);

		/* Only support branch_stack snapshot on perfmon v2 */
		if (x86_pmu.handle_irq == amd_pmu_v2_handle_irq)
			static_call_update(perf_snapshot_branch_stack, amd_pmu_v2_snapshot_branch_stack);
	} else if (!amd_brs_init()) {

If bpf_get_branch_snapshot() is later called when x86_pmu.lbr_nr is 0, won't
the registered perf_snapshot_branch_stack callback still run, manipulate
branch and global control registers, and unconditionally call
intel_pmu_enable_all(0) or amd_pmu_v2_enable_all(0)?

Should the MAX_LBR_ENTRIES check be performed before or during the
vendor-specific initialization routines (such as intel_pmu_lbr_init(),
intel_pmu_arch_lbr_init(), and amd_pmu_lbr_init()) so that None of the
hardware state, slab caches, flags, or static calls are enabled when
lbr_nr is out of range?

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