Re: [PATCH] x86/alternative: fix race in try_get_desc()

[Nadav Amit <namit@vmware.com>]

On Sep 20, 2022, at 3:47 PM, Nadav Amit <nadav.amit@gmail.com> wrote:

> From: Nadav Amit <namit@vmware.com>
> 
> 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. ]
> 
> I encountered some occasional crashes of poke_int3_handler() when
> kprobes are set, while accessing desc->vec. The analysis has been done
> offline and I did not corroborate the cause of the crashes. Yet, it
> seems that this race might be the root cause.
> 
> 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 on the stack.
> Anyhow, there is only a single descriptor at any given moment.
> 
> Fixes: 1f676247f36a4 ("x86/alternatives: Implement a better poke_int3_handler() completion scheme"

A bracket is mistakenly missing after the patch title. Sorry.