Re: [PATCH v15 02/12] x86/hw_breakpoints: Make DR7 updates NMI safe

[sashiko-bot@kernel.org]

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

New issues:
- [High] local_db_restore() incorrectly uses dr7 == 0 to discard NMI-installed hardware breakpoints, permanently losing debug state and corrupting KVM guest debug registers.
- [High] local_db_save() destructively clears hardware DR7 inside its sequence retry loop, permanently corrupting the restored state if a retry occurs.

Pre-existing issues:
- [High] The early return in local_db_save() bypasses the sequence retry loop, leaving a race window where an NMI can leave hardware breakpoints enabled during critical exception handlers.
--

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

x86/hw_breakpoints: Make DR7 updates NMI safe

This commit introduces atomic per-CPU operations and sequence numbers to
make DR7 hardware breakpoint updates NMI safe. It updates local_db_save()
and local_db_restore() to use retry loops that detect NMI interruptions,
preventing state corruption.

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 854d82b88ff4e..3399e166d3044 100644
> --- a/arch/x86/include/asm/debugreg.h
> +++ b/arch/x86/include/asm/debugreg.h
[ ... ]
> @@ -125,40 +126,66 @@ 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;
> +	do {
> +		*dr7_seq = this_cpu_read(cpu_dr7_seq);
> +		*dr7 = 0;
>  
> -	if (cpu_feature_enabled(X86_FEATURE_HYPERVISOR) && !hw_breakpoint_active())
> -		return 0;
> -
> -	get_debugreg(dr7, 7);
> +		if (cpu_feature_enabled(X86_FEATURE_HYPERVISOR) &&
> +		    !hw_breakpoint_active())
> +			return;

[Severity: High]
This isn't a bug introduced by this patch, but could this early return
in local_db_save() bypass the sequence retry loop?

If an NMI hits exactly before the return instruction and installs a
breakpoint, the function exits immediately. This leaves the newly installed
hardware breakpoint active during exception handlers like MCE, which could
trigger a recursive #DB exception and lead to a kernel panic.

>  
> +		get_debugreg(*dr7, 7);
>  
> -	/* Architecturally set bit */
> -	dr7 &= ~DR7_FIXED_1;
> -	if (dr7)
> -		set_debugreg(DR7_FIXED_1, 7);
> +		/* Architecturally set bit */
> +		*dr7 &= ~DR7_FIXED_1;
> +		if (*dr7)
> +			set_debugreg(DR7_FIXED_1, 7);

[Severity: High]
Can this destructively overwrite the hardware DR7 state inside the sequence
retry loop?

If an NMI interrupts execution between reading and clearing DR7, installs a
breakpoint, and increments cpu_dr7_seq, this set_debugreg() call will
overwrite the NMI's new hardware state with 0x400.

When the loop detects the sequence mismatch and retries, the next
iteration's get_debugreg() will read this clobbered 0x400 value instead of
the state installed by the NMI, permanently corrupting the restored state.

>  
> -	/*
> -	 * Ensure the compiler doesn't lower the above statements into
> -	 * the critical section; disabling breakpoints late would not
> -	 * be good.
> -	 */
> -	barrier();
> +		/*
> +		 * Ensure the compiler doesn't lower the above statements into
> +		 * the critical section; disabling breakpoints late would not
> +		 * be good.
> +		 */
> +		barrier();
> +	} while (unlikely(*dr7_seq != this_cpu_read(cpu_dr7_seq)));
>  }
[ ... ]
> -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 long val;
> +	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)
> -		set_debugreg(dr7, 7);
> +
> +	do {
> +		seq = this_cpu_read(cpu_dr7_seq);
> +		if (seq == dr7_seq) {
> +			if (!dr7)
> +				return;
> +			val = dr7;
> +		} else {
> +			/*
> +			 * If an NMI modified breakpoints while DR7 was saved as 0
> +			 * (e.g. KVM guest entry), arch_install_hw_breakpoint()
> +			 * clobbered hardware DR7. Restore val = 0 (DR7_FIXED_1)
> +			 * to clean up the NMI's write and preserve guest isolation.
> +			 * Otherwise, load the latest cpu_dr7 state.
> +			 */
> +			val = dr7 ? this_cpu_read(cpu_dr7) : 0;

[Severity: High]
Does this logic permanently discard the hardware breakpoint installed by an
NMI if dr7 was initially 0?

If dr7 was saved as 0, which happens when no host breakpoints were active,
and an NMI subsequently installs a breakpoint, this logic evaluates to 0
and ignores the new cpu_dr7 state.

It will then write 0x400 to hardware DR7, completely overriding the NMI
update. Furthermore, if an NMI interrupts a KVM guest, the guest's DR7
state is completely destroyed and replaced with 0x400.

> +		}
> +
> +		set_debugreg(val | DR7_FIXED_1, 7);
> +		barrier();
> +	} while (unlikely(seq != this_cpu_read(cpu_dr7_seq)));
>  }

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