Re: [PATCH v2 2/3] dt-bindings: perf: Add Arm Bus Monitor Unit

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] The driver calculates an offset from hardware configuration registers and accesses memory within an ioremapped region without bounds checking, which can lead to out-of-bounds MMIO accesses.
- [High] The CPU hotplug instance is added before PMU registration completes, exposing a race condition where a CPU hotplug event will cause a NULL pointer dereference.
- [Medium] A pending overflow interrupt will trigger a spurious `WARN_ON` when an event is deleted because the hardware interrupt flag is not cleared.
--

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

perf: Add Arm Bus Monitor Unit driver

This commit introduces the Arm Bus Monitor Unit (BMU) driver, which provides
a low-level performance analysis tool for matching and counting transactions
at interconnect interfaces. It supports multiple Interface Monitoring Units
(IMUs) by exposing them as perf PMU instances.

> 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_del(struct perf_event *event, int flags)
> +{
> +	struct arm_bmu_pmu *pmu = to_bmu_pmu(event);
> +
> +	arm_bmu_event_stop(event, PERF_EF_UPDATE);
> +	pmu->evcnt[event->hw.idx] = NULL;
> +}

[Severity: Medium]
Will an unhandled pending overflow interrupt trigger a spurious warning here?

If a performance counter overflows right before or during arm_bmu_event_del(),
local interrupts are disabled by the perf core, leaving the interrupt pending.
Since arm_bmu_event_del() stops the counter and sets the evcnt pointer to
NULL but doesn't clear the pending overflow interrupt from the PMOVSCLR
register, this pending interrupt will be processed when interrupts are
re-enabled.

This leads to arm_bmu_handle_irq() executing and reading the uncleared bit
from PMOVSCLR, which evaluates to true in the following WARN_ON():

[ ... ]

> +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;

[ ... ]

> +static int arm_bmu_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	const struct resource *res;
> +	struct arm_bmu *bmu;

[ ... ]

> +	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);
> +	/* We don't expect to have dual-page complications to worry about */
> +	sz = FIELD_GET(MCUCFG_PMU_ELEMENT_SIZE, cfg);
> +	if (sz != 1)
> +		return dev_err_probe(dev, -EINVAL, "PMU_ELEMENT_SIZE 0x%x not supported\n", sz);
> +
> +	/* 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");

[Severity: High]
Does this calculation permit an out-of-bounds MMIO access if the offset from
the hardware configuration register is unexpectedly large?

The off and num values are read from the MCU_CONFIG hardware register and
used to calculate an offset into an ioremapped memory region that was mapped
to resource_size(res). There does not appear to be a check verifying that
off + num * SZ_4K does not exceed resource_size(res). 

If the hardware registers provide an unexpectedly large offset, accessing
memory outside the ioremapped bounds will trigger a synchronous external
abort when the driver attempts to read the PMCFGR register below:

> +
> +	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);

[ ... ]

> +	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);

[Severity: High]
Could a CPU hotplug event during this narrow window cause a NULL pointer
dereference?

The cpuhp_state_add_instance_nocalls() function is invoked before
perf_pmu_register(). If the assigned CPU goes offline during this exact
window, the hotplug state machine invokes arm_bmu_offline_cpu() which in
turn calls arm_bmu_migrate(). 

This leads to perf_pmu_migrate_context() being called. Because
perf_pmu_register() has not yet executed, the core context pointer is still
NULL, resulting in a dereference of an invalid pointer at the start of the
percpu offset region.

> +	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=2