Re: [PATCH v14 04/14] x86/hw_breakpoints: Make DR7 updates NMI safe

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [Critical] Preexisting race condition in `local_db_save()` leads to recursive exceptions. The function reads DR7, then checks if it was non-zero before disabling hardware breakpoints. If an NMI (e.g., KGDB) installs a breakpoint after DR7 is read but before the check, `local_db_save()` will falsely believe no breakpoints are active and skip disabling them. The exception handler (e.g., `#DB` or `#MC`) will then run with hardware breakpoints enabled, risking a recursive exception.
- [High] The patch adds `if (!dr7) return;` in `local_db_restore()` to ostensibly prevent an NMI from overwriting an intentionally cleared hardware DR7 state (such as during KVM guest entry). However, if an NMI (e.g., from KGDB) installs a hardware breakpoint, `arch_install_hw_breakpoint()` directly overwrites the hardware DR7 register with `cpu_dr7`. By returning immediately when `dr7 == 0`, `local_db_restore()` fails to restore the hardware DR7 back to its cleared state (0x400). Consequently, KVM resumes the guest with the host's newly installed hardware breakpoint active.
--

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

x86/hw_breakpoints: Make DR7 updates NMI safe

Hardware breakpoint installation and removal run with IRQs disabled, but
an NMI can still enter the same code through KGDB. This patch uses cmpxchg
and a per-CPU sequence number to make DR7 updates safe from NMI interruptions.

Link: https://lore.kernel.org/all/4ee0a2efc9e8387af83286b8495b7d490247e165.1785067572.git.wangjinchao600@gmail.com/

> diff --git a/arch/x86/include/asm/debugreg.h b/arch/x86/include/asm/debugreg.h
> index a2c1f2d24b645..65e80cd62193f 100644
> --- a/arch/x86/include/asm/debugreg.h
> +++ b/arch/x86/include/asm/debugreg.h

[ ... ]

> @@ -125,18 +126,20 @@ static __always_inline bool hw_breakpoint_active(void)
>  
>  extern void hw_breakpoint_restore(void);
>  
> -static __always_inline unsigned long local_db_save(void)
> +static __always_inline void local_db_save(unsigned long *dr7,
> +					  unsigned int *dr7_seq)
>  {
> -	unsigned long dr7;
> +	*dr7 = 0;
> +	*dr7_seq = this_cpu_read(cpu_dr7_seq);
>  
>  	if (static_cpu_has(X86_FEATURE_HYPERVISOR) && !hw_breakpoint_active())
> -		return 0;
> +		return;
>  
> -	get_debugreg(dr7, 7);
> +	get_debugreg(*dr7, 7);
>  
>  	/* Architecturally set bit */
> -	dr7 &= ~DR7_FIXED_1;
> -	if (dr7)
> +	*dr7 &= ~DR7_FIXED_1;
> +	if (*dr7)
>  		set_debugreg(DR7_FIXED_1, 7);

[Severity: Critical]
This is a pre-existing issue, but does this conditional check rely on a stale
read of DR7?

If an NMI (e.g. KGDB) installs a breakpoint after DR7 is read via
get_debugreg() but before this check, local_db_save() will evaluate the old 0
and skip disabling DR7. Can this cause the exception handler (such as #DB or
#MC) to run with hardware breakpoints still enabled, risking a recursive
exception?

>  
>  	/*
> @@ -145,20 +148,34 @@ static __always_inline unsigned long local_db_save(void)
>  	 * be good.
>  	 */
>  	barrier();
> -
> -	return dr7;
>  }
>  
> -static __always_inline void local_db_restore(unsigned long dr7)
> +static __always_inline void local_db_restore(unsigned long dr7,
> +					     unsigned int dr7_seq)
>  {
> +	unsigned int seq;
> +
>  	/*
>  	 * Ensure the compiler doesn't raise this statement into
>  	 * the critical section; enabling breakpoints early would
>  	 * not be good.
>  	 */
>  	barrier();
> -	if (dr7)
> +
> +	if (!dr7)
> +		return;

[Severity: High]
This isn't a bug introduced by this patch, but if an NMI interrupts a
cleared DR7 state (such as during KVM guest entry), and the NMI installs
a hardware breakpoint, doesn't this early return fail to restore the hardware
DR7 back to its cleared state (0x400)?

Consequently, it looks like KVM would resume the guest with the host's
newly installed hardware breakpoint active.

[ ... ]

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

[ ... ]

> @@ -97,38 +100,31 @@ int decode_dr7(unsigned long dr7, int bpnum, unsigned *len, unsigned *type)
>  int arch_install_hw_breakpoint(struct perf_event *bp)
>  {
>  	struct arch_hw_breakpoint *info = counter_arch_bp(bp);
> -	unsigned long *dr7;
> +	unsigned int seq;
>  	int i;
>  
>  	lockdep_assert_irqs_disabled();
>  
>  	for (i = 0; i < HBP_NUM; i++) {
> -		struct perf_event **slot = this_cpu_ptr(&bp_per_reg[i]);
> -
> -		if (!*slot) {
> -			*slot = bp;
> +		if (!this_cpu_cmpxchg(bp_per_reg[i], NULL, bp))
>  			break;
> -		}
>  	}
>  
>  	if (WARN_ONCE(i == HBP_NUM, "Can't find any breakpoint slot"))
>  		return -EBUSY;
>  
> -	set_debugreg(info->address, i);
> -	__this_cpu_write(cpu_debugreg[i], info->address);
> -
> -	dr7 = this_cpu_ptr(&cpu_dr7);
> -	*dr7 |= encode_dr7(i, info->len, info->type);
> -
> -	/*
> -	 * 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.
> -	 */
> -	barrier();
> -
> -	set_debugreg(*dr7, 7);
> -	if (info->mask)
> -		amd_set_dr_addr_mask(info->mask, i);
> +	do {
> +		seq = this_cpu_inc_return(cpu_dr7_seq);
> +		this_cpu_write(cpu_debugreg[i], info->address);
> +		barrier();
> +		set_debugreg(info->address, i);
> +		if (info->mask)
> +			amd_set_dr_addr_mask(info->mask, i);
> +		this_cpu_or(cpu_dr7, encode_dr7(i, info->len, info->type));
> +		barrier();
> +		set_debugreg(this_cpu_read(cpu_dr7), 7);

[Severity: High]
This is a pre-existing issue, but since arch_install_hw_breakpoint()
unconditionally overwrites the hardware DR7 register inside the NMI,
does this mean the KVM cleared state is already lost here before
local_db_restore() is even called?

> +		barrier();
> +	} while (seq != this_cpu_read(cpu_dr7_seq));
>  
>  	return 0;
>  }

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