Re: [PATCH v5 3/4] perf/arm64: Add BRBE support for bpf_get_branch_snapshot()

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] `arm_brbe_snapshot_branch_stack` contains a TOCTOU vulnerability where caching `SYS_BRBCR_EL1` before disabling interrupts allows a stale value to be restored, unintentionally re-enabling BRBE if an interrupt disables the PMU event in the intervening window.
--

commit dbaa008d5027ec5e859cbcac51237295c1187ed6
Author: Puranjay Mohan <puranjay@kernel.org>

perf/arm64: Add BRBE support for bpf_get_branch_snapshot()

Enable bpf_get_branch_snapshot() on ARM64 by implementing the
perf_snapshot_branch_stack static call for BRBE. This pauses BRBE,
masks exceptions to prevent PMU overflow pseudo-NMI interference,
and reads the branch records.

> diff --git a/drivers/perf/arm_brbe.c b/drivers/perf/arm_brbe.c
> index effbdeacfcbb3..a141ad7abcf25 100644
> --- a/drivers/perf/arm_brbe.c
> +++ b/drivers/perf/arm_brbe.c
[ ... ]
> @@ -774,32 +795,97 @@ void brbe_read_filtered_entries(struct perf_branch_stack *branch_stack,
[ ... ]
> +int arm_brbe_snapshot_branch_stack(struct perf_branch_entry *entries, unsigned int cnt)
> +{
> +	unsigned long flags;
> +	int nr_hw, nr_copied = 0;
> +	u64 brbfcr, brbcr;
> +
> +	if (!cnt)
> +		return 0;
> +
> +	/* Guard against running on a CPU without BRBE (e.g. big.LITTLE). */
> +	if (!cpu_has_brbe())
> +		return 0;
> +
> +	/*
> +	 * Pause BRBE first to avoid recording our own branches. The
> +	 * sysreg read/write and ISB are branchless, so pausing before
> +	 * checking BRBCR avoids polluting the buffer with our own
> +	 * conditional branches.
> +	 */
> +	brbfcr = read_sysreg_s(SYS_BRBFCR_EL1);
> +	brbcr = read_sysreg_s(SYS_BRBCR_EL1);
> +	write_sysreg_s(brbfcr | BRBFCR_EL1_PAUSED, SYS_BRBFCR_EL1);
> +	isb();
> +
> +	/* Bail out if BRBE is not enabled (BRBCR_EL1 == 0). */
> +	if (!brbcr) {
> +		write_sysreg_s(brbfcr, SYS_BRBFCR_EL1);
> +		isb();
> +		return 0;
> +	}
> +
> +	/* Block local exception delivery while reading the buffer. */
> +	flags = local_daif_save();
> +
> +	/*
> +	 * A PMU overflow before local_daif_save() could have re-enabled
> +	 * BRBE, clearing the PAUSED bit. The overflow handler already
> +	 * restored BRBE to its correct state, so just bail out.
> +	 */
> +	if (!(read_sysreg_s(SYS_BRBFCR_EL1) & BRBFCR_EL1_PAUSED)) {
> +		local_daif_restore(flags);
> +		return 0;
> +	}
> +
> +	nr_hw = FIELD_GET(BRBIDR0_EL1_NUMREC_MASK,
> +			  read_sysreg_s(SYS_BRBIDR0_EL1));
> +
> +	for_each_brbe_entry(i, nr_hw) {
> +		if (nr_copied >= cnt)
> +			break;
> +
> +		if (!perf_entry_from_brbe_regset(i, &entries[nr_copied], NULL))
> +			break;
> +
> +		nr_copied++;
> +	}
> +
> +	brbe_invalidate();
> +
> +	/* Restore BRBCR before unpausing via BRBFCR, matching brbe_enable(). */
> +	write_sysreg_s(brbcr, SYS_BRBCR_EL1);

[Severity: High]
Is it possible for a race condition here to accidentally re-enable BRBE?

SYS_BRBCR_EL1 is read and cached in brbcr before local_daif_save() disables
interrupts.

If an interrupt arrives in this window and disables the PMU (for example,
an IPI calling perf_remove_from_context() that invokes brbe_disable()),
it would set the BRBFCR_EL1_PAUSED bit.

When execution resumes, the bailout condition evaluates to false because
the PAUSED bit is now set:

    if (!(read_sysreg_s(SYS_BRBFCR_EL1) & BRBFCR_EL1_PAUSED)) {
        local_daif_restore(flags);
        return 0;
    }

The function then continues, and blindly writes the cached brbcr back to
the hardware here.

Since the cached brbcr had BRBE enabled, could this unintentionally
re-enable BRBE globally despite the kernel having just explicitly disabled it?

> +	isb();
> +	write_sysreg_s(brbfcr, SYS_BRBFCR_EL1);
> +	/* Ensure BRBE is unpaused before returning to the caller. */
> +	isb();
> +	local_daif_restore(flags);
> +
> +	return nr_copied;
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260616155716.2631508-1-puranjay@kernel.org?part=3