| Message ID | 20230515130849.57502-2-laoar.shao@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | BPF |
| Headers | show |
| Series | bpf: bpf trampoline improvements | expand |
On Mon, May 15, 2023 at 6:09 AM Yafang Shao <laoar.shao@gmail.com> wrote: > > If it fails to attach fentry, the allocated bpf trampoline image will be > left in the system. That can be verified by checking /proc/kallsyms. > > This meamleak can be verified by a simple bpf program as follows, > > SEC("fentry/trap_init") > int fentry_run() > { > return 0; > } > > It will fail to attach trap_init because this function is freed after > kernel init, and then we can find the trampoline image is left in the > system by checking /proc/kallsyms. > $ tail /proc/kallsyms > ffffffffc0613000 t bpf_trampoline_6442453466_1 [bpf] > ffffffffc06c3000 t bpf_trampoline_6442453466_1 [bpf] > > $ bpftool btf dump file /sys/kernel/btf/vmlinux | grep "FUNC 'trap_init'" > [2522] FUNC 'trap_init' type_id=119 linkage=static > > $ echo $((6442453466 & 0x7fffffff)) > 2522 > > Note that there are two left bpf trampoline images, that is because the > libbpf will fallback to raw tracepoint if -EINVAL is returned. > > Fixes: e21aa341785c ("bpf: Fix fexit trampoline.") > Signed-off-by: Yafang Shao <laoar.shao@gmail.com> > Cc: Song Liu <song@kernel.org> > Cc: Jiri Olsa <olsajiri@gmail.com> Acked-by: Song Liu <song@kernel.org>
On 5/15/23 5:52 PM, Song Liu wrote: > On Mon, May 15, 2023 at 6:09 AM Yafang Shao <laoar.shao@gmail.com> wrote: >> >> If it fails to attach fentry, the allocated bpf trampoline image will be >> left in the system. That can be verified by checking /proc/kallsyms. >> >> This meamleak can be verified by a simple bpf program as follows, >> >> SEC("fentry/trap_init") >> int fentry_run() >> { >> return 0; >> } >> >> It will fail to attach trap_init because this function is freed after >> kernel init, and then we can find the trampoline image is left in the >> system by checking /proc/kallsyms. >> $ tail /proc/kallsyms >> ffffffffc0613000 t bpf_trampoline_6442453466_1 [bpf] >> ffffffffc06c3000 t bpf_trampoline_6442453466_1 [bpf] >> >> $ bpftool btf dump file /sys/kernel/btf/vmlinux | grep "FUNC 'trap_init'" >> [2522] FUNC 'trap_init' type_id=119 linkage=static >> >> $ echo $((6442453466 & 0x7fffffff)) >> 2522 >> >> Note that there are two left bpf trampoline images, that is because the >> libbpf will fallback to raw tracepoint if -EINVAL is returned. >> >> Fixes: e21aa341785c ("bpf: Fix fexit trampoline.") >> Signed-off-by: Yafang Shao <laoar.shao@gmail.com> >> Cc: Song Liu <song@kernel.org> >> Cc: Jiri Olsa <olsajiri@gmail.com> > > Acked-by: Song Liu <song@kernel.org> Won't this trigger a UAF for the case when progs are already running at this address aka modify_fentry() fails where you would then also hit the goto out_free path? This looks not correct to me. Thanks, Daniel
> On May 15, 2023, at 1:17 PM, Daniel Borkmann <daniel@iogearbox.net> wrote: > > On 5/15/23 5:52 PM, Song Liu wrote: >> On Mon, May 15, 2023 at 6:09 AM Yafang Shao <laoar.shao@gmail.com> wrote: >>> >>> If it fails to attach fentry, the allocated bpf trampoline image will be >>> left in the system. That can be verified by checking /proc/kallsyms. >>> >>> This meamleak can be verified by a simple bpf program as follows, >>> >>> SEC("fentry/trap_init") >>> int fentry_run() >>> { >>> return 0; >>> } >>> >>> It will fail to attach trap_init because this function is freed after >>> kernel init, and then we can find the trampoline image is left in the >>> system by checking /proc/kallsyms. >>> $ tail /proc/kallsyms >>> ffffffffc0613000 t bpf_trampoline_6442453466_1 [bpf] >>> ffffffffc06c3000 t bpf_trampoline_6442453466_1 [bpf] >>> >>> $ bpftool btf dump file /sys/kernel/btf/vmlinux | grep "FUNC 'trap_init'" >>> [2522] FUNC 'trap_init' type_id=119 linkage=static >>> >>> $ echo $((6442453466 & 0x7fffffff)) >>> 2522 >>> >>> Note that there are two left bpf trampoline images, that is because the >>> libbpf will fallback to raw tracepoint if -EINVAL is returned. >>> >>> Fixes: e21aa341785c ("bpf: Fix fexit trampoline.") >>> Signed-off-by: Yafang Shao <laoar.shao@gmail.com> >>> Cc: Song Liu <song@kernel.org> >>> Cc: Jiri Olsa <olsajiri@gmail.com> >> Acked-by: Song Liu <song@kernel.org> > > Won't this trigger a UAF for the case when progs are already running at > this address aka modify_fentry() fails where you would then also hit the > goto out_free path? This looks not correct to me. I am not following. If modify_fentry() fails, we will not use the new image anywhere, no? Did I miss something? Thanks, Song
On 5/15/23 11:14 PM, Song Liu wrote: >> On May 15, 2023, at 1:17 PM, Daniel Borkmann <daniel@iogearbox.net> wrote: >> On 5/15/23 5:52 PM, Song Liu wrote: >>> On Mon, May 15, 2023 at 6:09 AM Yafang Shao <laoar.shao@gmail.com> wrote: >>>> >>>> If it fails to attach fentry, the allocated bpf trampoline image will be >>>> left in the system. That can be verified by checking /proc/kallsyms. >>>> >>>> This meamleak can be verified by a simple bpf program as follows, >>>> >>>> SEC("fentry/trap_init") >>>> int fentry_run() >>>> { >>>> return 0; >>>> } >>>> >>>> It will fail to attach trap_init because this function is freed after >>>> kernel init, and then we can find the trampoline image is left in the >>>> system by checking /proc/kallsyms. >>>> $ tail /proc/kallsyms >>>> ffffffffc0613000 t bpf_trampoline_6442453466_1 [bpf] >>>> ffffffffc06c3000 t bpf_trampoline_6442453466_1 [bpf] >>>> >>>> $ bpftool btf dump file /sys/kernel/btf/vmlinux | grep "FUNC 'trap_init'" >>>> [2522] FUNC 'trap_init' type_id=119 linkage=static >>>> >>>> $ echo $((6442453466 & 0x7fffffff)) >>>> 2522 >>>> >>>> Note that there are two left bpf trampoline images, that is because the >>>> libbpf will fallback to raw tracepoint if -EINVAL is returned. >>>> >>>> Fixes: e21aa341785c ("bpf: Fix fexit trampoline.") >>>> Signed-off-by: Yafang Shao <laoar.shao@gmail.com> >>>> Cc: Song Liu <song@kernel.org> >>>> Cc: Jiri Olsa <olsajiri@gmail.com> >>> Acked-by: Song Liu <song@kernel.org> >> >> Won't this trigger a UAF for the case when progs are already running at >> this address aka modify_fentry() fails where you would then also hit the >> goto out_free path? This looks not correct to me. > > I am not following. If modify_fentry() fails, we will not use the new > image anywhere, no? Did I miss something? Hm, agree, I think I got confused with the again label earlier. Looks ok indeed. Applied, thanks!
diff --git a/kernel/bpf/trampoline.c b/kernel/bpf/trampoline.c index ac021bc..2a3849c 100644 --- a/kernel/bpf/trampoline.c +++ b/kernel/bpf/trampoline.c @@ -251,11 +251,8 @@ static int register_fentry(struct bpf_trampoline *tr, void *new_addr) return tlinks; } -static void __bpf_tramp_image_put_deferred(struct work_struct *work) +static void bpf_tramp_image_free(struct bpf_tramp_image *im) { - struct bpf_tramp_image *im; - - im = container_of(work, struct bpf_tramp_image, work); bpf_image_ksym_del(&im->ksym); bpf_jit_free_exec(im->image); bpf_jit_uncharge_modmem(PAGE_SIZE); @@ -263,6 +260,14 @@ static void __bpf_tramp_image_put_deferred(struct work_struct *work) kfree_rcu(im, rcu); } +static void __bpf_tramp_image_put_deferred(struct work_struct *work) +{ + struct bpf_tramp_image *im; + + im = container_of(work, struct bpf_tramp_image, work); + bpf_tramp_image_free(im); +} + /* callback, fexit step 3 or fentry step 2 */ static void __bpf_tramp_image_put_rcu(struct rcu_head *rcu) { @@ -438,7 +443,7 @@ static int bpf_trampoline_update(struct bpf_trampoline *tr, bool lock_direct_mut &tr->func.model, tr->flags, tlinks, tr->func.addr); if (err < 0) - goto out; + goto out_free; set_memory_rox((long)im->image, 1); @@ -468,7 +473,7 @@ static int bpf_trampoline_update(struct bpf_trampoline *tr, bool lock_direct_mut } #endif if (err) - goto out; + goto out_free; if (tr->cur_image) bpf_tramp_image_put(tr->cur_image); @@ -480,6 +485,10 @@ static int bpf_trampoline_update(struct bpf_trampoline *tr, bool lock_direct_mut tr->flags = orig_flags; kfree(tlinks); return err; + +out_free: + bpf_tramp_image_free(im); + goto out; } static enum bpf_tramp_prog_type bpf_attach_type_to_tramp(struct bpf_prog *prog)
If it fails to attach fentry, the allocated bpf trampoline image will be left in the system. That can be verified by checking /proc/kallsyms. This meamleak can be verified by a simple bpf program as follows, SEC("fentry/trap_init") int fentry_run() { return 0; } It will fail to attach trap_init because this function is freed after kernel init, and then we can find the trampoline image is left in the system by checking /proc/kallsyms. $ tail /proc/kallsyms ffffffffc0613000 t bpf_trampoline_6442453466_1 [bpf] ffffffffc06c3000 t bpf_trampoline_6442453466_1 [bpf] $ bpftool btf dump file /sys/kernel/btf/vmlinux | grep "FUNC 'trap_init'" [2522] FUNC 'trap_init' type_id=119 linkage=static $ echo $((6442453466 & 0x7fffffff)) 2522 Note that there are two left bpf trampoline images, that is because the libbpf will fallback to raw tracepoint if -EINVAL is returned. Fixes: e21aa341785c ("bpf: Fix fexit trampoline.") Signed-off-by: Yafang Shao <laoar.shao@gmail.com> Cc: Song Liu <song@kernel.org> Cc: Jiri Olsa <olsajiri@gmail.com> --- kernel/bpf/trampoline.c | 21 +++++++++++++++------ 1 file changed, 15 insertions(+), 6 deletions(-)