netdev - Re: [PATCH bpf-next v2 1/5] bpf: Support __nullable argument suffix for tp

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: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <CAEf4BzYjGaJGzw+dXCOhUwJS-QhyZ-_sWL6Oo8yUXOoeWWA1=w@mail.gmail.com>
Date: Fri, 6 Sep 2024 14:25:00 -0700
From: Andrii Nakryiko <andrii.nakryiko@...il.com>
To: Philo Lu <lulie@...ux.alibaba.com>
Cc: bpf@...r.kernel.org, edumazet@...gle.com, rostedt@...dmis.org, 
	mhiramat@...nel.org, mathieu.desnoyers@...icios.com, martin.lau@...ux.dev, 
	ast@...nel.org, daniel@...earbox.net, andrii@...nel.org, eddyz87@...il.com, 
	song@...nel.org, yonghong.song@...ux.dev, john.fastabend@...il.com, 
	kpsingh@...nel.org, sdf@...ichev.me, haoluo@...gle.com, jolsa@...nel.org, 
	davem@...emloft.net, kuba@...nel.org, pabeni@...hat.com, mykolal@...com, 
	shuah@...nel.org, mcoquelin.stm32@...il.com, alexandre.torgue@...s.st.com, 
	thinker.li@...il.com, juntong.deng@...look.com, jrife@...gle.com, 
	alan.maguire@...cle.com, davemarchevsky@...com, dxu@...uu.xyz, 
	vmalik@...hat.com, cupertino.miranda@...cle.com, mattbobrowski@...gle.com, 
	xuanzhuo@...ux.alibaba.com, netdev@...r.kernel.org, 
	linux-trace-kernel@...r.kernel.org
Subject: Re: [PATCH bpf-next v2 1/5] bpf: Support __nullable argument suffix
 for tp_btf

On Thu, Sep 5, 2024 at 12:56 AM Philo Lu <lulie@...ux.alibaba.com> wrote:
>
> Pointers passed to tp_btf were trusted to be valid, but some tracepoints
> do take NULL pointer as input, such as trace_tcp_send_reset(). Then the
> invalid memory access cannot be detected by verifier.
>
> This patch fix it by add a suffix "__nullable" to the unreliable
> argument. The suffix is shown in btf, and PTR_MAYBE_NULL will be added
> to nullable arguments. Then users must check the pointer before use it.
>
> A problem here is that we use "btf_trace_##call" to search func_proto.
> As it is a typedef, argument names as well as the suffix are not
> recorded. To solve this, I use bpf_raw_event_map to find
> "__bpf_trace##template" from "btf_trace_##call", and then we can see the
> suffix.
>
> Suggested-by: Alexei Starovoitov <ast@...nel.org>
> Signed-off-by: Philo Lu <lulie@...ux.alibaba.com>
> ---
>  kernel/bpf/btf.c      | 13 +++++++++++++
>  kernel/bpf/verifier.c | 36 +++++++++++++++++++++++++++++++++---
>  2 files changed, 46 insertions(+), 3 deletions(-)
>
> diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
> index 1e29281653c62..157f5e1247c81 100644
> --- a/kernel/bpf/btf.c
> +++ b/kernel/bpf/btf.c
> @@ -6385,6 +6385,16 @@ static bool prog_args_trusted(const struct bpf_prog *prog)
>         }
>  }
>
> +static bool prog_arg_maybe_null(const struct bpf_prog *prog, const struct btf *btf,
> +                               const struct btf_param *arg)
> +{
> +       if (prog->type != BPF_PROG_TYPE_TRACING ||
> +           prog->expected_attach_type != BPF_TRACE_RAW_TP)
> +               return false;
> +
> +       return btf_param_match_suffix(btf, arg, "__nullable");

why does this need to be BPF_TRACE_RAW_TP-specific logic? Are we
afraid that there might be "some_arg__nullable" argument name?..

> +}
> +
>  int btf_ctx_arg_offset(const struct btf *btf, const struct btf_type *func_proto,
>                        u32 arg_no)
>  {

[...]

> @@ -21923,10 +21929,34 @@ int bpf_check_attach_target(struct bpf_verifier_log *log,
>                         return -EINVAL;
>                 }
>                 tname += sizeof(prefix) - 1;
> -               t = btf_type_by_id(btf, t->type);
> -               if (!btf_type_is_ptr(t))
> -                       /* should never happen in valid vmlinux build */
> +
> +               /* The func_proto of "btf_trace_##tname" is generated from typedef without argument
> +                * names. Thus using bpf_raw_event_map to get argument names. For module, the module
> +                * name is printed in "%ps" after the template function name, so use strsep to cut
> +                * it off.
> +                */
> +               btp = bpf_get_raw_tracepoint(tname);
> +               if (!btp)
>                         return -EINVAL;

there has to be bpf_put_raw_tracepoint() somewhere below

pw-bot: cr

> +               sprintf(trace_symbol, "%ps", btp->bpf_func);

snprintf, but this is really a very round-about way of doing kallsym
lookup, no? Why not call kallsyms_lookup() directly?


> +               fname = trace_symbol;
> +               fname = strsep(&fname, " ");
> +
> +               ret = btf_find_by_name_kind(btf, fname, BTF_KIND_FUNC);
> +               if (ret < 0) {
> +                       bpf_log(log, "Cannot find btf of template %s, fall back to %s%s.\n",
> +                               fname, prefix, tname);
> +                       t = btf_type_by_id(btf, t->type);
> +                       if (!btf_type_is_ptr(t))
> +                               /* should never happen in valid vmlinux build */
> +                               return -EINVAL;
> +               } else {
> +                       t = btf_type_by_id(btf, ret);
> +                       if (!btf_type_is_func(t))
> +                               /* should never happen in valid vmlinux build */
> +                               return -EINVAL;
> +               }
> +
>                 t = btf_type_by_id(btf, t->type);
>                 if (!btf_type_is_func_proto(t))
>                         /* should never happen in valid vmlinux build */
> --
> 2.32.0.3.g01195cf9f
>