Re: [PATCH bpf-next v4 3/3] bpf, selftest/bpf: Fix re-attachment branch in bpf_tracing_prog_attach

[Jiri Olsa <olsajiri@gmail.com>]

On Thu, Nov 30, 2023 at 04:14:55PM +0100, Jiri Olsa wrote:
> On Wed, Nov 29, 2023 at 08:52:38PM +0100, Dmitrii Dolgov wrote:
> > It looks like there is an issue in bpf_tracing_prog_attach, in the
> > "prog->aux->dst_trampoline and tgt_prog is NULL" case. One can construct
> > a sequence of events when prog->aux->attach_btf will be NULL, and
> > bpf_trampoline_compute_key will fail.
> > 
> >     BUG: kernel NULL pointer dereference, address: 0000000000000058
> >     Call Trace:
> >      <TASK>
> >      ? __die+0x20/0x70
> >      ? page_fault_oops+0x15b/0x430
> >      ? fixup_exception+0x22/0x330
> >      ? exc_page_fault+0x6f/0x170
> >      ? asm_exc_page_fault+0x22/0x30
> >      ? bpf_tracing_prog_attach+0x279/0x560
> >      ? btf_obj_id+0x5/0x10
> >      bpf_tracing_prog_attach+0x439/0x560
> >      __sys_bpf+0x1cf4/0x2de0
> >      __x64_sys_bpf+0x1c/0x30
> >      do_syscall_64+0x41/0xf0
> >      entry_SYSCALL_64_after_hwframe+0x6e/0x76
> > 
> > The issue seems to be not relevant to the previous changes with
> > recursive tracing prog attach, because the reproducing test doesn't
> > actually include recursive fentry attaching.
> > 
> > Signed-off-by: Dmitrii Dolgov <9erthalion6@gmail.com>
> > ---
> >  kernel/bpf/syscall.c                          |  4 +-
> >  .../bpf/prog_tests/recursive_attach.c         | 48 +++++++++++++++++++
> >  .../bpf/progs/fentry_recursive_target.c       | 11 +++++
> >  3 files changed, 62 insertions(+), 1 deletion(-)
> > 
> > diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
> > index a595d7a62dbc..e01a949dfed7 100644
> > --- a/kernel/bpf/syscall.c
> > +++ b/kernel/bpf/syscall.c
> > @@ -3197,7 +3197,9 @@ static int bpf_tracing_prog_attach(struct bpf_prog *prog,
> >  			goto out_unlock;
> >  		}
> >  		btf_id = prog->aux->attach_btf_id;
> > -		key = bpf_trampoline_compute_key(NULL, prog->aux->attach_btf, btf_id);
> > +		if (prog->aux->attach_btf)
> > +			key = bpf_trampoline_compute_key(NULL, prog->aux->attach_btf,
> > +											 btf_id);
> >  	}
> 
> nice catch.. I'd think dst_trampoline would caught it, because the
> program is loaded with attach_prog_fd=x and check_attach_btf_id should
> create dst_trampoline.. hum

looks like we don't handle case like this one:

  1) load rawtp program
  2) load fentry program with rawtp as target_fd
  3) create tracing link for fentry program with target_fd = 0
  4) repeat 3

in 3 we will use prog->aux->dst_trampoline and prog->aux->dst_prog
(set from fentry loading) to attach the link, and then set both to NULL

in 4 we have:

  - prog->aux->dst_trampoline == NULL
  - tgt_prog == NULL (because we did not provide target_fd to link_create)
  - prog->aux->attach_btf == NULL (becase program was loaded with attach_prog_fd=X)

AFAICS we can't do anything here, because program was loaded for tgt_prog but we
have no way to find out which one.. so return -EINVAL, like in the patch below

jirka


---
diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
index 5e43ddd1b83f..558ce7bdd781 100644
--- a/kernel/bpf/syscall.c
+++ b/kernel/bpf/syscall.c
@@ -3180,6 +3180,10 @@ static int bpf_tracing_prog_attach(struct bpf_prog *prog,
 	 *
 	 * - if prog->aux->dst_trampoline and tgt_prog is NULL, the program
 	 *   was detached and is going for re-attachment.
+	 *
+	 * - if prog->aux->dst_trampoline is NULL and tgt_prog and prog->aux->attach_btf
+	 *   are NULL, then program was already attached and user did not provide
+	 *   tgt_prog_fd so we have no way to find out or create trampoline
 	 */
 	if (!prog->aux->dst_trampoline && !tgt_prog) {
 		/*
@@ -3193,6 +3197,11 @@ static int bpf_tracing_prog_attach(struct bpf_prog *prog,
 			err = -EINVAL;
 			goto out_unlock;
 		}
+		/* We can allow re-attach only if we have valid attach_btf. */
+		if (!prog->aux->attach_btf) {
+			err = -EINVAL;
+			goto out_unlock;
+		}
 		btf_id = prog->aux->attach_btf_id;
 		key = bpf_trampoline_compute_key(NULL, prog->aux->attach_btf, btf_id);
 	}