Re: [PATCH 1/3] kprobes: Fixed probe nodes not correctly removed when forcibly unoptimized

[Google]

Hi Yang,

Thanks for reporting, but maybe this is a part of following fix.

https://lore.kernel.org/all/167448024501.3253718.13037333683110512967.stgit@devnote3/

Can you confirm that fixes the same issue?

Thank you,

On Wed, 15 Feb 2023 19:54:28 +0800
Yang Jihong <yangjihong1@xxxxxxxxxx> wrote:

> When unoptimize_kprobe forcibly unoptimize the kprobe, simply queue it in
> the freeing_list, and do_free_cleaned_kprobes directly reclaims the kprobe
> if unoptimizing_list is empty (see do_unoptimize_kprobes), which may cause
> WARN or UAF problems.
> 
> The specific scenarios are as follows:
> 
>                           Thread1
> arm_kprobe(p)
>   mutex_lock(&kprobe_mutex)
>   __arm_kprobe(kp)
>     p = get_optimized_kprobe(p->addr)
>     if (unlikely(_p))
>       unoptimize_kprobe(_p, true)  // now _p is queued in freeing_list
>   mutex_unlock(&kprobe_mutex)
> 
>                            Thread2
> kprobe_optimizer
>   mutex_lock(&kprobe_mutex)
>   do_unoptimize_kprobes
>     if (list_empty(&unoptimizing_list))
>       return;  // here directly returned and does not process freeing_list.
>   ...
>   do_free_cleaned_kprobes
>     foreach op in freeing_list:
>       WARN_ON_ONCE(!kprobe_unused(&op->kp))  // WANR will be triggered here.
>       free_aggr_kprobe((&op->kp)  // Delete op->kp directly, if access hash
>                                   // list later, UAF problem will be triggered.
>   mutex_unlock(&kprobe_mutex)
> 
> The freeing_list needs to be processed in do_unoptimize_kprobes regardless
> of whether unoptimizing_list is empty.
> 
> Signed-off-by: Yang Jihong <yangjihong1@xxxxxxxxxx>
> ---
>  kernel/kprobes.c | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/kernel/kprobes.c b/kernel/kprobes.c
> index 1c18ecf9f98b..0730e595f4c1 100644
> --- a/kernel/kprobes.c
> +++ b/kernel/kprobes.c
> @@ -556,10 +556,9 @@ static void do_unoptimize_kprobes(void)
>  	lockdep_assert_cpus_held();
>  
>  	/* Unoptimization must be done anytime */
> -	if (list_empty(&unoptimizing_list))
> -		return;
> +	if (!list_empty(&unoptimizing_list))
> +		arch_unoptimize_kprobes(&unoptimizing_list, &freeing_list);
>  
> -	arch_unoptimize_kprobes(&unoptimizing_list, &freeing_list);
>  	/* Loop on 'freeing_list' for disarming */
>  	list_for_each_entry_safe(op, tmp, &freeing_list, list) {
>  		/* Switching from detour code to origin */
> -- 
> 2.30.GIT
> 


-- 
Masami Hiramatsu (Google) <mhiramat@xxxxxxxxxx>