[bpf-next,v2,10/14] bpf: Disallow extensions to exception callbacks

Commit Message

Kumar Kartikeya Dwivedi Aug. 9, 2023, 11:41 a.m. UTC

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 <[e-mail]>
---
 kernel/bpf/helpers.c  | 1 +
 kernel/bpf/verifier.c | 5 +++++
 2 files changed, 6 insertions(+)

Comments

Alexei Starovoitov Aug. 22, 2023, 5:09 a.m. UTC | #1

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 <[e-mail]>
> ---
>  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.

Kumar Kartikeya Dwivedi Aug. 22, 2023, 12:53 p.m. UTC | #2

On Tue, 22 Aug 2023 at 10:40, Alexei Starovoitov
<[e-mail]> wrote:
>
> 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 <[e-mail]>
> > ---
> >  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.
>

Yes, great catch. I think you are right. I will disable both of them as well.
Trampoline does not expect the stack frame to be reset as it pushes
data to it and will need to restore it after the call to exception cb.

> And also disallow fentry/fexit for main prog that is exception_boundary ?
> since bpf trampoline doesn't know that it needs to save r12.

Hmm, I think I should probably enable that instead of blocking it. I
think it's a common enough use case. Compared to exception cb it also
seems a valid one. We can enable pushing of r12 for such generate
trampolines, IIUC it's not a lot of complexity and we will know if we
are attaching to exception boundary prog.

In any case, I will add more selftests to ensure these cases are
handled/rejected properly in v3, thanks!

Patch

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;
+			}
 		}
 		if (!tgt_prog->jited) {
 			bpf_log(log, "Can attach to only JITed progs\n");