Re: [PATCH v10 05/11] x86/hw_breakpoint: Unify breakpoint install/uninstall

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

New issues:
- [High] Spurious debug exceptions due to modifying DR0-DR3 while enabled in DR7.

Pre-existing issues:
- [High] NMI race condition during breakpoint uninstallation leaves the hardware breakpoint permanently enabled.
--

--- Patch [5]: [PATCH v10 05/11] x86/hw_breakpoint: Unify breakpoint install/uninstall ---
commit 68cc4a145838d44957bc67ccfa580a6107987aa3
Author: Jinchao Wang <wangjinchao600@gmail.com>

x86/hw_breakpoint: Unify breakpoint install/uninstall

Consolidate breakpoint management to reduce code duplication.
The diffstat was misleading, so the stripped code size is compared instead.
After refactoring, it is reduced from 11976 bytes to 11448 bytes on my
x86_64 system built with clang.

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

[ ... ]

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

[Severity: High]
Does this code introduce a spurious debug exception when uninstalling a
breakpoint?

When uninstalling (enable == false), the debug address register is modified
here, but its corresponding enable bit in the hardware DR7 register is still
active. According to the Intel SDM Volume 3 (Section 17.2.4), modifying
DR0-DR3 without first clearing the corresponding enable bits in DR7 is
forbidden. The old implementation bypassed modifying DR0-DR3 on uninstall,
which avoided this issue.

> +
> +	dr7 = this_cpu_read(cpu_dr7);
> +	dr7 &= ~(__encode_dr7(slot, 0xc, 0x3) |
> +		 (DR_LOCAL_ENABLE << (slot * DR_ENABLE_SIZE)));
> +	if (enable)
> +		dr7 |= encode_dr7(slot, info->len, info->type);
> +
> +	/*
> +	 * 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)
> +		this_cpu_write(cpu_dr7, dr7);
> +
> +	barrier();
> +
> +	set_debugreg(dr7, 7);
> +
> +	amd_set_dr_addr_mask(enable ? info->mask : 0, slot);
> +
> +	barrier();
> +
> +	/*
> +	 * Disabling:
> +	 *   Ensure the write to cpu_dr7 is after we've set the DR7 register.
> +	 *   This ensures an NMI never see cpu_dr7 0 when DR7 is not.
> +	 */
> +	if (!enable)
> +		this_cpu_write(cpu_dr7, dr7);

[Severity: High]
This is a pre-existing issue, but does this sequence create an NMI race
condition that could leave the hardware breakpoint permanently enabled?

When uninstalling, this code writes to the hardware DR7 register first
(set_debugreg(dr7, 7)), issues a barrier, and then updates the per-CPU
tracking variable (this_cpu_write(cpu_dr7, dr7)).

If an NMI (like SysRq-G for KGDB) fires in the window between these writes,
the NMI handler (such as kgdb_correct_hw_break()) will call
hw_breakpoint_restore(). This reads the stale cpu_dr7 (which still has the
slot enabled) and writes it back to hardware DR7, re-enabling the
breakpoint.

When the NMI returns, setup_hwbp() continues and writes the disabled state
to cpu_dr7 in memory, but the hardware DR7 is left enabled. If the
breakpoint triggers later, hw_breakpoint_handler() will see a NULL
bp_per_reg and could enter an infinite #DB loop.

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