Re: [PATCH v2 1/3] perf: Add Arm Bus Monitor Unit driver

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 9 potential issue(s) to consider:
- [High] Unconditional use of `ACPI_COMPANION()` and `acpi_device_uid()` causes a NULL pointer dereference or build failure.
- [High] Missing `continue` when Non-Secure access is disabled leads to writes to inaccessible PMU registers.
- [High] ABA vulnerability and NMI torn state in lockless `prev_count` update causes massive PMU count corruption.
- [High] Missing bounds check on the computed PMU region offset against the actual ioremapped memory resource size.
- [High] Missing cleanup of hardware interrupt state on probe error paths causes an unhandled interrupt storm.
- [Medium] TOCTOU race in PMU CPU assignment during probe can bind the PMU to an offline CPU, leaving it non-functional.
- [Medium] Ignoring `-EPROBE_DEFER` for the interrupt permanently breaks IRQ functionality.
- [Medium] False-positive `WARN_ON` triggered by a race between event deletion and pending PMU interrupts.
- [Low] Format string type mismatch passing a 32-bit `resource_size_t` / `phys_addr_t` to a 64-bit `%llx` specifier.
--

--- Patch [1]: [PATCH v2 1/3] perf: Add Arm Bus Monitor Unit driver ---
commit 46bae7b04d539138ea677d6d9ef5697dcb457c20
Author: Robin Murphy <robin.murphy@arm.com>

perf: Add Arm Bus Monitor Unit driver

Arm's Bus Monitor Unit is a low-level performance analysis tool for
matching and counting transactions at interconnect interfaces. Each
BMU consists of a number of "interface monitoring units", with some
global controls to synchronise them and so permit accurate calculation
of cross-interface metrics.

> diff --git a/drivers/perf/arm_cspmu/arm-bmu.c b/drivers/perf/arm_cspmu/arm-bmu.c
> new file mode 100644
> index 0000000000000..c829a0728e9b2
> --- /dev/null
> +++ b/drivers/perf/arm_cspmu/arm-bmu.c
[ ... ]
> +static void arm_bmu_event_read(struct perf_event *event)
> +{
> +	struct hw_perf_event *hw = &event->hw;
> +	u64 count, prev;
> +
> +	do {
> +		prev = local64_read(&hw->prev_count);
> +		count = arm_bmu_read_evcnt(event);
> +	} while (local64_cmpxchg(&hw->prev_count, prev, count) != prev);
> +
> +	count -= prev;
> +	local64_add(count, &event->count);
> +}
> +
> +static void arm_bmu_event_start(struct perf_event *event, int flags)
> +{
> +	struct arm_bmu_pmu *pmu = to_bmu_pmu(event);
> +
> +	writel_relaxed(1ULL << event->hw.idx, pmu->base + PMCNTENSET);
> +}
> +
> +static void arm_bmu_event_stop(struct perf_event *event, int flags)
> +{
> +	struct arm_bmu_pmu *pmu = to_bmu_pmu(event);
> +
> +	writel_relaxed(1ULL << event->hw.idx, pmu->base + PMCNTENCLR);

[Severity: Medium]
Should the hardware overflow state (PMOVSCLR) be cleared here when stopping
the event?

If an overflow occurs right before stopping, the pending interrupt bit
remains set. When arm_bmu_event_del() completes, it clears the event pointer.
When the interrupt is finally delivered, wouldn't the interrupt handler see
the overflow bit set and incorrectly trigger the WARN_ON for a NULL event
pointer?

> +	if (flags & PERF_EF_UPDATE)
> +		arm_bmu_event_read(event);
> +}
[ ... ]
> +static void arm_bmu_pmu_irq(struct arm_bmu_pmu *pmu)
> +{
> +	u32 reg = readl_relaxed(pmu->base + PMOVSCLR);
> +	u64 __iomem *pmevcnt = pmu->base + PMEVCNTR_LO;
> +
> +	for (int i = 0; i < PMU_MAX_COUNTERS; i++) {
> +		if (!(reg & (1U << i)))
> +			continue;
> +		if (WARN_ON(!pmu->evcnt[i]))
> +			continue;
> +		arm_bmu_event_read(pmu->evcnt[i]);
> +		local64_set(&pmu->evcnt[i]->hw.prev_count, S64_MIN);
> +		lo_hi_writeq_relaxed(S64_MIN, pmevcnt + i);

[Severity: High]
Is the lockless compare-and-exchange logic in arm_bmu_event_read() vulnerable
to the ABA problem when resetting prev_count here?

If an NMI interrupts this IRQ handler right after prev_count is set to S64_MIN
but before the hardware register is written, arm_bmu_event_read() could read
the old wrapped hardware value and falsely succeed in the cmpxchg because
prev_count is S64_MIN.

This would save the wrapped value as prev_count. When the IRQ handler resumes
and resets the hardware to S64_MIN, wouldn't subsequent delta calculations
underflow and cause massive permanent counter corruption?

> +	}
> +	writel_relaxed(reg, pmu->base + PMOVSCLR);
> +}
[ ... ]
> +static int arm_bmu_probe(struct platform_device *pdev)
> +{
[ ... ]
> +	/* The PMU pages *are* exclusively ours */
> +	off = SZ_4K * FIELD_GET(MCUCFG_PMU_ELEMENT_START, cfg);
> +	if (!devm_request_mem_region(dev, res->start + off, num * SZ_4K, dev_name(dev)))

[Severity: High]
Does this compute the PMU region offset without verifying it fits within the
originally mapped memory resource size?

If the hardware provides an offset such that the new region exceeds
resource_size(res), applying this unvalidated offset to the virtual base pointer
and dereferencing it later will access memory outside the mapped virtual
memory area, potentially causing a kernel page fault or panic.

> +		return dev_err_probe(dev, -EADDRINUSE, "Unable to request PMU region\n");
> +
> +	bmu = devm_kzalloc(dev, struct_size(bmu, imus, num), GFP_KERNEL);
> +	if (!bmu)
> +		return -ENOMEM;
> +
> +	bmu->dev = dev;
> +	bmu->base = base;
> +	bmu->num_imus = num;
> +	platform_set_drvdata(pdev, bmu);
> +
> +	base += off;
> +	for (int i = 0; i < bmu->num_imus; i++, base += SZ_4K) {
> +		/* At least PMCFGR.SIZE should always be nonzero if visible */
> +		reg = readl_relaxed(base + PMCFGR);
> +		if (!reg) {
> +			dev_warn(dev, "Non-Secure access to PMU %d not enabled?\n", i);
> +			num = 0;
> +		} else {

[Severity: High]
Should there be a continue statement when Non-Secure access is disabled?

Without a continue, the code proceeds to unconditionally write to BMU_PMCR and
other registers for this PMU below. Writing to inaccessible secure hardware
from EL1 typically triggers a Synchronous External Abort (SError), resulting
in an immediate kernel panic.

> +			num = 1 + FIELD_GET(PMCFGR_N, reg);
> +		}
> +		if (num > PMU_MAX_COUNTERS) {
> +			dev_notice(dev, "PMU %d has %d counters, only using %d\n",
> +				   i, num, PMU_MAX_COUNTERS);
> +			num = PMU_MAX_COUNTERS;
> +		}
> +		bmu->imus[i].base = base;
> +		bmu->imus[i].num_counters = num;
> +
> +		writel_relaxed(PMCR_P | PMCR_E, base + BMU_PMCR);
> +		writel_relaxed(U32_MAX, base + PMCNTENCLR);
> +		writel_relaxed(U32_MAX, base + PMOVSCLR);
> +		writel_relaxed(U32_MAX, base + PMINTENSET);
> +	}

[Severity: High]
Are hardware interrupts left enabled if an error occurs later in the probe
function?

If cpuhp_state_add_instance_nocalls() or perf_pmu_register() fails, the error
path exits without clearing PMINTENCLR. If the hardware asserts the IRQ line,
the kernel would receive an interrupt without a registered handler, leading to
an unhandled interrupt storm.

> +
> +	bmu->cpu = cpumask_local_spread(atomic_fetch_inc(&n), dev_to_node(dev));

[Severity: Medium]
Is there a race in assigning the target CPU without holding the CPU hotplug
read lock?

If the selected CPU goes offline before the hotplug registration, the
arm_bmu_offline_cpu callback will not be invoked retroactively because
cpuhp_state_add_instance_nocalls() is used below. Could this leave the PMU
bound to an offline CPU, rendering it non-functional?

> +	bmu->irq = platform_get_irq(pdev, 0);
> +	if (bmu->irq > 0) {
> +		err = devm_request_irq(dev, bmu->irq, arm_bmu_handle_irq,
> +				       IRQF_NOBALANCING | IRQF_NO_THREAD,
> +				       dev_name(dev), bmu);
> +		if (err)
> +			bmu->irq = err;
> +		else
> +			irq_set_affinity(bmu->irq, cpumask_of(bmu->cpu));
> +	}
> +	if (bmu->irq < 0)
> +		dev_info(dev, "Continuing without IRQ\n");

[Severity: Medium]
Does this completely ignore -EPROBE_DEFER from platform_get_irq()?

If platform_get_irq() returns -EPROBE_DEFER, the probe simply succeeds and
falls back to polling mode. This permanently breaks deferred probing for the
interrupt, preventing the driver from retrying when the interrupt controller
is ready.

> +
> +	bmu->pmu = (struct pmu) {
> +		.module = THIS_MODULE,
> +		.parent = dev,
> +		.attr_groups = arm_bmu_attr_groups,
> +		.capabilities = PERF_PMU_CAP_NO_EXCLUDE,
> +		.task_ctx_nr = perf_invalid_context,
> +		.pmu_enable = arm_bmu_enable,
> +		.pmu_disable = arm_bmu_disable,
> +		.event_init = arm_bmu_event_init,
> +		.add = arm_bmu_event_add,
> +		.del = arm_bmu_event_del,
> +		.start = arm_bmu_event_start,
> +		.stop = arm_bmu_event_stop,
> +		.read = arm_bmu_event_read,
> +	};
> +
> +	name = acpi_device_uid(ACPI_COMPANION(dev));

[Severity: High]
Will this cause a NULL pointer dereference if the device has no ACPI companion
or if CONFIG_ACPI is disabled?

ACPI_COMPANION() returns NULL in these cases, and acpi_device_uid() directly
dereferences it. This would lead to a kernel panic during driver probe on
such configurations.

> +
> +	if (name)
> +		name = devm_kasprintf(dev, GFP_KERNEL, "arm_bmu_%s", name);
> +	else
> +		name = devm_kasprintf(dev, GFP_KERNEL, "arm_bmu_%llx", res->start >> 12);

[Severity: Low]
Is there a format string type mismatch here?

When CONFIG_PHYS_ADDR_T_64BIT is not enabled, resource_size_t is 32-bit.
Passing a 32-bit integer to a %llx specifier causes undefined behavior,
which can read garbage from the stack for the upper 32 bits.

> +	if (!name)
> +		return -ENOMEM;
> +
> +	err = cpuhp_state_add_instance_nocalls(arm_bmu_cpuhp_state, &bmu->cpuhp_node);
> +	if (err)
> +		return err;
> +
> +	err = perf_pmu_register(&bmu->pmu, name, -1);
> +	if (err)
> +		cpuhp_state_remove_instance_nocalls(arm_bmu_cpuhp_state, &bmu->cpuhp_node);
> +
> +	return err;
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1784214112.git.robin.murphy@arm.com?part=1