x86/alternative: Fix race in try_get_desc()
authorNadav Amit <namit@vmware.com>
Wed, 21 Sep 2022 18:09:32 +0000 (18:09 +0000)
committerPeter Zijlstra <peterz@infradead.org>
Tue, 27 Sep 2022 20:50:26 +0000 (22:50 +0200)
commitefd608fa7403ba106412b437f873929e2c862e28
tree1d0e325bc72821108986b1dcd850bad2679da997
parente400ad8b7e6a1b9102123c6240289a811501f7d9
x86/alternative: Fix race in try_get_desc()

I encountered some occasional crashes of poke_int3_handler() when
kprobes are set, while accessing desc->vec.

The text poke mechanism claims to have an RCU-like behavior, but it
does not appear that there is any quiescent state to ensure that
nobody holds reference to desc. As a result, the following race
appears to be possible, which can lead to memory corruption.

  CPU0 CPU1
  ---- ----
  text_poke_bp_batch()
  -> smp_store_release(&bp_desc, &desc)

  [ notice that desc is on
    the stack ]

poke_int3_handler()

[ int3 might be kprobe's
  so sync events are do not
  help ]

-> try_get_desc(descp=&bp_desc)
   desc = __READ_ONCE(bp_desc)

   if (!desc) [false, success]
  WRITE_ONCE(bp_desc, NULL);
  atomic_dec_and_test(&desc.refs)

  [ success, desc space on the stack
    is being reused and might have
    non-zero value. ]
arch_atomic_inc_not_zero(&desc->refs)

[ might succeed since desc points to
  stack memory that was freed and might
  be reused. ]

Fix this issue with small backportable patch. Instead of trying to
make RCU-like behavior for bp_desc, just eliminate the unnecessary
level of indirection of bp_desc, and hold the whole descriptor as a
global.  Anyhow, there is only a single descriptor at any given
moment.

Fixes: 1f676247f36a4 ("x86/alternatives: Implement a better poke_int3_handler() completion scheme")
Signed-off-by: Nadav Amit <namit@vmware.com>
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Cc: stable@kernel.org
Link: https://lkml.kernel.org/r/20220920224743.3089-1-namit@vmware.com
arch/x86/kernel/alternative.c