Date:   Wed, 15 Feb 2023 23:55:17 +0900
From:   Masami Hiramatsu (Google) <mhiramat@kernel.org>
To:     Yang Jihong <yangjihong1@huawei.com>
Cc:     <tglx@linutronix.de>, <mingo@redhat.com>, <bp@alien8.de>,
        <dave.hansen@linux.intel.com>, <x86@kernel.org>, <hpa@zytor.com>,
        <naveen.n.rao@linux.ibm.com>, <anil.s.keshavamurthy@intel.com>,
        <davem@davemloft.net>, <ast@kernel.org>, <peterz@infradead.org>,
        <linux-kernel@vger.kernel.org>,
        <linux-trace-kernel@vger.kernel.org>
Subject: Re: [PATCH 1/3] kprobes: Fixed probe nodes not correctly removed
 when forcibly unoptimized
Message-Id: <20230215235517.800c5089318b5353e7304b67@kernel.org>
In-Reply-To: <20230215115430.236046-2-yangjihong1@huawei.com>
References: <20230215115430.236046-1-yangjihong1@huawei.com>
        <20230215115430.236046-2-yangjihong1@huawei.com>
Mime-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit
Precedence: bulk

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>