diff mbox series

[v6,08/12] tools/bpf/bpftool/skeleton: make it adopt to task comm size change

Message ID 20211025083315.4752-9-laoar.shao@gmail.com (mailing list archive)
State Superseded
Delegated to: BPF
Headers show
Series extend task comm from 16 to 24 | expand

Checks

Context Check Description
netdev/tree_selection success Not a local patch
bpf/vmtest-bpf-next success VM_Test
bpf/vmtest-bpf-next-PR success PR summary
bpf/vmtest-bpf success VM_Test
bpf/vmtest-bpf-PR success PR summary

Commit Message

Yafang Shao Oct. 25, 2021, 8:33 a.m. UTC
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>
---
 tools/bpf/bpftool/skeleton/pid_iter.bpf.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Comments

Kees Cook Oct. 25, 2021, 9:24 p.m. UTC | #1
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?

get_task_comm(comm, task->group_leader);
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
>
Yafang Shao Oct. 26, 2021, 2:18 a.m. UTC | #2
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
Steven Rostedt Oct. 26, 2021, 1:12 p.m. UTC | #3
On Tue, 26 Oct 2021 10:18:51 +0800
Yafang Shao <laoar.shao@gmail.com> wrote:

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

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?

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

-- Steve
Yafang Shao Oct. 26, 2021, 1:55 p.m. UTC | #4
On Tue, Oct 26, 2021 at 9:12 PM Steven Rostedt <rostedt@goodmis.org> wrote:
>
> On Tue, 26 Oct 2021 10:18:51 +0800
> Yafang Shao <laoar.shao@gmail.com> wrote:
>
> > > 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);
>
> 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().
Yafang Shao Oct. 26, 2021, 2:02 p.m. UTC | #5
On Tue, Oct 26, 2021 at 9:55 PM Yafang Shao <laoar.shao@gmail.com> wrote:
>
> On Tue, Oct 26, 2021 at 9:12 PM Steven Rostedt <rostedt@goodmis.org> wrote:
> >
> > On Tue, 26 Oct 2021 10:18:51 +0800
> > Yafang Shao <laoar.shao@gmail.com> wrote:
> >
> > > > 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);
> >
> > 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?
> >
>

Ah, you mean the BPF JIT has already avoided leaking information to user.
I will check the BPF JIT code first.

> 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
Yafang Shao Oct. 26, 2021, 4:09 p.m. UTC | #6
On Tue, Oct 26, 2021 at 9:12 PM Steven Rostedt <rostedt@goodmis.org> wrote:
>
> On Tue, 26 Oct 2021 10:18:51 +0800
> Yafang Shao <laoar.shao@gmail.com> wrote:
>
> > > 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);
>
> 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?
>
> I'm guessing it does, otherwise this would be a bigger issue than this
> patch series.
>

You guess is correct per my verification.
But the BPF JIT doesn't  zero it out, while it really does is adding
some character like '?' in my verification.

Anyway we don't need to worry that the kernel information may be
leaked though bpf_probe_read_kernel_str().
diff mbox series

Patch

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;