[bpf,v2,0/2] Don't trust r0 bounds after BPF to BPF calls with abnormal returns

Message

Arthur Fabre Dec. 13, 2024, 9:27 p.m. UTC

A BPF function can return before its exit instruction: LD_ABS, LD_IND,
and tail_call() can all cause it to return prematurely.

When such a function is called by another BPF function, the verifier
doesn't take this into account when calculating the bounds of r0 in the
caller after the callee returns.

---
Changes in v2:
- Handle LD_ABS and LD_IND, not just tail_call()
- Split tests out
- Use inline asm for tests

---
Arthur Fabre (2):
  bpf: Don't trust r0 bounds after BPF to BPF calls with abnormal
    returns
  selftests/bpf: Test r0 bounds after BPF to BPF call with abnormal
    return

 kernel/bpf/verifier.c                         | 18 ++--
 .../selftests/bpf/prog_tests/verifier.c       |  2 +
 .../bpf/progs/verifier_abnormal_ret.c         | 88 +++++++++++++++++++
 3 files changed, 102 insertions(+), 6 deletions(-)
 create mode 100644 tools/testing/selftests/bpf/progs/verifier_abnormal_ret.c

Comments

Arthur Fabre Dec. 16, 2024, 5:45 p.m. UTC | #1

On Fri Dec 13, 2024 at 10:27 PM CET, Arthur Fabre wrote:
> A BPF function can return before its exit instruction: LD_ABS, LD_IND,
> and tail_call() can all cause it to return prematurely.
>
> When such a function is called by another BPF function, the verifier
> doesn't take this into account when calculating the bounds of r0 in the
> caller after the callee returns.

I've just realized r0 isn't he only problem: a caller can pass a
reference to its stack to a callee, and the verifier also tracks the
value of this.

If the callee writes to the caller's stack after the abnormal return
(tail_call, ld_abs), the verifier will also incorrectly assume the 
write always happens.

I think we need to treat these abnormal returns as a branch that can
exit. That way the verifier will explore both possibilities, and the
combined result will really reflect what can happen.
I'll try that out for a v3.

Alexei Starovoitov Dec. 16, 2024, 6:03 p.m. UTC | #2

On Mon, Dec 16, 2024 at 9:45 AM Arthur Fabre <[e-mail]> wrote:
>
> On Fri Dec 13, 2024 at 10:27 PM CET, Arthur Fabre wrote:
> > A BPF function can return before its exit instruction: LD_ABS, LD_IND,
> > and tail_call() can all cause it to return prematurely.
> >
> > When such a function is called by another BPF function, the verifier
> > doesn't take this into account when calculating the bounds of r0 in the
> > caller after the callee returns.
>
> I've just realized r0 isn't he only problem: a caller can pass a
> reference to its stack to a callee, and the verifier also tracks the
> value of this.
>
> If the callee writes to the caller's stack after the abnormal return
> (tail_call, ld_abs), the verifier will also incorrectly assume the
> write always happens.
>
> I think we need to treat these abnormal returns as a branch that can
> exit. That way the verifier will explore both possibilities, and the
> combined result will really reflect what can happen.
> I'll try that out for a v3.

Good idea. Makes sense to me.