Skip to contentSkip to conversation

bpf: Forbid trampoline attach for functions with variable arguments

PATCH Applied Jiri Olsa 5 participants 5 messages May 5, 2021 – May 7, 2021

Subsystems (3)
Networking Network drivers BPF
lore
Share & tools
lore

Jiri Olsa

author reviewer
#0 lore raw

PATCH 1/1 bpf: Forbid trampoline attach for functions with variable arguments

We can't currently allow to attach functions with variable arguments. The problem is that we should save all the registers for arguments, which is probably doable, but if caller uses more than 6 arguments, we need stack data, which will be wrong, because of the extra stack frame we do in bpf trampoline, so we could crash.

Also currently there's malformed trampoline code generated for such functions at the moment as described in:
https://lore.kernel.org/bpf/[email protected]/

Signed-off-by Jiri Olsa <[email protected]>
---
 kernel/bpf/btf.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)
kernel/bpf/btf.c +13 -0
index 0600ed325fa0..161511bb3e51 100644
@@ -5206,6 +5206,13 @@ int btf_distill_func_proto(struct bpf_verifier_log *log,
 	m->ret_size = ret;
 
 	for (i = 0; i < nargs; i++) {
+		if (i == nargs - 1 && args[i].type == 0) {
+			bpf_log(log,
+				"The function %s with variable args is unsupported.\n",
+				tname);
+			return -EINVAL;
+
+		}
 		ret = __get_type_size(btf, args[i].type, &t);
 		if (ret < 0) {
 			bpf_log(log,

@@ -5213,6 +5220,12 @@ int btf_distill_func_proto(struct bpf_verifier_log *log,
 				tname, i, btf_kind_str[BTF_INFO_KIND(t->info)]);
 			return -EINVAL;
 		}
+		if (ret == 0) {
+			bpf_log(log,
+				"The function %s has malformed void argument.\n",
+				tname);
+			return -EINVAL;
+		}
 		m->arg_size[i] = ret;
 	}
 	m->nr_args = nargs;
Signature · 1 line
2.30.2
Acked-by on v1 1/1
#1 lore raw
Quoted 7 lines · Jiri Olsa

We can't currently allow to attach functions with variable arguments. The problem is that we should save all the registers for arguments, which is probably doable, but if caller uses more than 6 arguments, we need stack data, which will be wrong, because of the extra stack frame we do in bpf trampoline, so we could crash.

Quoted 6 lines

Also currently there's malformed trampoline code generated for such functions at the moment as described in:
https://lore.kernel.org/bpf/[email protected]/

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

---

LGTM.

Acked-by Andrii Nakryiko <[email protected]>
Quoted 37 lines
 kernel/bpf/btf.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)
kernel/bpf/btf.c +13 -1
index 0600ed325fa0..161511bb3e51 100644
@@ -5206,6 +5206,13 @@ int btf_distill_func_proto(struct bpf_verifier_log *log,
        m->ret_size = ret;
 
        for (i = 0; i < nargs; i++) {
+               if (i == nargs - 1 && args[i].type == 0) {
+                       bpf_log(log,
+                               "The function %s with variable args is unsupported.\n",
+                               tname);
+                       return -EINVAL;
+
+               }
                ret = __get_type_size(btf, args[i].type, &t);
                if (ret < 0) {
                        bpf_log(log,

@@ -5213,6 +5220,12 @@ int btf_distill_func_proto(struct bpf_verifier_log *log,
                                tname, i, btf_kind_str[BTF_INFO_KIND(t->info)]);
                        return -EINVAL;
                }
+               if (ret == 0) {
+                       bpf_log(log,
+                               "The function %s has malformed void argument.\n",
+                               tname);
+                       return -EINVAL;
+               }
                m->arg_size[i] = ret;
        }
        m->nr_args = nargs;
--

2.30.2

maintainer
#2 lore raw
Quoted 29 lines · Andrii Nakryiko
Quoted 7 lines · Jiri Olsa

We can't currently allow to attach functions with variable arguments. The problem is that we should save all the registers for arguments, which is probably doable, but if caller uses more than 6 arguments, we need stack data, which will be wrong, because of the extra stack frame we do in bpf trampoline, so we could crash.

Quoted 6 lines

Also currently there's malformed trampoline code generated for such functions at the moment as described in:

   https://lore.kernel.org/bpf/[email protected]/
Signed-off-by Jiri Olsa <[email protected]>

---

LGTM.

Acked-by Andrii Nakryiko <[email protected]>
Quoted 10 lines
  kernel/bpf/btf.c | 13 +++++++++++++
  1 file changed, 13 insertions(+)
kernel/bpf/btf.c +0 -0
index 0600ed325fa0..161511bb3e51 100644
@@ -5206,6 +5206,13 @@ int btf_distill_func_proto(struct bpf_verifier_log *log,
         m->ret_size = ret;
Quoted 7 lines

kernel/bpf/btf.c int btf_distill_func_proto(struct bpf_verifier_log *log, · line 5214

Quoted 7 lines
kernel/bpf/btf.c +6 -0
         for (i = 0; i < nargs; i++) {
+               if (i == nargs - 1 && args[i].type == 0) {
+                       bpf_log(log,
+                               "The function %s with variable args is unsupported.\n",
+                               tname);
+                       return -EINVAL;
+

(Jiri, fyi, I removed this extra newline while applying. Please scan for such things before submitting.)

Quoted 20 lines
Quoted 20 lines
kernel/bpf/btf.c +7 -1
+               }
                 ret = __get_type_size(btf, args[i].type, &t);
                 if (ret < 0) {
                         bpf_log(log,

@@ -5213,6 +5220,12 @@ int btf_distill_func_proto(struct bpf_verifier_log *log,
                                 tname, i, btf_kind_str[BTF_INFO_KIND(t->info)]);
                         return -EINVAL;
                 }
+               if (ret == 0) {
+                       bpf_log(log,
+                               "The function %s has malformed void argument.\n",
+                               tname);
+                       return -EINVAL;
+               }
                 m->arg_size[i] = ret;
         }
         m->nr_args = nargs;
--

2.30.2

#3 lore raw
Quoted 30 lines · Daniel Borkmann
Quoted 29 lines · Andrii Nakryiko
Quoted 7 lines · Jiri Olsa

We can't currently allow to attach functions with variable arguments. The problem is that we should save all the registers for arguments, which is probably doable, but if caller uses more than 6 arguments, we need stack data, which will be wrong, because of the extra stack frame we do in bpf trampoline, so we could crash.

Quoted 6 lines

Also currently there's malformed trampoline code generated for such functions at the moment as described in:

   https://lore.kernel.org/bpf/[email protected]/
Signed-off-by Jiri Olsa <[email protected]>

---

LGTM.

Acked-by Andrii Nakryiko <[email protected]>
Quoted 10 lines
  kernel/bpf/btf.c | 13 +++++++++++++
  1 file changed, 13 insertions(+)
kernel/bpf/btf.c +0 -0
index 0600ed325fa0..161511bb3e51 100644
@@ -5206,6 +5206,13 @@ int btf_distill_func_proto(struct bpf_verifier_log *log,
         m->ret_size = ret;
Quoted 10 lines
Quoted 7 lines

kernel/bpf/btf.c int btf_distill_func_proto(struct bpf_verifier_log *log, · line 5214

Quoted 7 lines
kernel/bpf/btf.c +6 -0
         for (i = 0; i < nargs; i++) {
+               if (i == nargs - 1 && args[i].type == 0) {
+                       bpf_log(log,
+                               "The function %s with variable args is unsupported.\n",
+                               tname);
+                       return -EINVAL;
+

(Jiri, fyi, I removed this extra newline while applying. Please scan for such things before submitting.)

sorry, will do

jirka

CI Applied
#4 lore raw
In reply to Jiri Olsa
This patch was applied to bpf/bpf.git (refs/heads/master):

Hello:

This patch was applied to bpf/bpf.git (refs/heads/master):

Quoted 11 lines · On Wed, 5 May 2021 15:25:29 +0200 you wrote:

We can't currently allow to attach functions with variable arguments. The problem is that we should save all the registers for arguments, which is probably doable, but if caller uses more than 6 arguments, we need stack data, which will be wrong, because of the extra stack frame we do in bpf trampoline, so we could crash.

Also currently there's malformed trampoline code generated for such functions at the moment as described in:
https://lore.kernel.org/bpf/[email protected]/

[...]

Here is the summary with links:
- bpf: Forbid trampoline attach for functions with variable arguments

    https://git.kernel.org/bpf/bpf/c/0a1a616720d9

You are awesome, thank you!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html