| Message ID | 20230628115329.248450-6-laoar.shao@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | bpf: Support ->fill_link_info for kprobe_multi and perf_event links | expand |
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@gmail.com> > Acked-by: Yonghong Song <yhs@fb.com> > --- > 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? 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;
On Wed, Jul 5, 2023 at 4:19 PM Daniel Borkmann <daniel@iogearbox.net> 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@gmail.com> > > Acked-by: Yonghong Song <yhs@fb.com> > > --- > > 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? Agree. Will change it. > > 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;
On Wed, Jul 5, 2023 at 4:19 PM Daniel Borkmann <daniel@iogearbox.net> 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@gmail.com> > > Acked-by: Yonghong Song <yhs@fb.com> > > --- > > 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;
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; + } #endif }
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@gmail.com> Acked-by: Yonghong Song <yhs@fb.com> --- kernel/trace/bpf_trace.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)