bpf: Add deny list of btf ids check for tracing programs

PATCH In review Jiri Olsa 3 participants 4 messages Apr 29, 2021 – May 11, 2021

Subsystems (4)

Networking Network drivers BPF BPF core

lore

Share & tools

lore

Jiri Olsa

author reviewer

2021-04-29 11:47 UTC #0 lore raw

PATCH v2 1/1 bpf: Add deny list of btf ids check for tracing programs

The recursion check in __bpf_prog_enter and __bpf_prog_exit leaves some (not inlined) functions unprotected:

In __bpf_prog_enter:
- migrate_disable is called before prog->active is checked

In __bpf_prog_exit:
- migrate_enable,rcu_read_unlock_strict are called after

    prog->active is decreased

When attaching trampoline to them we get panic like:

traps: PANIC: double fault, error_code: 0x0 double fault: 0000 [#1] SMP PTI

  RIP: 0010:__bpf_prog_enter+0x4/0x50
  ...
  Call Trace:
   <IRQ>
   bpf_trampoline_6442466513_0+0x18/0x1000
   migrate_disable+0x5/0x50
   __bpf_prog_enter+0x9/0x50
   bpf_trampoline_6442466513_0+0x18/0x1000
   migrate_disable+0x5/0x50
   __bpf_prog_enter+0x9/0x50
   bpf_trampoline_6442466513_0+0x18/0x1000
   migrate_disable+0x5/0x50
   __bpf_prog_enter+0x9/0x50
   bpf_trampoline_6442466513_0+0x18/0x1000
   migrate_disable+0x5/0x50
   ...

Fixing this by adding deny list of btf ids for tracing programs and checking btf id during program verification. Adding above functions to this list.

Signed-off-by Jiri Olsa <[email protected]>

---
v2 changes:
- drop check for EXT programs [Andrii]

 kernel/bpf/verifier.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

kernel/bpf/verifier.c +14 -0

index 2579f6fbb5c3..42311e51ac71 100644
@@ -13112,6 +13112,17 @@ int bpf_check_attach_target(struct bpf_verifier_log *log,
 	return 0;
 }
 
+BTF_SET_START(btf_id_deny)
+BTF_ID_UNUSED
+#ifdef CONFIG_SMP
+BTF_ID(func, migrate_disable)
+BTF_ID(func, migrate_enable)
+#endif
+#if !defined CONFIG_PREEMPT_RCU && !defined CONFIG_TINY_RCU
+BTF_ID(func, rcu_read_unlock_strict)
+#endif
+BTF_SET_END(btf_id_deny)
+
 static int check_attach_btf_id(struct bpf_verifier_env *env)
 {
 	struct bpf_prog *prog = env->prog;

@@ -13171,6 +13182,9 @@ static int check_attach_btf_id(struct bpf_verifier_env *env)
 		ret = bpf_lsm_verify_prog(&env->log, prog);
 		if (ret < 0)
 			return ret;
+	} else if (prog->type == BPF_PROG_TYPE_TRACING &&
+		   btf_id_set_contains(&btf_id_deny, btf_id)) {
+		return -EINVAL;
 	}
 
 	key = bpf_trampoline_compute_key(tgt_prog, prog->aux->attach_btf, btf_id);

Signature · 1 line

2.30.2

Alexei Starovoitov

2021-05-07 01:36 UTC #1 lore raw

Quoted 53 lines · Jiri Olsa

The recursion check in __bpf_prog_enter and __bpf_prog_exit leaves some (not inlined) functions unprotected:
In __bpf_prog_enter:
- migrate_disable is called before prog->active is checked
In __bpf_prog_exit:
- migrate_enable,rcu_read_unlock_strict are called after
    prog->active is decreased
When attaching trampoline to them we get panic like:
traps: PANIC: double fault, error_code: 0x0 double fault: 0000 [#1] SMP PTI
  RIP: 0010:__bpf_prog_enter+0x4/0x50
  ...
  Call Trace:
   <IRQ>
   bpf_trampoline_6442466513_0+0x18/0x1000
   migrate_disable+0x5/0x50
   __bpf_prog_enter+0x9/0x50
   bpf_trampoline_6442466513_0+0x18/0x1000
   migrate_disable+0x5/0x50
   __bpf_prog_enter+0x9/0x50
   bpf_trampoline_6442466513_0+0x18/0x1000
   migrate_disable+0x5/0x50
   __bpf_prog_enter+0x9/0x50
   bpf_trampoline_6442466513_0+0x18/0x1000
   migrate_disable+0x5/0x50
   ...
Fixing this by adding deny list of btf ids for tracing programs and checking btf id during program verification. Adding above functions to this list.
Suggested-by Alexei Starovoitov <[email protected]>
Signed-off-by Jiri Olsa <[email protected]>
---
v2 changes:
- drop check for EXT programs [Andrii]
 kernel/bpf/verifier.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)
kernel/bpf/verifier.c +0 -0
index 2579f6fbb5c3..42311e51ac71 100644
@@ -13112,6 +13112,17 @@ int bpf_check_attach_target(struct bpf_verifier_log *log,
        return 0;
 }
 

kernel/bpf/verifier.c int bpf_check_attach_target(struct bpf_verifier_log *log, · line 13124

Quoted 10 lines

kernel/bpf/verifier.c +10 -0

+BTF_SET_START(btf_id_deny)
+BTF_ID_UNUSED
+#ifdef CONFIG_SMP
+BTF_ID(func, migrate_disable)
+BTF_ID(func, migrate_enable)
+#endif
+#if !defined CONFIG_PREEMPT_RCU && !defined CONFIG_TINY_RCU
+BTF_ID(func, rcu_read_unlock_strict)
+#endif
+BTF_SET_END(btf_id_deny)

I was wondering whether it makes sense to do this on pahole side instead ? It can do more flexible regex matching and excluding all such functions from vmlinux btf without the kernel having to do a maze of #ifdef depending on config.
On one side we will lose BTF info about such functions, but what do we need it for?
On the other side it will be a tiny reduction in vmlinux btf :) Thoughts?

Jiri Olsa

2021-05-10 09:50 UTC #2 lore raw

Quoted 65 lines · Alexei Starovoitov

Quoted 53 lines · Jiri Olsa
The recursion check in __bpf_prog_enter and __bpf_prog_exit leaves some (not inlined) functions unprotected:
In __bpf_prog_enter:
- migrate_disable is called before prog->active is checked
In __bpf_prog_exit:
- migrate_enable,rcu_read_unlock_strict are called after
    prog->active is decreased
When attaching trampoline to them we get panic like:
traps: PANIC: double fault, error_code: 0x0 double fault: 0000 [#1] SMP PTI
  RIP: 0010:__bpf_prog_enter+0x4/0x50
  ...
  Call Trace:
   <IRQ>
   bpf_trampoline_6442466513_0+0x18/0x1000
   migrate_disable+0x5/0x50
   __bpf_prog_enter+0x9/0x50
   bpf_trampoline_6442466513_0+0x18/0x1000
   migrate_disable+0x5/0x50
   __bpf_prog_enter+0x9/0x50
   bpf_trampoline_6442466513_0+0x18/0x1000
   migrate_disable+0x5/0x50
   __bpf_prog_enter+0x9/0x50
   bpf_trampoline_6442466513_0+0x18/0x1000
   migrate_disable+0x5/0x50
   ...
Fixing this by adding deny list of btf ids for tracing programs and checking btf id during program verification. Adding above functions to this list.
Suggested-by Alexei Starovoitov <[email protected]>
Signed-off-by Jiri Olsa <[email protected]>
---
v2 changes:
- drop check for EXT programs [Andrii]
 kernel/bpf/verifier.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)
kernel/bpf/verifier.c +0 -0
index 2579f6fbb5c3..42311e51ac71 100644
@@ -13112,6 +13112,17 @@ int bpf_check_attach_target(struct bpf_verifier_log *log,
        return 0;
 }
 
kernel/bpf/verifier.c int bpf_check_attach_target(struct bpf_verifier_log *log, · line 13124
Quoted 10 lines
kernel/bpf/verifier.c +10 -0
+BTF_SET_START(btf_id_deny)
+BTF_ID_UNUSED
+#ifdef CONFIG_SMP
+BTF_ID(func, migrate_disable)
+BTF_ID(func, migrate_enable)
+#endif
+#if !defined CONFIG_PREEMPT_RCU && !defined CONFIG_TINY_RCU
+BTF_ID(func, rcu_read_unlock_strict)
+#endif
+BTF_SET_END(btf_id_deny)

Quoted 8 lines

I was wondering whether it makes sense to do this on pahole side instead ? It can do more flexible regex matching and excluding all such functions from vmlinux btf without the kernel having to do a maze of #ifdef depending on config.
On one side we will lose BTF info about such functions, but what do we need it for?
On the other side it will be a tiny reduction in vmlinux btf :) Thoughts?

we just removed the ftrace filter so BTF will have 'all' functions

I think the filtering on pahole side could cause problems like the recent one with cubictcp_state.. it's just 3 functions, but what if they rename? this way we at least get compilation error ;-)

I'd go with all functions in BTF and restrict attachment for those that cause problems

jirka

Alexei Starovoitov

2021-05-11 21:06 UTC #3 lore raw

Quoted 76 lines · Jiri Olsa

Quoted 65 lines · Alexei Starovoitov
Quoted 53 lines · Jiri Olsa
The recursion check in __bpf_prog_enter and __bpf_prog_exit leaves some (not inlined) functions unprotected:
In __bpf_prog_enter:
- migrate_disable is called before prog->active is checked
In __bpf_prog_exit:
- migrate_enable,rcu_read_unlock_strict are called after
    prog->active is decreased
When attaching trampoline to them we get panic like:
traps: PANIC: double fault, error_code: 0x0 double fault: 0000 [#1] SMP PTI
  RIP: 0010:__bpf_prog_enter+0x4/0x50
  ...
  Call Trace:
   <IRQ>
   bpf_trampoline_6442466513_0+0x18/0x1000
   migrate_disable+0x5/0x50
   __bpf_prog_enter+0x9/0x50
   bpf_trampoline_6442466513_0+0x18/0x1000
   migrate_disable+0x5/0x50
   __bpf_prog_enter+0x9/0x50
   bpf_trampoline_6442466513_0+0x18/0x1000
   migrate_disable+0x5/0x50
   __bpf_prog_enter+0x9/0x50
   bpf_trampoline_6442466513_0+0x18/0x1000
   migrate_disable+0x5/0x50
   ...
Fixing this by adding deny list of btf ids for tracing programs and checking btf id during program verification. Adding above functions to this list.
Suggested-by Alexei Starovoitov <[email protected]>
Signed-off-by Jiri Olsa <[email protected]>
---
v2 changes:
- drop check for EXT programs [Andrii]
 kernel/bpf/verifier.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)
kernel/bpf/verifier.c +0 -0
index 2579f6fbb5c3..42311e51ac71 100644
@@ -13112,6 +13112,17 @@ int bpf_check_attach_target(struct bpf_verifier_log *log,
        return 0;
 }
 
kernel/bpf/verifier.c int bpf_check_attach_target(struct bpf_verifier_log *log, · line 13124
Quoted 10 lines
kernel/bpf/verifier.c +10 -0
+BTF_SET_START(btf_id_deny)
+BTF_ID_UNUSED
+#ifdef CONFIG_SMP
+BTF_ID(func, migrate_disable)
+BTF_ID(func, migrate_enable)
+#endif
+#if !defined CONFIG_PREEMPT_RCU && !defined CONFIG_TINY_RCU
+BTF_ID(func, rcu_read_unlock_strict)
+#endif
+BTF_SET_END(btf_id_deny)
Quoted 8 lines
I was wondering whether it makes sense to do this on pahole side instead ? It can do more flexible regex matching and excluding all such functions from vmlinux btf without the kernel having to do a maze of #ifdef depending on config.
On one side we will lose BTF info about such functions, but what do we need it for?
On the other side it will be a tiny reduction in vmlinux btf :) Thoughts?

Quoted 8 lines

we just removed the ftrace filter so BTF will have 'all' functions
I think the filtering on pahole side could cause problems like the recent one with cubictcp_state.. it's just 3 functions, but what if they rename? this way we at least get compilation error ;-)
I'd go with all functions in BTF and restrict attachment for those that cause problems

Ok. Let's see how it will work in practice.
Applied to bpf tree.