On Tue, Oct 26, 2021 at 5:24 AM Kees Cook <keescook@xxxxxxxxxxxx> 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@xxxxxxxxx> > > Cc: Mathieu Desnoyers <mathieu.desnoyers@xxxxxxxxxxxx> > > Cc: Arnaldo Carvalho de Melo <arnaldo.melo@xxxxxxxxx> > > Cc: Andrii Nakryiko <andrii.nakryiko@xxxxxxxxx> > > Cc: Peter Zijlstra <peterz@xxxxxxxxxxxxx> > > Cc: Steven Rostedt <rostedt@xxxxxxxxxxx> > > Cc: Al Viro <viro@xxxxxxxxxxxxxxxxxx> > > Cc: Kees Cook <keescook@xxxxxxxxxxxx> > > Cc: Petr Mladek <pmladek@xxxxxxxx> > > 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
