netdev - Re: [PATCH bpf-next] bpf, seccomp: fix false positive preemption splat for cbpf->ebpf progs

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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <CAADnVQKjtPAaSnrTnTO_oCk5wp-8QyuLOa6XT9OMhyx9PsHr3Q@mail.gmail.com>
Date:   Wed, 20 Feb 2019 10:34:45 -0800
From:   Alexei Starovoitov <alexei.starovoitov@...il.com>
To:     Daniel Borkmann <daniel@...earbox.net>
Cc:     Alexei Starovoitov <ast@...nel.org>,
        Network Development <netdev@...r.kernel.org>
Subject: Re: [PATCH bpf-next] bpf, seccomp: fix false positive preemption
 splat for cbpf->ebpf progs

On Wed, Feb 20, 2019 at 10:27 AM Daniel Borkmann <daniel@...earbox.net> wrote:
>
> On 02/20/2019 06:07 PM, Alexei Starovoitov wrote:
> > On Wed, Feb 20, 2019 at 12:06:29PM +0100, Daniel Borkmann wrote:
> >> In 568f196756ad ("bpf: check that BPF programs run with preemption disabled")
> >> a check was added for BPF_PROG_RUN() that for every invocation preemption has
> >> to be disabled to not break eBPF assumptions (e.g. per-cpu map). Of course this
> >> does not count for seccomp because only cBPF -> eBPF is loaded here and it does
> >> not make use of any functionality that would require this assertion. Fix this
> >> false positive by adding and using __BPF_PROG_RUN() variant that does not have
> >> the cant_sleep(); check.
> >>
> >> Fixes: 568f196756ad ("bpf: check that BPF programs run with preemption disabled")
> >> Reported-by: syzbot+8bf19ee2aa580de7a2a7@...kaller.appspotmail.com
> >> Signed-off-by: Daniel Borkmann <daniel@...earbox.net>
> >> ---
> >>  include/linux/filter.h | 9 ++++++++-
> >>  kernel/seccomp.c       | 2 +-
> >>  2 files changed, 9 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/include/linux/filter.h b/include/linux/filter.h
> >> index f32b3ec..2648fd7 100644
> >> --- a/include/linux/filter.h
> >> +++ b/include/linux/filter.h
> >> @@ -533,7 +533,14 @@ struct sk_filter {
> >>      struct bpf_prog *prog;
> >>  };
> >>
> >> -#define BPF_PROG_RUN(filter, ctx)  ({ cant_sleep(); (*(filter)->bpf_func)(ctx, (filter)->insnsi); })
> >> +#define bpf_prog_run__non_preempt(prog, ctx)        \
> >> +    ({ cant_sleep(); __BPF_PROG_RUN(prog, ctx); })
> >> +/* Native eBPF or cBPF -> eBPF transitions. Preemption must be disabled. */
> >> +#define BPF_PROG_RUN(prog, ctx)                     \
> >> +    bpf_prog_run__non_preempt(prog, ctx)
> >> +/* Direct use for cBPF -> eBPF only, but not for native eBPF. */
> >
> > I think the comment is too abstract.
> > May be it should say that this is seccomp cBPF only ?
> > And macro name should be explicit as well ?
>
> I think macro naming is probably okay imho as used internally as
> well from BPF_PROG_RUN(), but I'll improve the comment to state
> seccomp specifically as an example there and providing some more
> background.

I'm worried about misuse of the macro.
If there was a word seccomp in it it would made people
think much harder before calling it.