Re: [PATCH v6 bpf-next 05/11] bpf: Clear the probe_addr for uprobe

[Yafang Shao <laoar.shao@xxxxxxxxx>]

On Wed, Jul 5, 2023 at 4:19 PM Daniel Borkmann <daniel@xxxxxxxxxxxxx> wrote:
>
> On 6/28/23 1:53 PM, Yafang Shao wrote:
> > To avoid returning uninitialized or random values when querying the file
> > descriptor (fd) and accessing probe_addr, it is necessary to clear the
> > variable prior to its use.
> >
> > Fixes: 41bdc4b40ed6 ("bpf: introduce bpf subcommand BPF_TASK_FD_QUERY")
> > Signed-off-by: Yafang Shao <laoar.shao@xxxxxxxxx>
> > Acked-by: Yonghong Song <yhs@xxxxxx>
> > ---
> >   kernel/trace/bpf_trace.c | 4 +++-
> >   1 file changed, 3 insertions(+), 1 deletion(-)
> >
> > diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
> > index 1f9f78e1992f..ac9958907a7c 100644
> > --- a/kernel/trace/bpf_trace.c
> > +++ b/kernel/trace/bpf_trace.c
> > @@ -2382,10 +2382,12 @@ int bpf_get_perf_event_info(const struct perf_event *event, u32 *prog_id,
> >                                                 event->attr.type == PERF_TYPE_TRACEPOINT);
> >   #endif
> >   #ifdef CONFIG_UPROBE_EVENTS
> > -             if (flags & TRACE_EVENT_FL_UPROBE)
> > +             if (flags & TRACE_EVENT_FL_UPROBE) {
> >                       err = bpf_get_uprobe_info(event, fd_type, buf,
> >                                                 probe_offset,
> >                                                 event->attr.type == PERF_TYPE_TRACEPOINT);
> > +                     *probe_addr = 0x0;
> > +             }
>
> Could we make this a bit more robust by just moving the zero'ing into the common path?

After a second thought, I prefer to clear it in bpf_get_uprobe_info().
That way we can avoid setting them twice for kprobe.

diff --git a/kernel/trace/trace_uprobe.c b/kernel/trace/trace_uprobe.c
index 8b92e34..015dbf2 100644
--- a/kernel/trace/trace_uprobe.c
+++ b/kernel/trace/trace_uprobe.c
@@ -1432,6 +1432,7 @@ int bpf_get_uprobe_info(const struct perf_event
*event, u32 *fd_type,
                                    : BPF_FD_TYPE_UPROBE;
        *filename = tu->filename;
        *probe_offset = tu->offset;
+       *probe_addr = 0;
        return 0;
 }
 #endif /* CONFIG_PERF_EVENTS */


>
> diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
> index 03b7f6b8e4f0..795e16d5d2f7 100644
> --- a/kernel/trace/bpf_trace.c
> +++ b/kernel/trace/bpf_trace.c
> @@ -2362,6 +2362,9 @@ int bpf_get_perf_event_info(const struct perf_event *event, u32 *prog_id,
>                  return -EOPNOTSUPP;
>
>          *prog_id = prog->aux->id;
> +       *probe_offset = 0x0;
> +       *probe_addr = 0x0;
> +
>          flags = event->tp_event->flags;
>          is_tracepoint = flags & TRACE_EVENT_FL_TRACEPOINT;
>          is_syscall_tp = is_syscall_trace_event(event->tp_event);
> @@ -2370,8 +2373,6 @@ int bpf_get_perf_event_info(const struct perf_event *event, u32 *prog_id,
>                  *buf = is_tracepoint ? event->tp_event->tp->name
>                                       : event->tp_event->name;
>                  *fd_type = BPF_FD_TYPE_TRACEPOINT;
> -               *probe_offset = 0x0;
> -               *probe_addr = 0x0;
>          } else {
>                  /* kprobe/uprobe */
>                  err = -EOPNOTSUPP;



-- 
Regards
Yafang