[bpf] bpf: use attach_btf instead of vmlinux in bpf_sk_storage_tracing_allowed

Commit Message

Jared Kangas Jan. 16, 2025, 4:23 p.m. UTC

When loading BPF programs, bpf_sk_storage_tracing_allowed() assumes that
the type is present in the vmlinux BTF. However, if a non-vmlinux kernel
BTF is attached to, this assumption is invalid and btf_type_by_id()
returns a null pointer. Proof-of-concept on a kernel with CONFIG_IPV6=m:

    $ cat bpfcrash.c
    #include <unistd.h>
    #include <linux/bpf.h>
    #include <sys/syscall.h>

    static int bpf(enum bpf_cmd cmd, union bpf_attr *attr)
    {
        return syscall(__NR_bpf, cmd, attr, sizeof(*attr));
    }

    int main(void)
    {
        const int btf_fd = bpf(BPF_BTF_GET_FD_BY_ID, &(union bpf_attr) {
            .btf_id = BTF_ID,
        });
        if (btf_fd < 0)
            return 1;

        const int bpf_sk_storage_get = 107;
        const struct bpf_insn insns[] = {
            { .code = BPF_JMP | BPF_CALL, .imm = bpf_sk_storage_get},
            { .code = BPF_JMP | BPF_EXIT },
        };
        return bpf(BPF_PROG_LOAD, &(union bpf_attr) {
            .prog_type            = BPF_PROG_TYPE_TRACING,
            .expected_attach_type = BPF_TRACE_FENTRY,
            .license              = (unsigned long)"GPL",
            .insns                = (unsigned long)&insns,
            .insn_cnt             = sizeof(insns) / sizeof(insns[0]),
            .attach_btf_obj_fd    = btf_fd,
            .attach_btf_id        = TYPE_ID,
        });
    }
    $ sudo bpftool btf list | grep ipv6
    2: name [ipv6]  size 928200B
    $ sudo bpftool btf dump id 2 | awk '$3 ~ /inet6_sock_destruct/'
    [130689] FUNC 'inet6_sock_destruct' type_id=130677 linkage=static
    $ gcc -D_DEFAULT_SOURCE -DBTF_ID=2 -DTYPE_ID=130689 \
        bpfcrash.c -o bpfcrash
    $ sudo ./bpfcrash

This results in a null pointer dereference:

    Unable to handle kernel NULL pointer dereference at virtual address 0000000000000000
    Call trace:
     bpf_sk_storage_tracing_allowed+0x8c/0xb0 P
     check_helper_call.isra.0+0xa8/0x1730
     do_check+0xa18/0xb40
     do_check_common+0x140/0x640
     bpf_check+0xb74/0xcb8
     bpf_prog_load+0x598/0x9a8
     __sys_bpf+0x580/0x980
     __arm64_sys_bpf+0x28/0x40
     invoke_syscall.constprop.0+0x54/0xe8
     do_el0_svc+0xb4/0xd0
     el0_svc+0x44/0x1f8
     el0t_64_sync_handler+0x13c/0x160
     el0t_64_sync+0x184/0x188

To get the proper type, use prog->aux->attach_btf instead of assuming
the type is in vmlinux.

Fixes: 8e4597c627fb ("bpf: Allow using bpf_sk_storage in FENTRY/FEXIT/RAW_TP")
Signed-off-by: Jared Kangas <[e-mail]>
---
 net/core/bpf_sk_storage.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

Comments

Martin KaFai Lau Jan. 16, 2025, 8:03 p.m. UTC | #1

On 1/16/25 8:23 AM, Jared Kangas wrote:
> diff --git a/net/core/bpf_sk_storage.c b/net/core/bpf_sk_storage.c
> index 2f4ed83a75ae..74584dd12550 100644
> --- a/net/core/bpf_sk_storage.c
> +++ b/net/core/bpf_sk_storage.c
> @@ -352,8 +352,8 @@ const struct bpf_func_proto bpf_sk_storage_delete_proto = {
>   
>   static bool bpf_sk_storage_tracing_allowed(const struct bpf_prog *prog)
>   {
> -	const struct btf *btf_vmlinux;
>   	const struct btf_type *t;
> +	const struct btf *btf;
>   	const char *tname;
>   	u32 btf_id;
>   
> @@ -371,12 +371,12 @@ static bool bpf_sk_storage_tracing_allowed(const struct bpf_prog *prog)
>   		return true;
>   	case BPF_TRACE_FENTRY:
>   	case BPF_TRACE_FEXIT:
> -		btf_vmlinux = bpf_get_btf_vmlinux();
> -		if (IS_ERR_OR_NULL(btf_vmlinux))
> +		btf = prog->aux->attach_btf;
> +		if (!btf)
>   			return false;
>   		btf_id = prog->aux->attach_btf_id;
> -		t = btf_type_by_id(btf_vmlinux, btf_id);
> -		tname = btf_name_by_offset(btf_vmlinux, t->name_off);
> +		t = btf_type_by_id(btf, btf_id);
> +		tname = btf_name_by_offset(btf, t->name_off);
>   		return !!strncmp(tname, "bpf_sk_storage",
>   				 strlen("bpf_sk_storage"));

Thanks for the report.

There is a prog->aux->attach_func_name, so it can be directly used, like:

	case BPF_TRACE_FENTRY:
	case BPF_TRACE_FEXIT:
		return !!strncmp(prog->aux->attach_func_name, "bpf_sk_storage",
				 strlen("bpf_sk_storage"));

The above should do for the fix.

No need to check for null on attach_func_name. It should have been checked 
earlier in bpf_check_attach_target (the "tname" variable).

pw-bot: cr

Patch

diff --git a/net/core/bpf_sk_storage.c b/net/core/bpf_sk_storage.c
index 2f4ed83a75ae..74584dd12550 100644
--- a/net/core/bpf_sk_storage.c
+++ b/net/core/bpf_sk_storage.c
@@ -352,8 +352,8 @@  const struct bpf_func_proto bpf_sk_storage_delete_proto = {
 
 static bool bpf_sk_storage_tracing_allowed(const struct bpf_prog *prog)
 {
-	const struct btf *btf_vmlinux;
 	const struct btf_type *t;
+	const struct btf *btf;
 	const char *tname;
 	u32 btf_id;
 
@@ -371,12 +371,12 @@  static bool bpf_sk_storage_tracing_allowed(const struct bpf_prog *prog)
 		return true;
 	case BPF_TRACE_FENTRY:
 	case BPF_TRACE_FEXIT:
-		btf_vmlinux = bpf_get_btf_vmlinux();
-		if (IS_ERR_OR_NULL(btf_vmlinux))
+		btf = prog->aux->attach_btf;
+		if (!btf)
 			return false;
 		btf_id = prog->aux->attach_btf_id;
-		t = btf_type_by_id(btf_vmlinux, btf_id);
-		tname = btf_name_by_offset(btf_vmlinux, t->name_off);
+		t = btf_type_by_id(btf, btf_id);
+		tname = btf_name_by_offset(btf, t->name_off);
 		return !!strncmp(tname, "bpf_sk_storage",
 				 strlen("bpf_sk_storage"));
 	default: