On Tue, Oct 26, 2021 at 5:24 AM Kees Cook [off-list ref] wrote:

On Mon, Oct 25, 2021 at 08:33:11AM +0000, Yafang Shao wrote:

quoted

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 <redacted>
Cc: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
Cc: Arnaldo Carvalho de Melo <redacted>
Cc: Andrii Nakryiko <redacted>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Steven Rostedt <rostedt@goodmis.org>
Cc: Al Viro <viro@zeniv.linux.org.uk>
Cc: Kees Cook <redacted>
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

quoted

---
 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