netdev - Re: [PATCH RFC] bpf: Fix trampoline for functions with variable arguments

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Tue, 4 May 2021 21:11:26 -0700
From:   Alexei Starovoitov <alexei.starovoitov@...il.com>
To:     Andrii Nakryiko <andrii.nakryiko@...il.com>
Cc:     Jiri Olsa <jolsa@...hat.com>, Jiri Olsa <jolsa@...nel.org>,
        Alexei Starovoitov <ast@...nel.org>,
        Daniel Borkmann <daniel@...earbox.net>,
        Andrii Nakryiko <andriin@...com>,
        Networking <netdev@...r.kernel.org>, bpf <bpf@...r.kernel.org>,
        Martin KaFai Lau <kafai@...com>,
        Song Liu <songliubraving@...com>, Yonghong Song <yhs@...com>,
        John Fastabend <john.fastabend@...il.com>,
        KP Singh <kpsingh@...omium.org>
Subject: Re: [PATCH RFC] bpf: Fix trampoline for functions with variable arguments

On Tue, May 4, 2021 at 3:37 PM Andrii Nakryiko
<andrii.nakryiko@...il.com> wrote:
>
> On Tue, May 4, 2021 at 6:27 AM Jiri Olsa <jolsa@...hat.com> wrote:
> >
> > On Mon, May 03, 2021 at 03:32:34PM -0700, Andrii Nakryiko wrote:
> > > On Sun, May 2, 2021 at 2:17 PM Jiri Olsa <jolsa@...hat.com> wrote:
> > > >
> > > > On Thu, Apr 29, 2021 at 11:28:34PM +0200, Jiri Olsa wrote:
> > > > > For functions with variable arguments like:
> > > > >
> > > > >   void set_worker_desc(const char *fmt, ...)
> > > > >
> > > > > the BTF data contains void argument at the end:
> > > > >
> > > > > [4061] FUNC_PROTO '(anon)' ret_type_id=0 vlen=2
> > > > >         'fmt' type_id=3
> > > > >         '(anon)' type_id=0
> > > > >
> > > > > When attaching function with this void argument the btf_distill_func_proto
> > > > > will set last btf_func_model's argument with size 0 and that
> > > > > will cause extra loop in save_regs/restore_regs functions and
> > > > > generate trampoline code like:
> > > > >
> > > > >   55             push   %rbp
> > > > >   48 89 e5       mov    %rsp,%rbp
> > > > >   48 83 ec 10    sub    $0x10,%rsp
> > > > >   53             push   %rbx
> > > > >   48 89 7d f0    mov    %rdi,-0x10(%rbp)
> > > > >   75 f8          jne    0xffffffffa00cf007
> > > > >                  ^^^ extra jump
> > > > >
> > > > > It's causing soft lockups/crashes probably depends on what context
> > > > > is the attached function called, like for set_worker_desc:
> > > > >
> > > > >   watchdog: BUG: soft lockup - CPU#16 stuck for 22s! [kworker/u40:4:239]
> > > > >   CPU: 16 PID: 239 Comm: kworker/u40:4 Not tainted 5.12.0-rc4qemu+ #178
> > > > >   Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS 1.14.0-1.fc33 04/01/2014
> > > > >   Workqueue: writeback wb_workfn
> > > > >   RIP: 0010:bpf_trampoline_6442464853_0+0xa/0x1000
> > > > >   Code: Unable to access opcode bytes at RIP 0xffffffffa3597fe0.
> > > > >   RSP: 0018:ffffc90000687da8 EFLAGS: 00000217
> > > > >   Call Trace:
> > > > >    set_worker_desc+0x5/0xb0
> > > > >    wb_workfn+0x48/0x4d0
> > > > >    ? psi_group_change+0x41/0x210
> > > > >    ? __bpf_prog_exit+0x15/0x20
> > > > >    ? bpf_trampoline_6442458903_0+0x3b/0x1000
> > > > >    ? update_pasid+0x5/0x90
> > > > >    ? __switch_to+0x187/0x450
> > > > >    process_one_work+0x1e7/0x380
> > > > >    worker_thread+0x50/0x3b0
> > > > >    ? rescuer_thread+0x380/0x380
> > > > >    kthread+0x11b/0x140
> > > > >    ? __kthread_bind_mask+0x60/0x60
> > > > >    ret_from_fork+0x22/0x30
> > > > >
> > > > > This patch is removing the void argument from struct btf_func_model
> > > > > in btf_distill_func_proto, but perhaps we should also check for this
> > > > > in JIT's save_regs/restore_regs functions.
> > > >
> > > > actualy looks like we need to disable functions with variable arguments
> > > > completely, because we don't know how many arguments to save
> > > >
> > > > I tried to disable them in pahole and it's easy fix, will post new fix
> > >
> > > Can we still allow access to fixed arguments for such functions and
> > > just disallow the vararg ones?
> >
> > 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
> >
> > the patch below prevents to attach these functions directly in kernel,
> > so we could keep these functions in BTF
> >
> > jirka
> >
> >
> > ---
> > diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
> > index 0600ed325fa0..f9709dc08c44 100644
> > --- a/kernel/bpf/btf.c
> > +++ b/kernel/bpf/btf.c
> > @@ -5213,6 +5213,13 @@ 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 variable args, it's unsupported.\n",
> > +                               tname);
> > +                       return -EINVAL;
> > +
> > +               }
>
> this will work, but the explicit check for vararg should be `i ==
> nargs - 1 && args[i].type == 0`. Everything else (if it happens) is
> probably a bad BTF data.

Jiri,
could you please resubmit with the check like Andrii suggested?
Thanks!