Re: [PATCH v13 02/12] 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:

New issues:
- [High] local_db_restore overwrites an intentionally cleared hardware DR7 state (e.g., KVM VMENTER) with the host's shadow cpu_dr7 if an NMI modifies the sequence counter.

Pre-existing issues:
- [High] Task migration during user-mode machine check exceptions corrupts hardware debug registers on the new CPU.
--

--- Patch [2]: [PATCH v13 02/12] x86/hw_breakpoints: Make DR7 updates NMI safe ---
commit 0d8b774ae86048690b3378737c33f417379e85de
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. The interrupted
operation and the NMI can consequently claim the same slot or overwrite
each other's DR7 state.
    
Claim and release per-CPU slots with cmpxchg. Update cpu_dr7 with
single-instruction per-CPU operations, and preserve hardware-first
disable and hardware-last enable ordering. Add a per-CPU sequence number
so interrupted DR7 writers and restore paths detect an NMI update and
retry from the latest shadow state.

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..c41ee981c2303 100644
> --- a/arch/x86/include/asm/debugreg.h
> +++ b/arch/x86/include/asm/debugreg.h
[ ... ]
> @@ -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)
> +
> +	do {
> +		seq = this_cpu_read(cpu_dr7_seq);
> +		if (seq == dr7_seq) {
> +			if (!dr7)
> +				return;
> +		} else {
> +			dr7 = this_cpu_read(cpu_dr7);
> +			if (!dr7)
> +				dr7 = DR7_FIXED_1;
> +		}
> +
>  		set_debugreg(dr7, 7);
> +		barrier();
> +	} while (unlikely(seq != this_cpu_read(cpu_dr7_seq)));
>  }

[Severity: High]
If KVM intentionally clears the hardware DR7 state to isolate the guest from
host breakpoints before VMENTER without modifying the host's cpu_dr7 shadow
state:

arch/x86/kvm/x86.c:vcpu_enter_guest() {
    ...
    if (unlikely(vcpu->arch.switch_db_regs &&
                 !(vcpu->arch.switch_db_regs & KVM_DEBUGREG_AUTO_SWITCH))) {
        set_debugreg(DR7_FIXED_1, 7);
    ...
    } else if (unlikely(hw_breakpoint_active())) {
        set_debugreg(DR7_FIXED_1, 7);
    }
    ...
}

Could an NMI firing shortly after this permanently undo this isolation
upon return?

If an NMI (such as a perf event) fires before vmlaunch/vmresume, exc_nmi
calls local_db_save, saving dr7=0. If the NMI modifies a breakpoint,
cpu_dr7_seq increments.

When exc_nmi calls local_db_restore, the sequence mismatch causes it to
discard the saved dr7=0 state and instead unconditionally restore the host's
active breakpoints (this_cpu_read(cpu_dr7)) to the hardware DR7. This could
leak host hardware breakpoints into a KVM guest and cause unexpected #DB
exceptions.

[ ... ]
> diff --git a/arch/x86/kernel/cpu/mce/core.c b/arch/x86/kernel/cpu/mce/core.c
> index 9bba1e2f03af7..8dba9cd04bfa5 100644
> --- a/arch/x86/kernel/cpu/mce/core.c
> +++ b/arch/x86/kernel/cpu/mce/core.c
[ ... ]
> @@ -2139,20 +2139,22 @@ static __always_inline void exc_machine_check_user(struct pt_regs *regs)
>  DEFINE_IDTENTRY_MCE(exc_machine_check)
>  {
>  	unsigned long dr7;
> +	unsigned int dr7_seq;
>  
> -	dr7 = local_db_save();
> +	local_db_save(&dr7, &dr7_seq);
>  	exc_machine_check_kernel(regs);
> -	local_db_restore(dr7);
> +	local_db_restore(dr7, dr7_seq);
>  }
>  
>  /* The user mode variant. */
>  DEFINE_IDTENTRY_MCE_USER(exc_machine_check)
>  {
>  	unsigned long dr7;
> +	unsigned int dr7_seq;
>  
> -	dr7 = local_db_save();
> +	local_db_save(&dr7, &dr7_seq);
>  	exc_machine_check_user(regs);
> -	local_db_restore(dr7);
> +	local_db_restore(dr7, dr7_seq);
>  }

[Severity: High]
This is a pre-existing issue, but does task migration during user-mode
machine check exceptions corrupt hardware debug registers on the new CPU?

exc_machine_check_user invokes irqentry_exit_to_user_mode, which processes
pending thread work (like TIF_NEED_RESCHED), enabling interrupts and allowing
the task to be scheduled out and migrate to a new CPU:

CPU1
exc_machine_check()
    local_db_save(&dr7, &dr7_seq);
    exc_machine_check_user(regs)
        irqentry_exit_to_user_mode(regs)
            // Task migrates to CPU2

CPU2
    local_db_restore(dr7, dr7_seq);

When it resumes on the new CPU, local_db_restore is executed with the dr7
and dr7_seq values saved from the old CPU. If the new CPU's cpu_dr7_seq
matches the old CPU's saved dr7_seq, it would erroneously write the old CPU's
hardware DR7 state to the new CPU's hardware DR7 register. Could this cause
spurious #DB exceptions or break hardware breakpoints on the new CPU?

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