| Message ID | 20230912233214.1518551-13-memxor@gmail.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | fd548e1a46185000191a89cae4be560e076ed6c7 |
| Delegated to: | BPF |
| Headers | show |
| Series | Exceptions - 1/2 | expand |
On Wed, Sep 13 2023, Kumar Kartikeya Dwivedi wrote: > During testing, it was discovered that extensions to exception callbacks > had no checks, upon running a testcase, the kernel ended up running off > the end of a program having final call as bpf_throw, and hitting int3 > instructions. > > The reason is that while the default exception callback would have reset > the stack frame to return back to the main program's caller, the > replacing extension program will simply return back to bpf_throw, which > will instead return back to the program and the program will continue > execution, now in an undefined state where anything could happen. > > The way to support extensions to an exception callback would be to mark > the BPF_PROG_TYPE_EXT main subprog as an exception_cb, and prevent it > from calling bpf_throw. This would make the JIT produce a prologue that > restores saved registers and reset the stack frame. But let's not do > that until there is a concrete use case for this, and simply disallow > this for now. > > Similar issues will exist for fentry and fexit cases, where trampoline > saves data on the stack when invoking exception callback, which however > will then end up resetting the stack frame, and on return, the fexit > program will never will invoked as the return address points to the main > program's caller in the kernel. Instead of additional complexity and > back and forth between the two stacks to enable such a use case, simply > forbid it. > > One key point here to note is that currently X86_TAIL_CALL_OFFSET didn't > require any modifications, even though we emit instructions before the > corresponding endbr64 instruction. This is because we ensure that a main > subprog never serves as an exception callback, and therefore the > exception callback (which will be a global subprog) can never serve as > the tail call target, eliminating any discrepancies. However, once we > support a BPF_PROG_TYPE_EXT to also act as an exception callback, it > will end up requiring change to the tail call offset to account for the > extra instructions. For simplicitly, tail calls could be disabled for > such targets. > > Noting the above, it appears better to wait for a concrete use case > before choosing to permit extension programs to replace exception > callbacks. > > As a precaution, we disable fentry and fexit for exception callbacks as > well. > > Signed-off-by: Kumar Kartikeya Dwivedi <memxor@gmail.com> > --- > kernel/bpf/helpers.c | 1 + > kernel/bpf/verifier.c | 11 +++++++++++ > 2 files changed, 12 insertions(+) > > diff --git a/kernel/bpf/helpers.c b/kernel/bpf/helpers.c > index 2c8e1ee97b71..7ff2a42f1996 100644 > --- a/kernel/bpf/helpers.c > +++ b/kernel/bpf/helpers.c > @@ -2490,6 +2490,7 @@ __bpf_kfunc void bpf_throw(u64 cookie) > */ > kasan_unpoison_task_stack_below((void *)ctx.sp); > ctx.aux->bpf_exception_cb(cookie, ctx.sp, ctx.bp); > + WARN(1, "A call to BPF exception callback should never return\n"); > } > > __diag_pop(); > diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c > index 0ba32b626320..21e37e46d792 100644 > --- a/kernel/bpf/verifier.c > +++ b/kernel/bpf/verifier.c > @@ -19750,6 +19750,12 @@ int bpf_check_attach_target(struct bpf_verifier_log *log, > bpf_log(log, "Subprog %s doesn't exist\n", tname); > return -EINVAL; > } > + if (aux->func && aux->func[subprog]->aux->exception_cb) { > + bpf_log(log, > + "%s programs cannot attach to exception callback\n", > + prog_extension ? "Extension" : "FENTRY/FEXIT"); > + return -EINVAL; > + } > conservative = aux->func_info_aux[subprog].unreliable; > if (prog_extension) { > if (conservative) { > @@ -19762,6 +19768,11 @@ int bpf_check_attach_target(struct bpf_verifier_log *log, > "Extension programs should be JITed\n"); > return -EINVAL; > } > + if (aux->func && aux->func[subprog]->aux->exception_cb) { > + bpf_log(log, > + "Extension programs cannot replace exception callback\n"); > + return -EINVAL; > + } This check is redundant because you already did this check above if (prog_extension branch) Remove this as it will never be reached. > } > if (!tgt_prog->jited) { > bpf_log(log, "Can attach to only JITed progs\n"); Thanks, Puranjay
On Wed, 13 Sept 2023 at 17:24, Puranjay Mohan <puranjay12@gmail.com> wrote: > > On Wed, Sep 13 2023, Kumar Kartikeya Dwivedi wrote: > > > During testing, it was discovered that extensions to exception callbacks > > had no checks, upon running a testcase, the kernel ended up running off > > the end of a program having final call as bpf_throw, and hitting int3 > > instructions. > > > > The reason is that while the default exception callback would have reset > > the stack frame to return back to the main program's caller, the > > replacing extension program will simply return back to bpf_throw, which > > will instead return back to the program and the program will continue > > execution, now in an undefined state where anything could happen. > > > > The way to support extensions to an exception callback would be to mark > > the BPF_PROG_TYPE_EXT main subprog as an exception_cb, and prevent it > > from calling bpf_throw. This would make the JIT produce a prologue that > > restores saved registers and reset the stack frame. But let's not do > > that until there is a concrete use case for this, and simply disallow > > this for now. > > > > Similar issues will exist for fentry and fexit cases, where trampoline > > saves data on the stack when invoking exception callback, which however > > will then end up resetting the stack frame, and on return, the fexit > > program will never will invoked as the return address points to the main > > program's caller in the kernel. Instead of additional complexity and > > back and forth between the two stacks to enable such a use case, simply > > forbid it. > > > > One key point here to note is that currently X86_TAIL_CALL_OFFSET didn't > > require any modifications, even though we emit instructions before the > > corresponding endbr64 instruction. This is because we ensure that a main > > subprog never serves as an exception callback, and therefore the > > exception callback (which will be a global subprog) can never serve as > > the tail call target, eliminating any discrepancies. However, once we > > support a BPF_PROG_TYPE_EXT to also act as an exception callback, it > > will end up requiring change to the tail call offset to account for the > > extra instructions. For simplicitly, tail calls could be disabled for > > such targets. > > > > Noting the above, it appears better to wait for a concrete use case > > before choosing to permit extension programs to replace exception > > callbacks. > > > > As a precaution, we disable fentry and fexit for exception callbacks as > > well. > > > > Signed-off-by: Kumar Kartikeya Dwivedi <memxor@gmail.com> > > --- > > kernel/bpf/helpers.c | 1 + > > kernel/bpf/verifier.c | 11 +++++++++++ > > 2 files changed, 12 insertions(+) > > > > diff --git a/kernel/bpf/helpers.c b/kernel/bpf/helpers.c > > index 2c8e1ee97b71..7ff2a42f1996 100644 > > --- a/kernel/bpf/helpers.c > > +++ b/kernel/bpf/helpers.c > > @@ -2490,6 +2490,7 @@ __bpf_kfunc void bpf_throw(u64 cookie) > > */ > > kasan_unpoison_task_stack_below((void *)ctx.sp); > > ctx.aux->bpf_exception_cb(cookie, ctx.sp, ctx.bp); > > + WARN(1, "A call to BPF exception callback should never return\n"); > > } > > > > __diag_pop(); > > diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c > > index 0ba32b626320..21e37e46d792 100644 > > --- a/kernel/bpf/verifier.c > > +++ b/kernel/bpf/verifier.c > > @@ -19750,6 +19750,12 @@ int bpf_check_attach_target(struct bpf_verifier_log *log, > > bpf_log(log, "Subprog %s doesn't exist\n", tname); > > return -EINVAL; > > } > > + if (aux->func && aux->func[subprog]->aux->exception_cb) { > > + bpf_log(log, > > + "%s programs cannot attach to exception callback\n", > > + prog_extension ? "Extension" : "FENTRY/FEXIT"); > > + return -EINVAL; > > + } > > conservative = aux->func_info_aux[subprog].unreliable; > > if (prog_extension) { > > if (conservative) { > > @@ -19762,6 +19768,11 @@ int bpf_check_attach_target(struct bpf_verifier_log *log, > > "Extension programs should be JITed\n"); > > return -EINVAL; > > } > > + if (aux->func && aux->func[subprog]->aux->exception_cb) { > > + bpf_log(log, > > + "Extension programs cannot replace exception callback\n"); > > + return -EINVAL; > > + } > > This check is redundant because you already did this check above if (prog_extension branch) > Remove this as it will never be reached. > Good catch, will fix it in v4. > > > } > > if (!tgt_prog->jited) { > > bpf_log(log, "Can attach to only JITed progs\n"); > > > Thanks, > Puranjay
On Thu, Sep 14, 2023 at 5:13 AM Kumar Kartikeya Dwivedi <memxor@gmail.com> wrote: > > > } > > > + if (aux->func && aux->func[subprog]->aux->exception_cb) { > > > + bpf_log(log, > > > + "Extension programs cannot replace exception callback\n"); > > > + return -EINVAL; > > > + } > > > > This check is redundant because you already did this check above if (prog_extension branch) > > Remove this as it will never be reached. > > > > Good catch, will fix it in v4. No worries. I fixed this duplicate check while applying. Everything else can be addressed in the follow ups. This spam is a bit annoying: $ ./test_progs -t exceptions func#0 @0 FENTRY/FEXIT programs cannot attach to exception callback processed 0 insns (limit 1000000) max_states_per_insn 0 total_states 0 peak_states 0 mark_read 0 func#0 @0 FENTRY/FEXIT programs cannot attach to exception callback processed 0 insns (limit 1000000) max_states_per_insn 0 total_states 0 peak_states 0 mark_read 0
On Sat, 16 Sept 2023 at 18:44, Alexei Starovoitov <alexei.starovoitov@gmail.com> wrote: > > On Thu, Sep 14, 2023 at 5:13 AM Kumar Kartikeya Dwivedi > <memxor@gmail.com> wrote: > > > > } > > > > + if (aux->func && aux->func[subprog]->aux->exception_cb) { > > > > + bpf_log(log, > > > > + "Extension programs cannot replace exception callback\n"); > > > > + return -EINVAL; > > > > + } > > > > > > This check is redundant because you already did this check above if (prog_extension branch) > > > Remove this as it will never be reached. > > > > > > > Good catch, will fix it in v4. > > No worries. I fixed this duplicate check while applying. > Everything else can be addressed in the follow ups. > > This spam is a bit annoying: > $ ./test_progs -t exceptions > func#0 @0 > FENTRY/FEXIT programs cannot attach to exception callback > processed 0 insns (limit 1000000) max_states_per_insn 0 total_states 0 > peak_states 0 mark_read 0 > > func#0 @0 > FENTRY/FEXIT programs cannot attach to exception callback > processed 0 insns (limit 1000000) max_states_per_insn 0 total_states 0 > peak_states 0 mark_read 0 Thanks for fixing it while applying. I will send a follow up to silence these logs today.
On Sat, 16 Sept 2023 at 19:30, Kumar Kartikeya Dwivedi <memxor@gmail.com> wrote: > > On Sat, 16 Sept 2023 at 18:44, Alexei Starovoitov > <alexei.starovoitov@gmail.com> wrote: > > > > On Thu, Sep 14, 2023 at 5:13 AM Kumar Kartikeya Dwivedi > > <memxor@gmail.com> wrote: > > > > > } > > > > > + if (aux->func && aux->func[subprog]->aux->exception_cb) { > > > > > + bpf_log(log, > > > > > + "Extension programs cannot replace exception callback\n"); > > > > > + return -EINVAL; > > > > > + } > > > > > > > > This check is redundant because you already did this check above if (prog_extension branch) > > > > Remove this as it will never be reached. > > > > > > > > > > Good catch, will fix it in v4. > > > > No worries. I fixed this duplicate check while applying. > > Everything else can be addressed in the follow ups. > > > > This spam is a bit annoying: > > $ ./test_progs -t exceptions > > func#0 @0 > > FENTRY/FEXIT programs cannot attach to exception callback > > processed 0 insns (limit 1000000) max_states_per_insn 0 total_states 0 > > peak_states 0 mark_read 0 > > > > func#0 @0 > > FENTRY/FEXIT programs cannot attach to exception callback > > processed 0 insns (limit 1000000) max_states_per_insn 0 total_states 0 > > peak_states 0 mark_read 0 > > Thanks for fixing it while applying. I will send a follow up to > silence these logs today. For some reason, I don't seem to see these when just running ./test_progs -t exceptions. I am not sure what I'm doing differently when running the selftests. A bit weird, but anyway, just guessing the cause, do you see them when you apply this?
On Sat, Sep 16, 2023 at 12:35 PM Kumar Kartikeya Dwivedi <memxor@gmail.com> wrote: > > On Sat, 16 Sept 2023 at 19:30, Kumar Kartikeya Dwivedi <memxor@gmail.com> wrote: > > > > On Sat, 16 Sept 2023 at 18:44, Alexei Starovoitov > > <alexei.starovoitov@gmail.com> wrote: > > > > > > On Thu, Sep 14, 2023 at 5:13 AM Kumar Kartikeya Dwivedi > > > <memxor@gmail.com> wrote: > > > > > > } > > > > > > + if (aux->func && aux->func[subprog]->aux->exception_cb) { > > > > > > + bpf_log(log, > > > > > > + "Extension programs cannot replace exception callback\n"); > > > > > > + return -EINVAL; > > > > > > + } > > > > > > > > > > This check is redundant because you already did this check above if (prog_extension branch) > > > > > Remove this as it will never be reached. > > > > > > > > > > > > > Good catch, will fix it in v4. > > > > > > No worries. I fixed this duplicate check while applying. > > > Everything else can be addressed in the follow ups. > > > > > > This spam is a bit annoying: > > > $ ./test_progs -t exceptions > > > func#0 @0 > > > FENTRY/FEXIT programs cannot attach to exception callback > > > processed 0 insns (limit 1000000) max_states_per_insn 0 total_states 0 > > > peak_states 0 mark_read 0 > > > > > > func#0 @0 > > > FENTRY/FEXIT programs cannot attach to exception callback > > > processed 0 insns (limit 1000000) max_states_per_insn 0 total_states 0 > > > peak_states 0 mark_read 0 > > > > Thanks for fixing it while applying. I will send a follow up to > > silence these logs today. > > For some reason, I don't seem to see these when just running > ./test_progs -t exceptions. That's odd. We need to debug the difference. I definitely see them and I don't think my setup has anything special. Would be good for others to confirm. > I am not sure what I'm doing differently when running the selftests. > A bit weird, but anyway, just guessing the cause, do you see them when > you apply this? Yep. The patch fixes it for me. Pls submit officially.
diff --git a/kernel/bpf/helpers.c b/kernel/bpf/helpers.c index 2c8e1ee97b71..7ff2a42f1996 100644 --- a/kernel/bpf/helpers.c +++ b/kernel/bpf/helpers.c @@ -2490,6 +2490,7 @@ __bpf_kfunc void bpf_throw(u64 cookie) */ kasan_unpoison_task_stack_below((void *)ctx.sp); ctx.aux->bpf_exception_cb(cookie, ctx.sp, ctx.bp); + WARN(1, "A call to BPF exception callback should never return\n"); } __diag_pop(); diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c index 0ba32b626320..21e37e46d792 100644 --- a/kernel/bpf/verifier.c +++ b/kernel/bpf/verifier.c @@ -19750,6 +19750,12 @@ int bpf_check_attach_target(struct bpf_verifier_log *log, bpf_log(log, "Subprog %s doesn't exist\n", tname); return -EINVAL; } + if (aux->func && aux->func[subprog]->aux->exception_cb) { + bpf_log(log, + "%s programs cannot attach to exception callback\n", + prog_extension ? "Extension" : "FENTRY/FEXIT"); + return -EINVAL; + } conservative = aux->func_info_aux[subprog].unreliable; if (prog_extension) { if (conservative) { @@ -19762,6 +19768,11 @@ int bpf_check_attach_target(struct bpf_verifier_log *log, "Extension programs should be JITed\n"); return -EINVAL; } + if (aux->func && aux->func[subprog]->aux->exception_cb) { + bpf_log(log, + "Extension programs cannot replace exception callback\n"); + return -EINVAL; + } } if (!tgt_prog->jited) { bpf_log(log, "Can attach to only JITed progs\n");
During testing, it was discovered that extensions to exception callbacks had no checks, upon running a testcase, the kernel ended up running off the end of a program having final call as bpf_throw, and hitting int3 instructions. The reason is that while the default exception callback would have reset the stack frame to return back to the main program's caller, the replacing extension program will simply return back to bpf_throw, which will instead return back to the program and the program will continue execution, now in an undefined state where anything could happen. The way to support extensions to an exception callback would be to mark the BPF_PROG_TYPE_EXT main subprog as an exception_cb, and prevent it from calling bpf_throw. This would make the JIT produce a prologue that restores saved registers and reset the stack frame. But let's not do that until there is a concrete use case for this, and simply disallow this for now. Similar issues will exist for fentry and fexit cases, where trampoline saves data on the stack when invoking exception callback, which however will then end up resetting the stack frame, and on return, the fexit program will never will invoked as the return address points to the main program's caller in the kernel. Instead of additional complexity and back and forth between the two stacks to enable such a use case, simply forbid it. One key point here to note is that currently X86_TAIL_CALL_OFFSET didn't require any modifications, even though we emit instructions before the corresponding endbr64 instruction. This is because we ensure that a main subprog never serves as an exception callback, and therefore the exception callback (which will be a global subprog) can never serve as the tail call target, eliminating any discrepancies. However, once we support a BPF_PROG_TYPE_EXT to also act as an exception callback, it will end up requiring change to the tail call offset to account for the extra instructions. For simplicitly, tail calls could be disabled for such targets. Noting the above, it appears better to wait for a concrete use case before choosing to permit extension programs to replace exception callbacks. As a precaution, we disable fentry and fexit for exception callbacks as well. Signed-off-by: Kumar Kartikeya Dwivedi <memxor@gmail.com> --- kernel/bpf/helpers.c | 1 + kernel/bpf/verifier.c | 11 +++++++++++ 2 files changed, 12 insertions(+)