linux-kernel - Re: [PATCH v3] ftrace: Fix possible use-after-free issue in ftrace

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date: Thu, 2 May 2024 17:07:43 -0400
From: Steven Rostedt <rostedt@...dmis.org>
To: Zheng Yejian <zhengyejian1@...wei.com>
Cc: <mhiramat@...nel.org>, <mark.rutland@....com>,
 <mathieu.desnoyers@...icios.com>, <Markus.Elfring@....de>,
 <linux-kernel@...r.kernel.org>, <linux-trace-kernel@...r.kernel.org>
Subject: Re: [PATCH v3] ftrace: Fix possible use-after-free issue in
 ftrace_location()

On Wed, 17 Apr 2024 11:28:30 +0800
Zheng Yejian <zhengyejian1@...wei.com> wrote:

> diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
> index da1710499698..e05d3e3dc06a 100644
> --- a/kernel/trace/ftrace.c
> +++ b/kernel/trace/ftrace.c
> @@ -1581,7 +1581,7 @@ static struct dyn_ftrace *lookup_rec(unsigned long start, unsigned long end)
>  }
>  
>  /**
> - * ftrace_location_range - return the first address of a traced location
> + * ftrace_location_range_rcu - return the first address of a traced location

kerneldoc comments are for external functions. You need to move this down
to ftrace_location_range() as here you are commenting a local static function.

But I have to ask, why did you create this static function anyway? There's
only one user of it (the ftrace_location_range()). Why didn't you just
simply add the rcu locking there?

unsigned long ftrace_location_range(unsigned long start, unsigned long end)
{
	struct dyn_ftrace *rec;
	unsigned long ip = 0;

	rcu_read_lock();
	rec = lookup_rec(start, end);
	if (rec)
		ip = rec->ip;
	rcu_read_unlock();

	return ip;
}

-- Steve


>   *	if it touches the given ip range
>   * @start: start of range to search.
>   * @end: end of range to search (inclusive). @end points to the last byte
> @@ -1592,7 +1592,7 @@ static struct dyn_ftrace *lookup_rec(unsigned long start, unsigned long end)
>   * that is either a NOP or call to the function tracer. It checks the ftrace
>   * internal tables to determine if the address belongs or not.
>   */
> -unsigned long ftrace_location_range(unsigned long start, unsigned long end)
> +static unsigned long ftrace_location_range_rcu(unsigned long start, unsigned long end)
>  {
>  	struct dyn_ftrace *rec;
>  
> @@ -1603,6 +1603,16 @@ unsigned long ftrace_location_range(unsigned long start, unsigned long end)
>  	return 0;
>  }
>  
> +unsigned long ftrace_location_range(unsigned long start, unsigned long end)
> +{
> +	unsigned long loc;
> +
> +	rcu_read_lock();
> +	loc = ftrace_location_range_rcu(start, end);
> +	rcu_read_unlock();
> +	return loc;
> +}