linux-kernel - Re: NULL pointer deref when running BPF monitor program (6.11.0-rc1)

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAADnVQ+NpPtFOrvD0o2F8npCpZwPrLf4dX8h8Rt96uwM+crQcQ@mail.gmail.com>
Date: Tue, 6 Aug 2024 11:44:52 -0700
From: Alexei Starovoitov <alexei.starovoitov@...il.com>
To: Jiri Olsa <olsajiri@...il.com>
Cc: Juri Lelli <juri.lelli@...hat.com>, bpf <bpf@...r.kernel.org>, 
	LKML <linux-kernel@...r.kernel.org>, Artem Savkov <asavkov@...hat.com>
Subject: Re: NULL pointer deref when running BPF monitor program (6.11.0-rc1)

On Tue, Aug 6, 2024 at 6:24 AM Jiri Olsa <olsajiri@...il.com> wrote:
>
> > Jiri,
> >
> > the verifier removes the check because it assumes that pointers
> > passed by the kernel into tracepoint are valid and trusted.
> > In this case:
> >         trace_sched_pi_setprio(p, pi_task);
> >
> > pi_task can be NULL.
> >
> > We cannot make all tracepoint pointers to be PTR_TRUSTED | PTR_MAYBE_NULL
> > by default, since it will break a bunch of progs.
> > Instead we can annotate this tracepoint arg as __nullable and
> > teach the verifier to recognize such special arguments of tracepoints.
>
> ok, so you mean to be able to mark it in event header like:
>
>   TRACE_EVENT(sched_pi_setprio,
>         TP_PROTO(struct task_struct *tsk, struct task_struct *pi_task __nullable),
>
> I guess we could make pahole to emit DECL_TAG for that argument,
> but I'm not sure how to propagate that __nullable info to pahole
>
> while wondering about that, I tried the direct fix below ;-)

We don't need to rush such a hack below.
No need to add decl_tag and change pahole either.
The arg name is already vmlinux BTF:
[51371] FUNC_PROTO '(anon)' ret_type_id=0 vlen=3
        '__data' type_id=61
        'tsk' type_id=77
        'pi_task' type_id=77
[51372] FUNC '__bpf_trace_sched_pi_setprio' type_id=51371 linkage=static

just need to rename "pi_task" to "pi_task__nullable"
and teach the verifier.


> jirka
>
>
> ---
> diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
> index 95426d5b634e..1a20bbdead64 100644
> --- a/kernel/bpf/btf.c
> +++ b/kernel/bpf/btf.c
> @@ -6377,6 +6377,25 @@ int btf_ctx_arg_offset(const struct btf *btf, const struct btf_type *func_proto,
>         return off;
>  }
>
> +static bool is_tracing_prog_raw_tp(const struct bpf_prog *prog, const char *name)
> +{
> +       struct btf *btf = prog->aux->attach_btf;
> +       const struct btf_type *t;
> +       const char *tname;
> +
> +       if (prog->expected_attach_type != BPF_TRACE_RAW_TP)
> +               return false;
> +
> +       t = btf_type_by_id(btf, prog->aux->attach_btf_id);
> +       if (!t)
> +               return false;
> +
> +       tname = btf_name_by_offset(btf, t->name_off);
> +       if (!tname)
> +               return false;
> +       return !strcmp(tname, name);
> +}
> +
>  bool btf_ctx_access(int off, int size, enum bpf_access_type type,
>                     const struct bpf_prog *prog,
>                     struct bpf_insn_access_aux *info)
> @@ -6544,6 +6563,10 @@ bool btf_ctx_access(int off, int size, enum bpf_access_type type,
>                 }
>         }
>
> +       /* Second argument of sched_pi_setprio tracepoint can be null */
> +       if (is_tracing_prog_raw_tp(prog, "btf_trace_sched_pi_setprio") && arg == 1)
> +               info->reg_type |= PTR_MAYBE_NULL;
> +
>         info->btf = btf;
>         info->btf_id = t->type;
>         t = btf_type_by_id(btf, t->type);