# \[PATCH\] bpf: Forbid trampoline attach for functions with variable arguments - Subsystems: BPF \[GENERAL\] (Safe Dynamic Programs and Tools), THE REST - Maintainers: Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko, Eduard Zingerman, Kumar Kartikeya Dwivedi, Linus Torvalds - Status: LANDED (31379397dcc3 ยท 2021-05-06 23:28) - Landed in mainline as 31379397dcc3 on 2021-05-06. - List: netdev - 5 messages, 5 authors - Page: https://ratatoskr.run/netdev/2021/05/7768519/t ## \[PATCH\] bpf: Forbid trampoline attach for functions with variable arguments - From: Jiri Olsa \ - Date: 2021-05-05 13:25:45 - Message: https://ratatoskr.run/netdev/2021/05/7768519.md ```text 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/20210429212834.82621-1-jolsa@kernel.org/ Signed-off-by: Jiri Olsa --- kernel/bpf/btf.c | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c index 0600ed325fa0..161511bb3e51 100644 --- a/kernel/bpf/btf.c +++ b/kernel/bpf/btf.c @@ -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 ``` ## Re: \[PATCH\] bpf: Forbid trampoline attach for functions with variable arguments - From: Andrii Nakryiko \ - Date: 2021-05-05 18:45:31 - Message: https://ratatoskr.run/netdev/2021/05/7768522.md ```text On Wed, May 5, 2021 at 6:42 AM Jiri Olsa [ref] 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/20210429212834.82621-1-jolsa@kernel.org/ > > Signed-off-by: Jiri Olsa > --- LGTM. Acked-by: Andrii Nakryiko > kernel/bpf/btf.c | 13 +++++++++++++ > 1 file changed, 13 insertions(+) > > diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c > index 0600ed325fa0..161511bb3e51 100644 > --- a/kernel/bpf/btf.c > +++ b/kernel/bpf/btf.c > @@ -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 > ``` ## Re: \[PATCH\] bpf: Forbid trampoline attach for functions with variable arguments - From: patchwork-bot+netdevbpf@kernel.org - Date: 2021-05-06 23:30:15 - Message: https://ratatoskr.run/netdev/2021/05/7768556.md ```text Hello: This patch was applied to bpf/bpf.git (refs/heads/master): 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/20210429212834.82621-1-jolsa@kernel.org/ > > [...] 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 ``` ## Re: \[PATCH\] bpf: Forbid trampoline attach for functions with variable arguments - From: Daniel Borkmann \ - Date: 2021-05-06 23:32:00 - Message: https://ratatoskr.run/netdev/2021/05/7768558.md ```text On 5/5/21 8:45 PM, Andrii Nakryiko wrote: > On Wed, May 5, 2021 at 6:42 AM Jiri Olsa [ref] 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/20210429212834.82621-1-jolsa@kernel.org/ >> >> Signed-off-by: Jiri Olsa >> --- > > LGTM. > > Acked-by: Andrii Nakryiko > >> kernel/bpf/btf.c | 13 +++++++++++++ >> 1 file changed, 13 insertions(+) >> >> diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c >> index 0600ed325fa0..161511bb3e51 100644 >> --- a/kernel/bpf/btf.c >> +++ b/kernel/bpf/btf.c >> @@ -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; >> + (Jiri, fyi, I removed this extra newline while applying. Please scan for such things before submitting.) >> + } >> 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 >> ``` ## Re: \[PATCH\] bpf: Forbid trampoline attach for functions with variable arguments - From: Jiri Olsa \ - Date: 2021-05-07 08:13:25 - Message: https://ratatoskr.run/netdev/2021/05/7768573.md ```text On Fri, May 07, 2021 at 01:31:54AM +0200, Daniel Borkmann wrote: > On 5/5/21 8:45 PM, Andrii Nakryiko wrote: > > On Wed, May 5, 2021 at 6:42 AM Jiri Olsa [ref] 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/20210429212834.82621-1-jolsa@kernel.org/ > > > > > > Signed-off-by: Jiri Olsa > > > --- > > > > LGTM. > > > > Acked-by: Andrii Nakryiko > > > > > kernel/bpf/btf.c | 13 +++++++++++++ > > > 1 file changed, 13 insertions(+) > > > > > > diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c > > > index 0600ed325fa0..161511bb3e51 100644 > > > --- a/kernel/bpf/btf.c > > > +++ b/kernel/bpf/btf.c > > > @@ -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; > > > + > > (Jiri, fyi, I removed this extra newline while applying. Please scan for such > things before submitting.) sorry, will do jirka ```