lkml.org 
[lkml]   [2021]   [Oct]   [25]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
Patch in this message
/
From
Date
SubjectRe: [PATCH v6 08/12] tools/bpf/bpftool/skeleton: make it adopt to task comm size change
On Tue, Oct 26, 2021 at 5:24 AM Kees Cook <keescook@chromium.org> wrote:
>
> On Mon, Oct 25, 2021 at 08:33:11AM +0000, Yafang Shao wrote:
> > bpf_probe_read_kernel_str() will add a nul terminator to the dst, then
> > we don't care about if the dst size is big enough.
> >
> > Signed-off-by: Yafang Shao <laoar.shao@gmail.com>
> > Cc: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
> > Cc: Arnaldo Carvalho de Melo <arnaldo.melo@gmail.com>
> > Cc: Andrii Nakryiko <andrii.nakryiko@gmail.com>
> > Cc: Peter Zijlstra <peterz@infradead.org>
> > Cc: Steven Rostedt <rostedt@goodmis.org>
> > Cc: Al Viro <viro@zeniv.linux.org.uk>
> > Cc: Kees Cook <keescook@chromium.org>
> > Cc: Petr Mladek <pmladek@suse.com>
>
> So, if we're ever going to copying these buffers out of the kernel (I
> don't know what the object lifetime here in bpf is for "e", etc), we
> should be zero-padding (as get_task_comm() does).
>
> Should this, instead, be using a bounce buffer?

The comment in bpf_probe_read_kernel_str_common() says

: /*
: * The strncpy_from_kernel_nofault() call will likely not fill the
: * entire buffer, but that's okay in this circumstance as we're probing
: * arbitrary memory anyway similar to bpf_probe_read_*() and might
: * as well probe the stack. Thus, memory is explicitly cleared
: * only in error case, so that improper users ignoring return
: * code altogether don't copy garbage; otherwise length of string
: * is returned that can be used for bpf_perf_event_output() et al.
: */

It seems that it doesn't matter if the buffer is filled as that is
probing arbitrary memory.

>
> get_task_comm(comm, task->group_leader);

This helper can't be used by the BPF programs, as it is not exported to BPF.

> bpf_probe_read_kernel_str(&e.comm, sizeof(e.comm), comm);
>
> -Kees
>
> > ---
> > tools/bpf/bpftool/skeleton/pid_iter.bpf.c | 4 ++--
> > 1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/tools/bpf/bpftool/skeleton/pid_iter.bpf.c b/tools/bpf/bpftool/skeleton/pid_iter.bpf.c
> > index d9b420972934..f70702fcb224 100644
> > --- a/tools/bpf/bpftool/skeleton/pid_iter.bpf.c
> > +++ b/tools/bpf/bpftool/skeleton/pid_iter.bpf.c
> > @@ -71,8 +71,8 @@ int iter(struct bpf_iter__task_file *ctx)
> >
> > e.pid = task->tgid;
> > e.id = get_obj_id(file->private_data, obj_type);
> > - bpf_probe_read_kernel(&e.comm, sizeof(e.comm),
> > - task->group_leader->comm);
> > + bpf_probe_read_kernel_str(&e.comm, sizeof(e.comm),
> > + task->group_leader->comm);
> > bpf_seq_write(ctx->meta->seq, &e, sizeof(e));
> >
> > return 0;
> > --
> > 2.17.1
> >
>
> --
> Kees Cook



--
Thanks
Yafang

\
 
 \ /
  Last update: 2021-10-26 04:20    [W:0.090 / U:0.412 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site