Re: [PATCH v15 03/12] x86/hw_breakpoints: Add arch_modify_local_hw_breakpoint_addr() API

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Modifying a debug address register while the breakpoint is enabled in DR7 violates the Intel SDM. This can occur due to a race condition with an NMI because the `cpu_dr7` shadow state is not temporarily updated.
--

commit ca38cc4e9ed0533dfe16175c78dba60903420a8b
Author: Jinchao Wang <wangjinchao600@gmail.com>

x86/hw_breakpoints: Add arch_modify_local_hw_breakpoint_addr() API

Wprobe needs to move an active per-CPU watchpoint without releasing and
reserving its hardware slot.

Add arch_modify_local_hw_breakpoint_addr() as the architecture backend for
modify_local_hw_breakpoint_addr(). It validates the new address, finds the
installed local slot, and updates the hardware debug register and per-CPU
shadow register.

Link: https://lore.kernel.org/all/59637b96946653393a7ad3c7de094094796b39c2.1785067572.git.wangjinchao600@gmail.com/

> diff --git a/arch/x86/kernel/hw_breakpoint.c b/arch/x86/kernel/hw_breakpoint.c
> index 312550be3ad2b..99f0926298a85 100644
> --- a/arch/x86/kernel/hw_breakpoint.c
> +++ b/arch/x86/kernel/hw_breakpoint.c
[ ... ]
> @@ -129,6 +129,62 @@ int arch_install_hw_breakpoint(struct perf_event *bp)
[ ... ]
> +	do {
> +		seq = this_cpu_inc_return(cpu_dr7_seq);
> +
> +		/* Disable breakpoint in DR7 before updating address register */
> +		dr7 = this_cpu_read(cpu_dr7);
> +		set_debugreg((dr7 & ~__encode_dr7(i, info->len, info->type)) | DR7_FIXED_1, 7);

[Severity: High]
Does arch_modify_local_hw_breakpoint_addr() need to clear the breakpoint in
the shadow cpu_dr7 state as well?

Since the breakpoint isn't cleared in the cpu_dr7 shadow state, could an NMI
that interrupts execution here inadvertently re-enable the breakpoint
prematurely?

If an NMI handler (such as kgdb or perf) modifies breakpoints, it increments
cpu_dr7_seq and invokes local_db_save() and local_db_restore(). When the NMI
completes, local_db_restore() detects the sequence mismatch and restores
hardware DR7 using the cpu_dr7 shadow cache. Because cpu_dr7 still has this
breakpoint enabled, the target breakpoint gets re-enabled in hardware DR7
before the execution below resumes.

> +		barrier();
> +
> +		this_cpu_write(cpu_debugreg[i], addr);
> +		barrier();
> +		set_debugreg(addr, i);

[Severity: High]
If an NMI prematurely restored DR7 as described above, won't this modify the
debug address register while the breakpoint is enabled in DR7?

Modifying a debug address register while its corresponding DR7 enable bit is
set violates the Intel SDM and causes spurious #DB exceptions.

While the sequence loop retry mechanism will catch the mismatch, it only does
so after this instruction has already executed, which is too late to prevent
the spurious exception.

> +		barrier();
> +
> +		/* Re-enable breakpoint in DR7 */
> +		set_debugreg(this_cpu_read(cpu_dr7) | DR7_FIXED_1, 7);
> +		barrier();
> +	} while (seq != this_cpu_read(cpu_dr7_seq));
> +
> +	return 0;
> +}

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