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

From: "Steven Rostedt (VMware)" <rostedt@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@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@canonical.com>
---
 kernel/trace/ftrace.c | 40 ++++++++++++++++++++++++----------------
 1 file changed, 24 insertions(+), 16 deletions(-)

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@canonical.com> wrote:
>
> From: "Steven Rostedt (VMware)" <rostedt@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@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@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@lists.ubuntu.com
> https://lists.ubuntu.com/mailman/listinfo/kernel-team

On Mon, Oct 28, 2024 at 07:45:21PM +0800, Jian Hui Lee wrote:
> 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@canonical.com> wrote:
> >
> > From: "Steven Rostedt (VMware)" <rostedt@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@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@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.

Thank you for reviewing. If it looks different, please apply this patch
temporarily and see the changes with --diff-algorithm=patience (i.e.,
`git log -1 -p --patience`? You should observe a diff styled quite
similarly to what you see you run `git log -1 -p 7e16f581a817`.

> 
> > +       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@lists.ubuntu.com
> > https://lists.ubuntu.com/mailman/listinfo/kernel-team

On Mon, Oct 28, 2024 at 8:40 PM Koichiro Den <koichiro.den@canonical.com> wrote:
>
> On Mon, Oct 28, 2024 at 07:45:21PM +0800, Jian Hui Lee wrote:
> > 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@canonical.com> wrote:
> > >
> > > From: "Steven Rostedt (VMware)" <rostedt@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@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@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.
>
> Thank you for reviewing. If it looks different, please apply this patch
> temporarily and see the changes with --diff-algorithm=patience (i.e.,
> `git log -1 -p --patience`? You should observe a diff styled quite
> similarly to what you see you run `git log -1 -p 7e16f581a817`.
>

yes, looks better.

from the perspective of the patch file, it's not straightforward to
review, compared to the version intended for backporting [1]. it would
be appreciated if the patch is well-structured in the first place.
thanks.

1. https://lore.kernel.org/all/20191108213449.887070634@goodmis.org/

> >
> > > +       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@lists.ubuntu.com
> > > https://lists.ubuntu.com/mailman/listinfo/kernel-team