Received-SPF: pass (google.com: domain of linux-kernel+bounces-135331-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:40f1:3f00::1 as permitted sender) client-ip=2604:1380:40f1:3f00::1;
Date: Mon, 8 Apr 2024 21:41:02 +0900
From: Masami Hiramatsu (Google) <mhiramat@kernel.org>
To: Zheng Yejian <zhengyejian1@huawei.com>
Cc: <naveen.n.rao@linux.ibm.com>, <anil.s.keshavamurthy@intel.com>,
 <davem@davemloft.net>, <linux-kernel@vger.kernel.org>,
 <linux-trace-kernel@vger.kernel.org>
Subject: Re: [PATCH v2] kprobes: Avoid possible warn in
 __arm_kprobe_ftrace()
Message-Id: <20240408214102.be792c5cefd5ab757ef32a14@kernel.org>
In-Reply-To: <20240408083403.3302274-1-zhengyejian1@huawei.com>
References: <20240407035904.2556645-1-zhengyejian1@huawei.com>
	<20240408083403.3302274-1-zhengyejian1@huawei.com>
Precedence: bulk
Mime-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit

Hi Zheng,

On Mon, 8 Apr 2024 16:34:03 +0800
Zheng Yejian <zhengyejian1@huawei.com> wrote:

> There is once warn in __arm_kprobe_ftrace() on:
> 
>  ret = ftrace_set_filter_ip(ops, (unsigned long)p->addr, 0, 0);
>  if (WARN_ONCE(..., "Failed to arm kprobe-ftrace at %pS (error %d)\n", ...)
>    return ret;
> 
> This warning is generated because 'p->addr' is detected to be not a valid
> ftrace location in ftrace_set_filter_ip(). The ftrace address check is done
> by check_ftrace_location() at the beginning of check_kprobe_address_safe().
> At that point, ftrace_location(addr) == addr should return true if the
> module is loaded. Then the module is searched twice:
>   1. in is_module_text_address(), we find that 'p->addr' is in a module;
>   2. in __module_text_address(), we find the module;
> 
> If the module has just been unloaded before the second search, then
> '*probed_mod' is NULL and we would not go to get the module refcount,
> then the return value of check_kprobe_address_safe() would be 0, but
> actually we need to return -EINVAL.

OK, so you found a race window in check_kprobe_address_safe().

It does something like below.

check_kprobe_address_safe() {
	...

	/* Timing [A] */

	if (!(core_kernel_text(p->addr) ||
		is_module_text_address(p->addr)) ||
		...(other reserved address check)) {
		return -EINVAL;
	}

	/* Timing [B] */

	*probed_mod = __module_text_address(p->addr):
	if (*probe_mod) {
		if (!try_module_get(*probed_mod)) {
			return -ENOENT;
		}
		...	
	}
}

So, if p->addr is in a module which is alive at the timing [A], but
unloaded at timing [B], 'p->addr' is passed the
'is_module_text_address(p->addr)' check, but *probed_mod becomes NULL.
Thus the corresponding module is not referenced and kprobe_arm(p) will
access a wrong address (use after free).
This happens either kprobe on ftrace is enabled or not.

To fix this problem, we should move the mutex_lock(kprobe_mutex) before
check_kprobe_address_safe() because kprobe_module_callback() also lock it
so it can stop module unloading.

Can you ensure this will fix your problem?
I think your patch is just optimizing but not fixing the fundamental
problem, which is we don't have an atomic search symbol and get module
API. In that case, we should stop a whole module unloading system until
registering a new kprobe on a module. (After registering the kprobe,
the callback can mark it gone and disarm_kprobe does not work anymore.)

diff --git a/kernel/kprobes.c b/kernel/kprobes.c
index 9d9095e81792..94eaefd1bc51 100644
--- a/kernel/kprobes.c
+++ b/kernel/kprobes.c
@@ -1633,11 +1633,11 @@ int register_kprobe(struct kprobe *p)
 	p->nmissed = 0;
 	INIT_LIST_HEAD(&p->list);
 
+	mutex_lock(&kprobe_mutex);
+
 	ret = check_kprobe_address_safe(p, &probed_mod);
 	if (ret)
-		return ret;
-
-	mutex_lock(&kprobe_mutex);
+		goto out;
 
 	if (on_func_entry)
 		p->flags |= KPROBE_FLAG_ON_FUNC_ENTRY;

----

Thank you,

> 
> To fix it, originally we can simply check 'p->addr' is out of text again,
> like below. But that would check twice respectively in kernel text and
> module text, so finally I reduce them to be once.
> 
>   if (!(core_kernel_text((unsigned long) p->addr) ||
>       is_module_text_address((unsigned long) p->addr)) || ...) {
> 	ret = -EINVAL;
> 	goto out;
>   }
>   ...
>   *probed_mod = __module_text_address((unsigned long) p->addr);
>   if (*probed_mod) {
> 	...
>   } else if (!core_kernel_text((unsigned long) p->addr)) { // check again!
> 	ret = -EINVAL;
> 	goto out;
>   }
> 
> Signed-off-by: Zheng Yejian <zhengyejian1@huawei.com>
> ---
>  kernel/kprobes.c | 18 ++++++++++++------
>  1 file changed, 12 insertions(+), 6 deletions(-)
> 
> v2:
>  - Update commit messages and comments as suggested by Masami.
>    Link: https://lore.kernel.org/all/20240408115038.b0c85767bf1f249eccc32fff@kernel.org/
> 
> v1:
>  - Link: https://lore.kernel.org/all/20240407035904.2556645-1-zhengyejian1@huawei.com/
> 
> diff --git a/kernel/kprobes.c b/kernel/kprobes.c
> index 9d9095e81792..65adc815fc6e 100644
> --- a/kernel/kprobes.c
> +++ b/kernel/kprobes.c
> @@ -1567,10 +1567,17 @@ static int check_kprobe_address_safe(struct kprobe *p,
>  	jump_label_lock();
>  	preempt_disable();
>  
> -	/* Ensure it is not in reserved area nor out of text */
> -	if (!(core_kernel_text((unsigned long) p->addr) ||
> -	    is_module_text_address((unsigned long) p->addr)) ||
> -	    in_gate_area_no_mm((unsigned long) p->addr) ||
> +	/* Ensure the address is in a text area, and find a module if exists. */
> +	*probed_mod = NULL;
> +	if (!core_kernel_text((unsigned long) p->addr)) {
> +		*probed_mod = __module_text_address((unsigned long) p->addr);
> +		if (!(*probed_mod)) {
> +			ret = -EINVAL;
> +			goto out;
> +		}
> +	}
> +	/* Ensure it is not in reserved area. */
> +	if (in_gate_area_no_mm((unsigned long) p->addr) ||
>  	    within_kprobe_blacklist((unsigned long) p->addr) ||
>  	    jump_label_text_reserved(p->addr, p->addr) ||
>  	    static_call_text_reserved(p->addr, p->addr) ||
> @@ -1580,8 +1587,7 @@ static int check_kprobe_address_safe(struct kprobe *p,
>  		goto out;
>  	}
>  
> -	/* Check if 'p' is probing a module. */
> -	*probed_mod = __module_text_address((unsigned long) p->addr);
> +	/* Get module refcount and reject __init functions for loaded modules. */
>  	if (*probed_mod) {
>  		/*
>  		 * We must hold a refcount of the probed module while updating
> -- 
> 2.25.1
> 


-- 
Masami Hiramatsu (Google) <mhiramat@kernel.org>