On Wed, Aug 09, 2023 at 05:11:12PM +0530, 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.
>
> 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.
>
> Signed-off-by: Kumar Kartikeya Dwivedi <memxor@xxxxxxxxx>
> ---
> kernel/bpf/helpers.c | 1 +
> kernel/bpf/verifier.c | 5 +++++
> 2 files changed, 6 insertions(+)
>
> diff --git a/kernel/bpf/helpers.c b/kernel/bpf/helpers.c
> index 64a07232c58f..a04eff53354c 100644
> --- a/kernel/bpf/helpers.c
> +++ b/kernel/bpf/helpers.c
> @@ -2470,6 +2470,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 a0e1a1d1f5d3..13db1fa4163c 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -19622,6 +19622,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;
Should we disallow fentry/fexit to exception cb as well?
Probably things will go wrong for similar reasons as freplace.
And also disallow fentry/fexit for main prog that is exception_boundary ?
since bpf trampoline doesn't know that it needs to save r12.
