linux-kernel - Re: [PATCH V3 7/7] arm64/hw_breakpoint: Enable FEAT

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <Z2AH1SN5CipWkkE4@J2N7QTR9R3>
Date: Mon, 16 Dec 2024 10:58:29 +0000
From: Mark Rutland <mark.rutland@....com>
To: Anshuman Khandual <anshuman.khandual@....com>
Cc: linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org,
	Jonathan Corbet <corbet@....net>, Marc Zyngier <maz@...nel.org>,
	Oliver Upton <oliver.upton@...ux.dev>,
	James Morse <james.morse@....com>,
	Suzuki K Poulose <suzuki.poulose@....com>,
	Catalin Marinas <catalin.marinas@....com>,
	Will Deacon <will@...nel.org>, Mark Brown <broonie@...nel.org>,
	kvmarm@...ts.linux.dev
Subject: Re: [PATCH V3 7/7] arm64/hw_breakpoint: Enable FEAT_Debugv8p9

On Mon, Dec 16, 2024 at 09:38:31AM +0530, Anshuman Khandual wrote:
> Currently there can be maximum 16 breakpoints, and 16 watchpoints available
> on a given platform - as detected from ID_AA64DFR0_EL1.[BRPs|WRPs] register
> fields. But these breakpoint, and watchpoints can be extended further up to
> 64 via a new arch feature FEAT_Debugv8p9.
> 
> This first enables banked access for the breakpoint and watchpoint register
> set via MDSELR_EL1, extended exceptions via MDSCR_EL1.EMBWE and determining
> available breakpoints and watchpoints in the platform from ID_AA64DFR1_EL1,
> when FEAT_Debugv8p9 is enabled.

[...]

> +static u64 read_wb_reg(int reg, int n)
> +{
> +	unsigned long flags;
> +	u64 val;
> +
> +	if (!is_debug_v8p9_enabled())
> +		return __read_wb_reg(reg, n);
> +
> +	/*
> +	 * Bank selection in MDSELR_EL1, followed by an indexed read from
> +	 * breakpoint (or watchpoint) registers cannot be interrupted, as
> +	 * that might cause misread from the wrong targets instead. Hence
> +	 * this requires mutual exclusion.
> +	 */
> +	local_irq_save(flags);
> +	write_sysreg_s(SYS_FIELD_PREP(MDSELR_EL1, BANK, n / MAX_PER_BANK), SYS_MDSELR_EL1);
> +	isb();
> +	val = __read_wb_reg(reg, n % MAX_PER_BANK);
> +	local_irq_restore(flags);
> +	return val;
> +}
>  NOKPROBE_SYMBOL(read_wb_reg);

I don't believe that disabling interrupts here is sufficient. On the
last version I asked about the case of racing with a watchpoint handler:

| For example, what prevents watchpoint_handler() from firing in the
| middle of arch_install_hw_breakpoint() or
| arch_uninstall_hw_breakpoint()?

... and disabling interrupts cannot prevent that, because
local_irq_{save,restore}() do not affect the behaviour of watchpoints or
breakpoints.

Please can you try to answer the questions I asked last time, i.e.

| What prevents a race with an exception handler? e.g. 
| 
| * Does the structure of the code prevent that somehow?
| 
| * What context(s) does this code execute in?
|   - Are debug exceptions always masked?
|   - Do we disable breakpoints/watchpoints around (some) manipulation of
|     the relevant registers?

... and the question form the earlier comment, i.e.

| Is the existing code correct?

I suspect that the existing code might not have the necessary mutual
exclusion in all cases, but it's difficult to trigger an issue by
accident. Is there any way a handler could race with some other
manipulation of watchpoints/breakpoints such that some data structure
gets corrupted, or such that the kernel deadlocks?

Mark.