Re: [PATCH v10 06/11] x86/hw_breakpoint: Add arch_reinstall_hw_breakpoint

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] Spurious debug exceptions during reinstall of AMD range breakpoints due to incorrect hardware register update ordering.
--

--- Patch [6]: [PATCH v10 06/11] x86/hw_breakpoint: Add arch_reinstall_hw_breakpoint ---
commit f97fcd502b5557f06d1cd0cadf2259bf2197dc51
Author: Jinchao Wang <wangjinchao600@gmail.com>

x86/hw_breakpoint: Add arch_reinstall_hw_breakpoint

The new arch_reinstall_hw_breakpoint() function can be used in an
atomic context, unlike the more expensive free and re-allocation path.
This allows callers to efficiently re-establish an existing breakpoint.

> diff --git a/arch/x86/kernel/hw_breakpoint.c b/arch/x86/kernel/hw_breakpoint.c
> index c323c2aab2af9..0df3ff556f479 100644
> --- a/arch/x86/kernel/hw_breakpoint.c
> +++ b/arch/x86/kernel/hw_breakpoint.c

[ ... ]

> @@ -129,23 +133,36 @@ static int manage_bp_slot(struct perf_event *bp, enum bp_slot_action action)
>  static void setup_hwbp(struct arch_hw_breakpoint *info, int slot, bool enable)
>  {
>  	unsigned long dr7;
> -
> -	set_debugreg(info->address, slot);
> -	__this_cpu_write(cpu_debugreg[slot], info->address);
> +	bool enabled;
>  
>  	dr7 = this_cpu_read(cpu_dr7);
> +	enabled = dr7 & ((DR_LOCAL_ENABLE | DR_GLOBAL_ENABLE) << (slot * DR_ENABLE_SIZE));
>  	dr7 &= ~(__encode_dr7(slot, 0xc, 0x3) |
>  		 (DR_LOCAL_ENABLE << (slot * DR_ENABLE_SIZE)));
> -	if (enable)
> -		dr7 |= encode_dr7(slot, info->len, info->type);
> +
> +	/*
> +	 * If the slot is currently enabled, disable it first before updating
> +	 * the address register to prevent spurious debug exceptions.
> +	 */
> +	if (enable && enabled) {
> +		barrier();
> +		set_debugreg(dr7, 7);
> +		barrier();
> +		this_cpu_write(cpu_dr7, dr7);
> +	}
> +
> +	set_debugreg(info->address, slot);
> +	__this_cpu_write(cpu_debugreg[slot], info->address);
>  
>  	/*
>  	 * Enabling:
>  	 *   Ensure we first write cpu_dr7 before we set the DR7 register.
>  	 *   This ensures an NMI never see cpu_dr7 0 when DR7 is not.
>  	 */
> -	if (enable)
> +	if (enable) {
> +		dr7 |= encode_dr7(slot, info->len, info->type);
>  		this_cpu_write(cpu_dr7, dr7);
> +	}
>  
>  	barrier();

[Severity: Medium]
Could this ordering cause a spurious debug exception when reinstalling a
breakpoint with a narrowed address mask?

When arch_reinstall_hw_breakpoint() is called to narrow the mask of an active
AMD breakpoint (e.g., changing from a wide range mask like 0xFF to an exact
match 0x0), setup_hwbp() re-enables the breakpoint in DR7 via
set_debugreg(dr7, 7) just after this barrier, but before the AMD address mask
is updated via amd_set_dr_addr_mask().

If an NMI occurs in this small window, or if the memory accesses of
amd_set_dr_addr_mask() itself fall within the old wider range around the new
address, it seems a spurious #DB exception could fire.

Should the mask be updated before re-enabling the breakpoint in DR7?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/178476134787.26117.10094977293012760490.stgit@devnote2?part=6