Re: [PATCH v2 1/2] perf: Add Raspberry Pi BCM2835 AXI PMU driver

Thank you for your contribution! Sashiko AI review found 8 potential issue(s) to consider:
- [High] The probe function ignores a missing 'firmware' device tree phandle, leaving `pmu->firmware` NULL but leaving the VPU monitor enabled, causing a later NULL pointer dereference in the mailbox IPC.
- [High] The driver fails to prevent sysfs unbinding (`.suppress_bind_attrs = true`), leading to a Use-After-Free if unbound while perf events are active.
- [High] Incorrect manual parsing of the 'reg' property assumes fixed address and size cells, breaking on 64-bit platforms.
- [High] Incorrect PMU teardown ordering in `rpi_axi_pmu__exit` and missing cleanup in probe error paths cause Use-After-Free vulnerabilities.
- [High] The PMU driver uses a `spinlock_t` in a hardirq hrtimer callback, causing lockdep splats and sleeping in atomic context on PREEMPT_RT.
- [Medium] An unprotected write to `hw_events.enabled[idx]` creates a race condition with event deletion, which can silently break hardware configuration for recycled bus watcher slots.
- [Medium] The CPU hotplug offline callback erroneously restarts the polling hrtimer pinned to the dying CPU, sabotaging PMU migration.
- [Low] Raspberry Pi 5 (BCM2712) support is partially implemented but completely unreachable because the chip type is hardcoded to BCM2835 and the device tree match table lacks BCM2712.
--

--- Patch [1]: [PATCH v2 1/2] perf: Add Raspberry Pi BCM2835 AXI PMU driver ---
commit 3567c05975da01f684191bbbfa826e0e8d8e9927
Author: Ian Rogers <[email protected]>
Subject: perf: Add Raspberry Pi BCM2835 AXI PMU driver

This commit introduces a new uncore PMU driver for Broadcom BCM2835-BCM2711 
AXI system monitors. It supports tracking AXI traffic via MMIO for the system 
monitor and via mailbox IPC for the VPU monitor.

> diff --git a/drivers/perf/rpi_axi_pmu.c b/drivers/perf/rpi_axi_pmu.c
> new file mode 100644
> index 0000000000000..cdf7d9a74f13e
> --- /dev/null
> +++ b/drivers/perf/rpi_axi_pmu.c
[ ... ]
> +static int rpi_axi_pmu__init(struct rpi_axi_pmu *pmu, struct platform_device *pdev)
> +{
[ ... ]
> +	pmu->chip = CHIP_BCM2835;

[Severity: Low]
Is BCM2712 hardware meant to be supported here?

The chip type is unconditionally hardcoded to CHIP_BCM2835, which appears to
make the BCM2712 definitions unreachable.

> +
> +	pmu->pmu = (struct pmu) {
[ ... ]
> +	if (pmu->chip == CHIP_BCM2835) {
> +		pmu->monitor[MON__SYSTEM].use_mailbox_interface = false;
> +		pmu->monitor[MON__VPU].use_mailbox_interface = true;
> +		fw_node = of_parse_phandle(dev->of_node, "firmware", 0);
> +		if (fw_node) {
> +			pmu->firmware = rpi_firmware_get(fw_node);
> +			of_node_put(fw_node);
> +			if (!pmu->firmware)
> +				return -EPROBE_DEFER;
> +		}

[Severity: High]
Can this lead to a NULL pointer dereference in the mailbox IPC?

If the device tree lacks the firmware property, fw_node is NULL and
the driver continues with pmu->firmware left as NULL.

Later, set_monitor_control() will call rpi_firmware_property() using this
NULL pointer when VPU events are started.

> +	} else {
[ ... ]
> +	for (int i = 0; i < MON__MAX; i++) {
> +		rpi_axi_hw_events__init(&pmu->monitor[i].hw_events);
> +
> +		if (pmu->monitor[i].use_mailbox_interface) {
> +			ret = of_property_read_u32_index(dev->of_node, "reg", i * 2,
> +							 &pmu->monitor[i].mailbox);

[Severity: High]
Does this correctly parse the reg property on 64-bit platforms?

Using of_property_read_u32_index() with a fixed index of i * 2 assumes that
address and size cells are exactly 1.

On platforms with 64-bit device tree layouts, could this fetch the wrong
word and corrupt the mailbox address?

> +			if (ret) {
[ ... ]
> +	ret = cpuhp_state_add_instance(rpi_axi_pmu_cpuhp_state, &pmu->cpuhp_node);
> +	if (ret) {
> +		dev_err(dev, "Failed to add cpuhp instance %d\n", ret);
> +		goto err_pmu_unregister;
> +	}
> +	return 0;
> +
> +err_pmu_unregister:
> +	perf_pmu_unregister(&pmu->pmu);
> +err_firmware_put:
> +	if (pmu->firmware)
> +		rpi_firmware_put(pmu->firmware);
> +	return ret;
> +}

[Severity: High]
Could this regression cause a use-after-free if probe fails here?

If cpuhp_state_add_instance() fails, the error path unregisters the PMU but
does not cancel the hrtimer or vpu_work.

Since the PMU is briefly visible in sysfs, might userspace race to open an
event and arm the timer before the context is freed by devres?

> +static void rpi_axi_pmu__exit(struct rpi_axi_pmu *pmu)
> +{
> +	cpuhp_state_remove_instance(rpi_axi_pmu_cpuhp_state, &pmu->cpuhp_node);
> +	hrtimer_cancel(&pmu->hrtimer);
> +	cancel_work_sync(&pmu->vpu_work);
> +	if (pmu->firmware)
> +		rpi_firmware_put(pmu->firmware);
> +	perf_pmu_unregister(&pmu->pmu);
> +}

[Severity: High]
Does this teardown ordering allow new events to race with driver removal?

By cancelling the timer and workqueue before unregistering the PMU, there
appears to be a window where new events can be scheduled that restart the
timers while the context is being torn down.

[ ... ]
> +static void rpi_axi_pmu_enable_bus_watcher(struct rpi_axi_pmu *pmu, enum monitor mon,
> +					    int idx, int bus, int filter)
> +{
[ ... ]
> +	pmu->monitor[mon].hw_events.enabled[idx] = true;
> +}

[Severity: Medium]
Is this state update safe from concurrent deletion?

The array element is modified without holding pmu->lock.

When rpi_axi_pmu_vpu_work_handler() drops the lock to call this function:

rpi_axi_pmu_vpu_work_handler()
   spin_unlock_irq(&pmu->lock);
   rpi_axi_pmu_enable_bus_watcher(pmu, MON__VPU, idx, bus, filter);
   spin_lock_irq(&pmu->lock);

A concurrent rpi_axi_pmu_del() could clear the slot and set enabled to false.

If a new event immediately reuses this slot, wouldn't the unlocked write to
true overwrite the hardware configuration state for the new event?

[ ... ]
> +static enum hrtimer_restart rpi_axi_pmu_timer_handler(struct hrtimer *timer)
> +{
> +	struct rpi_axi_pmu *pmu = container_of(timer, struct rpi_axi_pmu, hrtimer);
> +	unsigned long flags;
> +
> +	spin_lock_irqsave(&pmu->lock, flags);

[Severity: High]
Will this lock acquisition panic on PREEMPT_RT kernels?

The timer defaults to hardirq context, where acquiring a sleepable lock like
spinlock_t triggers a panic on PREEMPT_RT.

[ ... ]
> +static int rpi_axi_pmu_offline_cpu(unsigned int cpu, struct hlist_node *node)
> +{
[ ... ]
> +	spin_lock_irqsave(&pmu->lock, spinflags);
> +	if (pmu->active_events > 0) {
> +		spin_unlock_irqrestore(&pmu->lock, spinflags);
> +		hrtimer_cancel(&pmu->hrtimer);
> +		spin_lock_irqsave(&pmu->lock, spinflags);
> +		if (pmu->active_events > 0)
> +			hrtimer_start(&pmu->hrtimer, RPI_AXI_PMU_TIMER_INTERVAL,
> +				      HRTIMER_MODE_REL_PINNED);
> +	}
> +	spin_unlock_irqrestore(&pmu->lock, spinflags);
> +
> +	return 0;
> +}

[Severity: Medium]
Does this logic unintentionally sabotage PMU migration during CPU offline?

This callback runs in the context of the dying CPU.

By passing HRTIMER_MODE_REL_PINNED here, the timer seems to be pinned to the
offlining CPU instead of being successfully migrated to the target CPU.

[ ... ]
> +static int rpi_axi_pmu_probe(struct platform_device *pdev)
> +{
> +	struct rpi_axi_pmu *pmu;
> +
> +	pmu = devm_kzalloc(&pdev->dev, sizeof(*pmu), GFP_KERNEL);

[ ... ]
> +static const struct of_device_id rpi_axi_pmu_match[] = {
> +	{
> +		.compatible = "brcm,bcm2835-axiperf",
> +		.data = (void *)CHIP_BCM2835,
> +	},
> +	{
> +		.compatible = "brcm,bcm2711-axiperf",
> +		.data = (void *)CHIP_BCM2835,
> +	},
> +	{ }
> +};

[Severity: Low]
Could this device tree match table be missing a compatible string for
BCM2712 hardware, preventing it from probing entirely?

> +MODULE_DEVICE_TABLE(of, rpi_axi_pmu_match);
> +
> +static struct platform_driver rpi_axi_pmu_driver  = {
> +	.probe =	rpi_axi_pmu_probe,
> +	.remove =	rpi_axi_pmu_remove,
> +	.driver = {
> +		.name   = PMU_NAME,
> +		.of_match_table = of_match_ptr(rpi_axi_pmu_match),
> +	},
> +};

[Severity: High]
Can an administrator unbinding this driver via sysfs trigger a use-after-free?

The pmu context is allocated with devm_kzalloc(), tying its memory lifetime
to the device bind state.

Since .suppress_bind_attrs = true is omitted, a sysfs unbind will free the
memory while active perf events might still exist and try to access the
freed context.

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