Re: [PATCH v3] perf: Add Arm Bus Monitor Unit driver

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:
- [High] arm-bmu: out-of-bounds MMIO access when calculating PMU base addresses
- [High] arm-bmu: race condition in CPU hotplug instance registration causes UAF/NULL deref
- [High] acpi: NULL pointer dereference when accessing ACPI_COMPANION(dev)
- [Medium] arm-bmu: hardware interrupts remain enabled on probe error paths and when missing IRQ
- [Medium] arm-bmu: hardware counter events are lost during overflow handling
- [Low] acpi: implicit declaration of acpi_device_uid() causes build failure when !CONFIG_ACPI
--

commit 7371a9609aa611a01ae74feaaad4d9c2b61e0cd7
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. This commit adds a
driver for the BMU PMU architecture under arm_cspmu/.

> diff --git a/drivers/perf/arm_cspmu/arm-bmu.c b/drivers/perf/arm_cspmu/arm-bmu.c
> index 0000000000000..fe25954ac8361
> --- /dev/null
> +++ b/drivers/perf/arm_cspmu/arm-bmu.c
[ ... ]
> +static void arm_bmu_pmu_irq(struct arm_bmu *bmu, int imu)
> +{
> +	struct arm_bmu_pmu *pmu = bmu->imus + imu;
> +	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 (!pmu->evcnt[i]) {
> +			dev_dbg(bmu->dev, "Spurious oveflow on IMU %d counter %d?\n", imu, 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: Medium]
Will hardware events occurring between arm_bmu_event_read() and this
lo_hi_writeq_relaxed() be lost since the actively running counter is
overwritten without being paused?

[ ... ]
> +static int arm_bmu_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	const struct resource *res;
> +	struct arm_bmu *bmu;
> +	const char *name = NULL;
> +	void __iomem *base;
> +	static atomic_t n;
> +	int err, num, sz, off;
> +	u64 cfg;
> +	u32 reg;
> +
> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	if (!res)
> +		return -EINVAL;
> +
> +	/* PMUs and MPAM monitors are intermingled so we can't claim the whole resource */
> +	base = devm_ioremap(dev, res->start, resource_size(res));
> +	if (!base)
> +		return -ENOMEM;
[ ... ]
> +	cfg = lo_hi_readq_relaxed(base + MCU_CONFIG);
> +	num = 1 + FIELD_GET(MCUCFG_NUM_IMU_MONITORS, cfg);
[ ... ]
> +	/* 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)))
> +		return dev_err_probe(dev, -EADDRINUSE, "Unable to request PMU region\n");
[ ... ]
> +	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);

[Severity: High]
Could this result in an out-of-bounds MMIO read/write if the hardware reports
off and num values such that off + num * SZ_4K exceeds the mapped
resource_size(res)? It doesn't appear the calculated offset and size are
validated against the ioremap region size before being dereferenced.

[ ... ]
> +		writel_relaxed(U32_MAX, base + PMCNTENCLR);
> +		writel_relaxed(U32_MAX, base + PMOVSCLR);
> +		writel_relaxed(U32_MAX, base + PMINTENSET);
> +	}
> +
> +	bmu->cpu = cpumask_local_spread(atomic_fetch_inc(&n), dev_to_node(dev));
> +	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]
Do hardware interrupts remain enabled if the driver proceeds here without an
IRQ handler? It looks like PMINTENSET was written earlier in the loop, but
the flags aren't cleared via PMINTENCLR when an IRQ isn't provided.

[ ... ]
> +	bmu->pmu = (struct pmu) {
> +		.module = THIS_MODULE,
> +		.parent = dev,
[ ... ]
> +	};
> +
> +	name = acpi_device_uid(ACPI_COMPANION(dev));

[Severity: High]
Can this cause a NULL pointer dereference if the driver binds to a device
without an ACPI companion? The pointer from ACPI_COMPANION(dev) appears to
be fed directly into acpi_device_uid() without being checked first.

[Severity: Low]
Additionally, since Kconfig allows building with CONFIG_COMPILE_TEST=y and
CONFIG_ACPI=n:

drivers/perf/arm_cspmu/Kconfig:ARM_BMU
        depends on (ARM64 && ACPI) || COMPILE_TEST

Does this unconditional call to acpi_device_uid() cause a build failure due
to an implicit declaration when !CONFIG_ACPI?

> +
> +	if (name)
> +		name = devm_kasprintf(dev, GFP_KERNEL, "arm_bmu_%s", name);
> +	else
> +		name = devm_kasprintf(dev, GFP_KERNEL, "arm_bmu_%llx", (u64)(res->start >> 12));
> +	if (!name)
> +		return -ENOMEM;
> +
> +	err = cpuhp_state_add_instance_nocalls(arm_bmu_cpuhp_state, &bmu->cpuhp_node);
> +	if (err)
> +		return err;

[Severity: Medium]
If this returns err, are hardware interrupts left enabled because PMINTENCLR
is not called on the error path?

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

[Severity: High]
Can adding the hotplug instance before perf_pmu_register() completes cause a
race condition where a concurrent hotplug event triggers arm_bmu_offline_cpu()
and dereferences an uninitialized context?

> +
> +static void arm_bmu_remove(struct platform_device *pdev)
> +{
> +	struct arm_bmu *bmu = platform_get_drvdata(pdev);
> +
> +	for (int i = 0; i < bmu->num_imus; i++)
> +		writel_relaxed(U32_MAX, bmu->imus[i].base + PMINTENCLR);
> +
> +	perf_pmu_unregister(&bmu->pmu);
> +	cpuhp_state_remove_instance_nocalls(arm_bmu_cpuhp_state, &bmu->cpuhp_node);

[Severity: High]
Does removing the instance after perf_pmu_unregister() leave a window open for
a use-after-free during teardown?

> +}

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