Re: [RFC PATCH 1/3] perf/x86/amd/uncore: Add common PMU helper functions

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [High] uncore_common_ctx_free() frees the PMU context without cancelling its hrtimer or verifying all events are removed, leading to UAF and NULL-pointer dereferences.
- [High] Unconditional double-stop of PMU events underflows ctx->nr_active, permanently breaking the hrtimer.
- [High] Unsanitized event->attr.config is written directly to hardware MSRs, enabling kernel panics and failing to stop counters.
- [Medium] Race condition during CPU context migration permanently disables the hrtimer on the destination CPU.
- [Low] Missing bounds check on pmu->num_counters can lead to an out-of-bounds write.
--

commit f16450b9a54bbeb570f328c8a8626473a456f6b8
Author: Qi Liu <liuqi@hygon.cn>

perf/x86/amd/uncore: Add common PMU helper functions
    
Add common helper functions for AMD-family uncore PMU handling.
The helpers cover event initialization, counter allocation, counter
read/update, event start/stop and per-CPU context management.

> diff --git a/arch/x86/events/amd/uncore_common.c b/arch/x86/events/amd/uncore_common.c
> new file mode 100644
> index 0000000000000..a6d50fe803df3
> --- /dev/null
> +++ b/arch/x86/events/amd/uncore_common.c
[ ... ]
> +void uncore_common_stop(struct perf_event *event, int flags)
> +{
> +	struct uncore_common_pmu *pmu = event_to_uncore_common_pmu(event);
> +	struct uncore_common_ctx *ctx = *per_cpu_ptr(pmu->ctx, event->cpu);
> +	struct hw_perf_event *hwc = &event->hw;
> +
> +	wrmsrq(hwc->config_base, hwc->config);
> +	hwc->state |= PERF_HES_STOPPED;
> +
> +	if ((flags & PERF_EF_UPDATE) && !(hwc->state & PERF_HES_UPTODATE)) {
> +		event->pmu->read(event);
> +		hwc->state |= PERF_HES_UPTODATE;
> +	}
> +
> +	if (!--ctx->nr_active)
> +		uncore_common_cancel_hrtimer(ctx);

When a perf event is closed, free_event() calls pmu->del(), which maps to
uncore_common_del(). Since uncore_common_del() unconditionally calls
event->pmu->stop(), and the event was likely already stopped by
event_sched_out(), could this cause ctx->nr_active to underflow (e.g., from
0 to -1) because there is no check if the event is already stopped?

If ctx->nr_active underflows, a subsequent event start will evaluate
!ctx->nr_active++ as false (!-1 is 0), which would fail to call
uncore_common_start_hrtimer(), causing the hardware counters to silently
overflow.

> +
> +	__clear_bit(hwc->idx, ctx->active_mask);
> +}
> +
> +int uncore_common_event_init(struct perf_event *event)
> +{
> +	struct uncore_common_pmu *pmu;
> +	struct uncore_common_ctx *ctx;
> +	struct hw_perf_event *hwc = &event->hw;
> +
> +	if (event->attr.type != event->pmu->type)
> +		return -ENOENT;
> +
> +	if (event->cpu < 0)
> +		return -EINVAL;
> +
> +	pmu = event_to_uncore_common_pmu(event);
> +	ctx = *per_cpu_ptr(pmu->ctx, event->cpu);
> +	if (!ctx)
> +		return -ENODEV;
> +
> +	hwc->config = event->attr.config;

Is it safe to copy the user-supplied event->attr.config directly to
hwc->config without any sanitization or bit masking?

If a user configures an event with reserved hardware bits set, could the
later call to wrmsrq() trigger a General Protection Fault and crash the
kernel?

Additionally, if the user explicitly sets the hardware enable bit (e.g.,
ARCH_PERFMON_EVENTSEL_ENABLE), wouldn't uncore_common_stop() fail to
actually stop the counter when it writes this configuration back to the MSR?

> +	hwc->idx = -1;
> +
> +	event->cpu = ctx->cpu;
> +
> +	return 0;
> +}
> +
> +int uncore_common_add(struct perf_event *event, int flags)
> +{
> +	struct uncore_common_pmu *pmu = event_to_uncore_common_pmu(event);
> +	struct uncore_common_ctx *ctx = *per_cpu_ptr(pmu->ctx, event->cpu);
> +	struct hw_perf_event *hwc = &event->hw;
> +	int i;
> +
> +	/* are we already assigned? */
> +	if (hwc->idx != -1 && ctx->events[hwc->idx] == event)
> +		goto out;
> +
> +	for (i = 0; i < pmu->num_counters; i++) {

Is there a bounds check to ensure pmu->num_counters does not exceed
NUM_COUNTERS_MAX?

ctx->active_mask is sized based on NUM_COUNTERS_MAX. If a PMU is initialized
with more than 64 counters, couldn't a later __set_bit(hwc->idx,
ctx->active_mask) write out of bounds and corrupt adjacent memory?

> +		if (ctx->events[i] == event) {
> +			hwc->idx = i;
> +			goto out;
> +		}
> +	}
[ ... ]
> +void uncore_common_ctx_free(struct uncore_common *uncore, unsigned int cpu)
> +{
> +	struct uncore_common_pmu *pmu;
> +	struct uncore_common_ctx *ctx;
> +	int i;
> +
> +	if (!uncore->init_done)
> +		return;
> +
> +	for (i = 0; i < uncore->num_pmus; i++) {
> +		pmu = &uncore->pmus[i];
> +
> +		if (!pmu->ctx)
> +			continue;
> +
> +		ctx = *per_cpu_ptr(pmu->ctx, cpu);
> +		if (!ctx)
> +			continue;
> +
> +		if (cpu == ctx->cpu)
> +			cpumask_clear_cpu(cpu, &pmu->active_mask);
> +
> +		if (!--ctx->refcnt) {
> +			kfree(ctx->events);
> +			kfree(ctx);
> +		}

When the last CPU in the PMU's domain goes offline, ctx->refcnt will drop
to 0 and ctx is freed. However, is there anything ensuring hrtimer_cancel()
is called if there are still active events attached?

If the hrtimer is still running, it will execute on the freed ctx memory.

> +
> +		*per_cpu_ptr(pmu->ctx, cpu) = NULL;

Additionally, since the user's perf_event file descriptors remain valid,
when the user later closes them, uncore_common_del() will attempt to access
the context.

Since *per_cpu_ptr(pmu->ctx, cpu) is set to NULL here, won't that result
in a NULL-pointer dereference?

> +	}
> +}
> +
> +void uncore_common_ctx_move(struct uncore_common *uncore, unsigned int cpu)
> +{
> +	struct uncore_common_ctx *curr, *next;
> +	struct uncore_common_pmu *pmu;
> +	int i, j;
[ ... ]
> +		for_each_online_cpu(j) {
> +			if (cpu == j)
> +				continue;
> +
> +			next = *per_cpu_ptr(pmu->ctx, j);
> +			if (!next)
> +				continue;
> +
> +			if (curr == next) {
> +				perf_pmu_migrate_context(&pmu->pmu, cpu, j);
> +				cpumask_clear_cpu(cpu, &pmu->active_mask);
> +				cpumask_set_cpu(j, &pmu->active_mask);
> +				next->cpu = j;

Should next->cpu = j be set before calling perf_pmu_migrate_context()?

perf_pmu_migrate_context() synchronously installs and starts events on the
destination CPU, which arms the hrtimer. If the hrtimer expires before the
migration thread reaches next->cpu = j, uncore_common_hrtimer() will
observe that ctx->cpu != smp_processor_id() and return HRTIMER_NORESTART.

This would permanently disable the PMU timer on the new CPU.

> +				break;
> +			}
> +		}
> +	}
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260519033225.1479907-1-liuqi@hygon.cn?part=1