NACK: [SRU][F][PATCH v2 1/3] ftrace: Separate out functionality from ftrace_location_range()

[Jian Hui Lee]

Hi Koichiro,

this backported patch does not look right. i found some unnecessary
modifications.

On Fri, Oct 25, 2024 at 9:43 PM Koichiro Den <koichiro.den at canonical.com> wrote:
>
> From: "Steven Rostedt (VMware)" <rostedt at goodmis.org>
>
> Create a new function called lookup_rec() from the functionality of
> ftrace_location_range(). The difference between lookup_rec() is that it
> returns the record that it finds, where as ftrace_location_range() returns
> only if it found a match or not.
>
> The lookup_rec() is static, and can be used for new functionality where
> ftrace needs to find a record of a specific address.
>
> Signed-off-by: Steven Rostedt (VMware) <rostedt at goodmis.org>
> (backported from commit 7e16f581a81759bafea04d049134b32d1a881226)
> [koichiroden: Prerequisite for "ftrace: Fix possible use-after-free
> issue in ftrace_location()". Adjusted context due to an extra commit
> 170d0fe552d0 in this tree, backported from v5.4.238 ac58b88ccbbb
> ("ftrace: Fix invalid address access in lookup_rec() when index is 0")]
> CVE-2024-38588
> Signed-off-by: Koichiro Den <koichiro.den at canonical.com>
> ---
>  kernel/trace/ftrace.c | 40 ++++++++++++++++++++++++----------------
>  1 file changed, 24 insertions(+), 16 deletions(-)
>
> diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
> index 412505d94865..d9992fef7287 100644
> --- a/kernel/trace/ftrace.c
> +++ b/kernel/trace/ftrace.c
> @@ -1535,22 +1535,10 @@ static int ftrace_cmp_recs(const void *a, const void *b)
>         return 0;
>  }
>
> -/**
> - * ftrace_location_range - return the first address of a traced location
> - *     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
> - *     to check.
> - *
> - * Returns rec->ip if the related ftrace location is a least partly within
> - * the given address range. That is, the first address of the instruction
> - * 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)

7e16f581a817 (ftrace: Separate out functionality from
ftrace_location_range()) does not have the above changes.

> +/**
> + * ftrace_location_range - return the first address of a traced location
> + *     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
> + *     to check.
> + *
> + * Returns rec->ip if the related ftrace location is a least partly within
> + * the given address range. That is, the first address of the instruction
> + * 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)
> +{

7e16f581a817 (ftrace: Separate out functionality from
ftrace_location_range()) does not have the above changes.

looks like you adjusted the lines of code unintentionally while backporting.

> +       struct dyn_ftrace *rec;
> +
> +       rec = lookup_rec(start, end);
> +       if (rec)
> +               return rec->ip;
>
>         return 0;
>  }
> --
> 2.43.0
>
>
> --
> kernel-team mailing list
> kernel-team at lists.ubuntu.com
> https://lists.ubuntu.com/mailman/listinfo/kernel-team