Potential (minor) issues in x86 HW breakpoint code

[Mark Rutland]

Hi,

While looking at reworking some of arm64's debug code, I spotted a
couple of (practically minor) issues in x86 HW breakpoint code. I
mentioned those to Andrew on IRC, and he suggested these probably should
probably be fixed, even if they fall into "don't do that" territory (and
can only be triggered by root/CAP_SYS_ADMIN, etc).

Details below.

1) Potential screaming #DB from arch_uninstall_hw_breakpoint().

  It's possible to set a HW breakpoint on most of the HW breakpoint
  code, including arch_uninstall_hw_breakpoint().

  In arch_uninstall_hw_breakpoint(), bp_per_reg[i] is cleared to NULL
  *before* DR7 is written to disable the breakpoint. If a HW breakpoint
  is placed between those two instructions, hw_breakpoint_handler() will
  find that bp_per_reg[i] is NULL, and won't set RF, leading to the #DB
  being taken ad infinitum with interrupts masked.

  I think that could be fixed by disabling the breakpoint in DR7 *before* 
  clearing bp_per_reg[i] to NULL (so the order would be the opposite of
  arch_install_hw_breakpoint()), or to just forbid HW breakpoints on
  arch_uninstall_hw_breakpoint().

  It might also be worth hw_breakpoint_handler() warning if it finds a
  breakpoint whose bp_per_reg[i] slot is NULL, and maybe setting RF so
  that the kernel can recover.

2) Theoretical potential tearing of accesses to bp_per_reg[i].

   The bp_per_reg[] array is read atomically by hw_breakpoint_handler(),
   but elements are written non-atomically (with plain C accesses) in
   arch_install_hw_breakpoint() and arch_uninstall_hw_breakpoint().

   In practice, those are almost certainly going to be compiled to
   tear-free stores, but *in theory* (and for KCSAN strict mode, IIRC)
   there is potential for tearing.

   Since arch_install_hw_breakpoint() writes to bp_per_reg[i] *before*
   setting the relevant bits in DR7, that's also probably fine. However,
   when arch_uninstall_hw_breakpoint() writes to bp_per_reg[i] it's
   theoretically possible for hw_breakpoint_handler() to see a torn
   store.

   I think that could be fixed by using WRITE_ONCE(), re-ordering
   the store vs write to DR7 such that tearing never matters, or
   forbidding breakpoints here.

Mark.