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@huawei.com>
---
 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

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@huawei.com> 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@huawei.com>
> ---
>  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@kernel.org>

Hello Masami,

On 2023/2/15 22:55, Masami Hiramatsu (Google) wrote:
> 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?
Yes, I've tested and confirmed that the patch you mentioned above fixes 
the same issue.

Will remove it in next version.

Thanks
Yang.