| Message ID | 20190220110629.21646-1-daniel@iogearbox.net |
|---|---|
| State | Superseded |
| Delegated to: | BPF Maintainers |
| Headers | show |
| Series | [bpf-next] bpf, seccomp: fix false positive preemption splat for cbpf->ebpf progs | expand |
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@syzkaller.appspotmail.com > Signed-off-by: Daniel Borkmann <daniel@iogearbox.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 ? > +#define __BPF_PROG_RUN(prog, ctx) \ > + (*(prog)->bpf_func)(ctx, (prog)->insnsi) > > #define BPF_SKB_CB_LEN QDISC_CB_PRIV_LEN > > diff --git a/kernel/seccomp.c b/kernel/seccomp.c > index e815781..826d4e4 100644 > --- a/kernel/seccomp.c > +++ b/kernel/seccomp.c > @@ -268,7 +268,7 @@ static u32 seccomp_run_filters(const struct seccomp_data *sd, > * value always takes priority (ignoring the DATA). > */ > for (; f; f = f->prev) { > - u32 cur_ret = BPF_PROG_RUN(f->prog, sd); > + u32 cur_ret = __BPF_PROG_RUN(f->prog, sd); > > if (ACTION_ONLY(cur_ret) < ACTION_ONLY(ret)) { > ret = cur_ret; > -- > 2.9.5 >
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@syzkaller.appspotmail.com >> Signed-off-by: Daniel Borkmann <daniel@iogearbox.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. >> +#define __BPF_PROG_RUN(prog, ctx) \ >> + (*(prog)->bpf_func)(ctx, (prog)->insnsi) >> >> #define BPF_SKB_CB_LEN QDISC_CB_PRIV_LEN >> >> diff --git a/kernel/seccomp.c b/kernel/seccomp.c >> index e815781..826d4e4 100644 >> --- a/kernel/seccomp.c >> +++ b/kernel/seccomp.c >> @@ -268,7 +268,7 @@ static u32 seccomp_run_filters(const struct seccomp_data *sd, >> * value always takes priority (ignoring the DATA). >> */ >> for (; f; f = f->prev) { >> - u32 cur_ret = BPF_PROG_RUN(f->prog, sd); >> + u32 cur_ret = __BPF_PROG_RUN(f->prog, sd); >> >> if (ACTION_ONLY(cur_ret) < ACTION_ONLY(ret)) { >> ret = cur_ret; >> -- >> 2.9.5 >>
On Wed, Feb 20, 2019 at 10:27 AM Daniel Borkmann <daniel@iogearbox.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@syzkaller.appspotmail.com > >> Signed-off-by: Daniel Borkmann <daniel@iogearbox.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.
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. */ +#define __BPF_PROG_RUN(prog, ctx) \ + (*(prog)->bpf_func)(ctx, (prog)->insnsi) #define BPF_SKB_CB_LEN QDISC_CB_PRIV_LEN diff --git a/kernel/seccomp.c b/kernel/seccomp.c index e815781..826d4e4 100644 --- a/kernel/seccomp.c +++ b/kernel/seccomp.c @@ -268,7 +268,7 @@ static u32 seccomp_run_filters(const struct seccomp_data *sd, * value always takes priority (ignoring the DATA). */ for (; f; f = f->prev) { - u32 cur_ret = BPF_PROG_RUN(f->prog, sd); + u32 cur_ret = __BPF_PROG_RUN(f->prog, sd); if (ACTION_ONLY(cur_ret) < ACTION_ONLY(ret)) { ret = cur_ret;
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@syzkaller.appspotmail.com Signed-off-by: Daniel Borkmann <daniel@iogearbox.net> --- include/linux/filter.h | 9 ++++++++- kernel/seccomp.c | 2 +- 2 files changed, 9 insertions(+), 2 deletions(-)