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@xxxxxxxxx>
> > ---
> > 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);
}
