[bpf-next,v2,1/5] bpf: Support __nullable argument suffix for tp_btf

Commit Message

Philo Lu Sept. 5, 2024, 7:56 a.m. UTC

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 <[e-mail]>
Signed-off-by: Philo Lu <[e-mail]>
---
 kernel/bpf/btf.c      | 13 +++++++++++++
 kernel/bpf/verifier.c | 36 +++++++++++++++++++++++++++++++++---
 2 files changed, 46 insertions(+), 3 deletions(-)

Comments

Andrii Nakryiko Sept. 6, 2024, 9:25 p.m. UTC | #1

On Thu, Sep 5, 2024 at 12:56 AM Philo Lu <[e-mail]> 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 <[e-mail]>
> Signed-off-by: Philo Lu <[e-mail]>
> ---
>  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
>

Andrii Nakryiko Sept. 7, 2024, 1:14 a.m. UTC | #2

On Thu, Sep 5, 2024 at 12:56 AM Philo Lu <[e-mail]> 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.

BTW, just curious, is it a pure coincidence that I solved the same
problem in retsnoop with the same approach (see extensive comment in
[0]) about 2 weeks ago, or retsnoop's approach was an inspiration
here?

  [0] https://github.com/anakryiko/retsnoop/commit/7b253fc55b51d447e5ea91d99f60d9c34934f799

>
> Suggested-by: Alexei Starovoitov <[e-mail]>
> Signed-off-by: Philo Lu <[e-mail]>
> ---
>  kernel/bpf/btf.c      | 13 +++++++++++++
>  kernel/bpf/verifier.c | 36 +++++++++++++++++++++++++++++++++---
>  2 files changed, 46 insertions(+), 3 deletions(-)
>

[...]

Philo Lu Sept. 7, 2024, 3:26 a.m. UTC | #3

On 2024/9/7 05:25, Andrii Nakryiko wrote:
> On Thu, Sep 5, 2024 at 12:56 AM Philo Lu <[e-mail]> 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 <[e-mail]>
>> Signed-off-by: Philo Lu <[e-mail]>
>> ---
>>   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?..
> 

Yes. I don't think the check is necessary but I'm not quite sure if it 
affects other prog/attach types. It's ok for me to remove the check. I 
added it just because this __nullable suffix only serves tp_btf now.

And thanks for your nice suggestions. I'll fix them in the next version.

Also, thanks for the information about retsnoop. Our solutions seem to 
be similar, and I'll look into it further. Honestly, it takes me some 
time to get argument names from tp_btf, and I cannot find a better 
solution but to use btp->bpf_func with kallsyms...

Patch

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");
+}
+
 int btf_ctx_arg_offset(const struct btf *btf, const struct btf_type *func_proto,
 		       u32 arg_no)
 {
@@ -6554,6 +6564,9 @@  bool btf_ctx_access(int off, int size, enum bpf_access_type type,
 	if (prog_args_trusted(prog))
 		info->reg_type |= PTR_TRUSTED;
 
+	if (prog_arg_maybe_null(prog, btf, &args[arg]))
+		info->reg_type |= PTR_MAYBE_NULL;
+
 	if (tgt_prog) {
 		enum bpf_prog_type tgt_type;
 
diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index 217eb0eafa2a6..9ee68ed915dfe 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -28,6 +28,7 @@ 
 #include <linux/cpumask.h>
 #include <linux/bpf_mem_alloc.h>
 #include <net/xdp.h>
+#include <linux/trace_events.h>
 
 #include "disasm.h"
 
@@ -21780,6 +21781,8 @@  static int check_non_sleepable_error_inject(u32 btf_id)
 	return btf_id_set_contains(&btf_non_sleepable_error_inject, btf_id);
 }
 
+#define BTF_MAX_NAME_SIZE 128
+
 int bpf_check_attach_target(struct bpf_verifier_log *log,
 			    const struct bpf_prog *prog,
 			    const struct bpf_prog *tgt_prog,
@@ -21788,6 +21791,8 @@  int bpf_check_attach_target(struct bpf_verifier_log *log,
 {
 	bool prog_extension = prog->type == BPF_PROG_TYPE_EXT;
 	bool prog_tracing = prog->type == BPF_PROG_TYPE_TRACING;
+	char trace_symbol[BTF_MAX_NAME_SIZE];
+	const struct bpf_raw_event_map *btp;
 	const char prefix[] = "btf_trace_";
 	int ret = 0, subprog = -1, i;
 	const struct btf_type *t;
@@ -21795,6 +21800,7 @@  int bpf_check_attach_target(struct bpf_verifier_log *log,
 	const char *tname;
 	struct btf *btf;
 	long addr = 0;
+	char *fname;
 	struct module *mod = NULL;
 
 	if (!btf_id) {
@@ -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;
+		sprintf(trace_symbol, "%ps", btp->bpf_func);
+		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 */