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 <[email protected]> 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.wangjinchao...@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
