Re: [PATCH v3] ftrace: Fix possible use-after-free issue in ftrace_location()

[Steven Rostedt]

On Wed, 17 Apr 2024 11:28:30 +0800
Zheng Yejian <zhengyeji...@huawei.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;
> +}