Hi,
> BPF_TRACE_FSESSION_MULTI is now accepted here, which means
> a program with this type can enter bpf_tracing_prog_attach()
> via BPF_RAW_TRACEPOINT_OPEN:
>
> bpf_raw_tracepoint_open()
> bpf_raw_tp_link_attach() /* name == NULL */
> bpf_tracing_prog_attach() /* BPF_TRACE_FSESSION_MULTI */
>
> Further down in bpf_tracing_prog_attach(), the fexit node
> initialization only checks for BPF_TRACE_FSESSION:
>
> kernel/bpf/syscall.c:bpf_tracing_prog_attach() {
> ...
> if (prog->expected_attach_type == BPF_TRACE_FSESSION) {
> link->fexit.link = &link->link.link;
> link->fexit.cookie = bpf_cookie;
> }
> ...
> }
>
> So for BPF_TRACE_FSESSION_MULTI, link->fexit.link stays NULL
> (from kzalloc). When __bpf_trampoline_link_prog() later calls
> fsession_exit(), it returns &link->fexit with a NULL link
> field. This node gets added to the trampoline FEXIT list, and
> bpf_trampoline_get_progs() then dereferences it:
>
> kernel/bpf/trampoline.c:bpf_trampoline_get_progs() {
> ...
> hlist_for_each_entry(node, &tr->progs_hlist[kind], tramp_hlist) {
> *ip_arg |= node->link->prog->call_get_func_ip;
> ^^^^^^^^^^
> ...
> }
>
> Would it make sense to either add BPF_TRACE_FSESSION_MULTI to
> the fexit initialization, or reject this type in
> bpf_tracing_prog_attach() since it should only be used through
> bpf_tracing_multi_attach()?
Yes, confirmed.
I reproduced this on x86_64 with a minimal tracing program loaded as
BPF_PROG_TYPE_TRACING with
expected_attach_type=BPF_TRACE_FSESSION_MULTI, then attached through
BPF_RAW_TRACEPOINT_OPEN with name=NULL.
This reaches bpf_tracing_prog_attach() without initializing link->fexit
for FSESSION_MULTI and later hits the NULL dereference path in
trampoline handling, as you pointed out.
C reproducer:
--8<--
#define _GNU_SOURCE
#include <errno.h>
#include <stdint.h>
#include <stdio.h>
#include <string.h>
#include <sys/resource.h>
#include <sys/syscall.h>
#include <unistd.h>
/* Use kernel-under-test UAPI, not host's potentially older one. */
#include "../kernel-source/include/uapi/linux/bpf.h"
#ifndef __NR_bpf
#define __NR_bpf 321
#endif
static int sys_bpf(int cmd, union bpf_attr *attr, unsigned int size)
{
return (int)syscall(__NR_bpf, cmd, attr, size);
}
static void bump_memlock(void)
{
struct rlimit r = {RLIM_INFINITY, RLIM_INFINITY};
setrlimit(RLIMIT_MEMLOCK, &r);
}
int main(void)
{
bump_memlock();
/* r0 = 0; exit */
struct bpf_insn prog[] = {
{ .code = 0xb7, .dst_reg = 0, .src_reg = 0, .off = 0, .imm = 0
}, { .code = 0x95, .dst_reg = 0, .src_reg = 0, .off = 0, .imm = 0 },
};
char license[] = "GPL";
static char log_buf[1 << 20];
union bpf_attr attr;
memset(&attr, 0, sizeof(attr));
attr.prog_type = BPF_PROG_TYPE_TRACING;
attr.expected_attach_type = BPF_TRACE_FSESSION_MULTI;
attr.insn_cnt = (uint32_t)(sizeof(prog) / sizeof(prog[0]));
attr.insns = (uint64_t)(uintptr_t)prog;
attr.license = (uint64_t)(uintptr_t)license;
attr.log_buf = (uint64_t)(uintptr_t)log_buf;
attr.log_size = sizeof(log_buf);
attr.log_level = 1;
int prog_fd = sys_bpf(BPF_PROG_LOAD, &attr, sizeof(attr));
if (prog_fd < 0) {
fprintf(stderr, "BPF_PROG_LOAD failed: errno=%d (%s)\n", errno,
strerror(errno)); if (log_buf[0])
fprintf(stderr, "verifier log:\n%s\n", log_buf);
return 1;
}
memset(&attr, 0, sizeof(attr));
attr.raw_tracepoint.prog_fd = prog_fd;
attr.raw_tracepoint.name = 0; /* NULL name drives TRACING attach
path */ attr.raw_tracepoint.cookie = 0x4141414142424242ULL;
int link_fd = sys_bpf(BPF_RAW_TRACEPOINT_OPEN, &attr, sizeof(attr));
if (link_fd < 0) {
fprintf(stderr, "BPF_RAW_TRACEPOINT_OPEN returned errno=%d
(%s)\n", errno, strerror(errno)); close(prog_fd);
return 2;
}
fprintf(stderr, "Unexpectedly succeeded: link_fd=%d\n", link_fd);
close(link_fd);
close(prog_fd);
return 0;
}
--8<--
I agree the patch should be made bisect-safe. I will post a follow-up
that ensures BPF_TRACE_FSESSION_MULTI cannot enter this uninitialized
fexit path (either by initializing it consistently where needed, or
rejecting this attach route and keeping it exclusive to
bpf_tracing_multi_attach()).
Signed-off-by: XIAO WU <[email protected]>
Thanks
