* [PATCH] bpf: Forbid trampoline attach for functions with variable arguments
@ 2021-05-05 13:25 Jiri Olsa
2021-05-05 18:45 ` Andrii Nakryiko
2021-05-06 23:30 ` patchwork-bot+netdevbpf
0 siblings, 2 replies; 5+ messages in thread
From: Jiri Olsa @ 2021-05-05 13:25 UTC (permalink / raw)
To: Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko
Cc: netdev, bpf, Martin KaFai Lau, Song Liu, Yonghong Song,
John Fastabend, KP Singh
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 <jolsa@kernel.org>
---
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
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] bpf: Forbid trampoline attach for functions with variable arguments
2021-05-05 13:25 [PATCH] bpf: Forbid trampoline attach for functions with variable arguments Jiri Olsa
@ 2021-05-05 18:45 ` Andrii Nakryiko
2021-05-06 23:31 ` Daniel Borkmann
2021-05-06 23:30 ` patchwork-bot+netdevbpf
1 sibling, 1 reply; 5+ messages in thread
From: Andrii Nakryiko @ 2021-05-05 18:45 UTC (permalink / raw)
To: Jiri Olsa
Cc: Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko, Networking,
bpf, Martin KaFai Lau, Song Liu, Yonghong Song, John Fastabend,
KP Singh
On Wed, May 5, 2021 at 6:42 AM Jiri Olsa <jolsa@kernel.org> 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 <jolsa@kernel.org>
> ---
LGTM.
Acked-by: Andrii Nakryiko <andrii@kernel.org>
> 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
>
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] bpf: Forbid trampoline attach for functions with variable arguments
2021-05-05 18:45 ` Andrii Nakryiko
@ 2021-05-06 23:31 ` Daniel Borkmann
2021-05-07 8:13 ` Jiri Olsa
0 siblings, 1 reply; 5+ messages in thread
From: Daniel Borkmann @ 2021-05-06 23:31 UTC (permalink / raw)
To: Andrii Nakryiko, Jiri Olsa
Cc: Alexei Starovoitov, Andrii Nakryiko, Networking, bpf,
Martin KaFai Lau, Song Liu, Yonghong Song, John Fastabend,
KP Singh
On 5/5/21 8:45 PM, Andrii Nakryiko wrote:
> On Wed, May 5, 2021 at 6:42 AM Jiri Olsa <jolsa@kernel.org> 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 <jolsa@kernel.org>
>> ---
>
> LGTM.
>
> Acked-by: Andrii Nakryiko <andrii@kernel.org>
>
>> 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
>>
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] bpf: Forbid trampoline attach for functions with variable arguments
2021-05-06 23:31 ` Daniel Borkmann
@ 2021-05-07 8:13 ` Jiri Olsa
0 siblings, 0 replies; 5+ messages in thread
From: Jiri Olsa @ 2021-05-07 8:13 UTC (permalink / raw)
To: Daniel Borkmann
Cc: Andrii Nakryiko, Jiri Olsa, Alexei Starovoitov, Andrii Nakryiko,
Networking, bpf, Martin KaFai Lau, Song Liu, Yonghong Song,
John Fastabend, KP Singh
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 <jolsa@kernel.org> 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 <jolsa@kernel.org>
> > > ---
> >
> > LGTM.
> >
> > Acked-by: Andrii Nakryiko <andrii@kernel.org>
> >
> > > 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
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] bpf: Forbid trampoline attach for functions with variable arguments
2021-05-05 13:25 [PATCH] bpf: Forbid trampoline attach for functions with variable arguments Jiri Olsa
2021-05-05 18:45 ` Andrii Nakryiko
@ 2021-05-06 23:30 ` patchwork-bot+netdevbpf
1 sibling, 0 replies; 5+ messages in thread
From: patchwork-bot+netdevbpf @ 2021-05-06 23:30 UTC (permalink / raw)
To: Jiri Olsa
Cc: ast, daniel, andriin, netdev, bpf, kafai, songliubraving, yhs,
john.fastabend, kpsingh
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
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2021-05-07 8:13 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-05-05 13:25 [PATCH] bpf: Forbid trampoline attach for functions with variable arguments Jiri Olsa
2021-05-05 18:45 ` Andrii Nakryiko
2021-05-06 23:31 ` Daniel Borkmann
2021-05-07 8:13 ` Jiri Olsa
2021-05-06 23:30 ` patchwork-bot+netdevbpf
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).