[bpf,v4,0/3] bpf: fix accesses to uninit stack slots

Message

Andrei Matei Dec. 8, 2023, 2:31 a.m. UTC

Fix two related issues issues around verifying stack accesses:
1. accesses to uninitialized stack memory was allowed inconsistently
2. the maximum stack depth needed for a program was not always
maintained correctly

The two issues are fixed together in one commit because the code for one
affects the other.

V3 to V4:
- minor fixup to comment in patch 1 (Eduard)
- C89-style in patch 3 (Andrii)

V2 to V3:
- address review comments from Andrii and Eduard
- drop new verifier tests in favor of editing existing tests to check
  for stack depth
- append a patch with a bit of cleanup coming out of the previous review


Andrei Matei (3):
  bpf: add some comments to stack representation
  bpf: fix accesses to uninit stack slots
  bpf: minor cleanup around stack bounds

 include/linux/bpf_verifier.h                  | 14 ++++
 kernel/bpf/verifier.c                         | 76 +++++++++----------
 tools/testing/selftests/bpf/progs/iters.c     |  2 +-
 .../selftests/bpf/progs/test_global_func16.c  |  2 +-
 .../bpf/progs/verifier_basic_stack.c          |  8 +-
 .../selftests/bpf/progs/verifier_int_ptr.c    |  5 +-
 .../selftests/bpf/progs/verifier_raw_stack.c  |  5 +-
 .../selftests/bpf/progs/verifier_var_off.c    | 62 ++++++++++++---
 .../selftests/bpf/verifier/atomic_cmpxchg.c   | 11 ---
 tools/testing/selftests/bpf/verifier/calls.c  |  4 +-
 10 files changed, 115 insertions(+), 74 deletions(-)

Comments

Andrei Matei Dec. 8, 2023, 2:45 a.m. UTC | #1

[...]

Some decorum questions from a newbie:

I'm not sure if this should go to bpf or bpf-next (or perhaps if only the 2nd
patch here should go to bpf and the rest to bpf-next). If anyone has opinions,
I'm happy to re-send or to let whoever applies the patches to choose at
application time. Patch 2 contains a couple of fixes, and there was talk of
backporting... although I'm not sure how bad the bug(s) were in practice.
Patches 1 and 3 don't fix anything, but I'm not sure if it's worth sending them
separately. The first patch doesn't apply cleanly to bpf-next because some
fields were reordered there, but it should be trivial to fixup.

Btw, I also don't know how backporting works, so if I should do anything about
it, please let me know.

The main patch (patch 2) adds some new testing conditions to some existing
tests, and also technically adds a new test (but it's really a subset of an
existing test; the comments there explain). I know that we like to generally
split tests to separate patches (for reasons I'm not sure about), but I don't
know how far to take that rule; not sure if I should apply it here. Will take
guidance.

Thanks!

Alexei Starovoitov Dec. 8, 2023, 2:50 a.m. UTC | #2

On Thu, Dec 7, 2023 at 6:45 PM Andrei Matei <[e-mail]> wrote:
>
> [...]
>
> Some decorum questions from a newbie:
>
> I'm not sure if this should go to bpf or bpf-next (or perhaps if only the 2nd
> patch here should go to bpf and the rest to bpf-next). If anyone has opinions,

bpf-next please. The changes are too tricky to expose the world immediately.

Andrei Matei Dec. 8, 2023, 3:01 a.m. UTC | #3

On Thu, Dec 7, 2023 at 9:50 PM Alexei Starovoitov
<[e-mail]> wrote:
>
> On Thu, Dec 7, 2023 at 6:45 PM Andrei Matei <[e-mail]> wrote:
> >
> > [...]
> >
> > Some decorum questions from a newbie:
> >
> > I'm not sure if this should go to bpf or bpf-next (or perhaps if only the 2nd
> > patch here should go to bpf and the rest to bpf-next). If anyone has opinions,
>
> bpf-next please. The changes are too tricky to expose the world immediately.

Ack. Resending.