Re: [PATCH bpf-next 10/17] bpf: Add support for tracing_multi link session

[XIAO WU <shawdoxwu@gmail.com>]

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 <shawdoxwu@gmail.com>

Thanks