On Tue, Oct 26, 2021 at 9:12 PM Steven Rostedt [off-list ref] wrote:

On Tue, 26 Oct 2021 10:18:51 +0800
Yafang Shao [off-list ref] wrote:

quoted

quoted

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.

quoted

get_task_comm(comm, task->group_leader);

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

quoted

bpf_probe_read_kernel_str(&e.comm, sizeof(e.comm), comm);

I guess Kees is worried that e.comm will have something exported to user
space that it shouldn't. But since e is part of the BPF program, does the
BPF JIT take care to make sure everything on its stack is zero'd out, such
that a user BPF couldn't just read various items off its stack and by doing
so, see kernel memory it shouldn't be seeing?

Understood.
It can leak information to the user if the user buffer is large enough.

I'm guessing it does, otherwise this would be a bigger issue than this
patch series.

I will think about how to fix it.
At first glance, it seems we'd better introduce a new BPF helper like
bpf_probe_read_kernel_str_pad().

-- 
Thanks
Yafang