[PATCH bpf v1 0/2] Fix incorrect pruning for ARG_CONST_ALLOC_SIZE_OR_ZERO

[PATCH bpf v1 0/2] Fix incorrect pruning for ARG_CONST_ALLOC_SIZE_OR_ZERO

[Kumar Kartikeya Dwivedi]

A fix for a missing mark_chain_precision call that leads to eager pruning and
loading of invalid programs when the more permissive case is in the straight
line exploration. Please see the commit log for details, and selftest for an
example.

Kumar Kartikeya Dwivedi (2):
  bpf: Do mark_chain_precision for ARG_CONST_ALLOC_SIZE_OR_ZERO
  selftests/bpf: Add regression test for pruning fix

 kernel/bpf/verifier.c                         |  3 +++
 .../testing/selftests/bpf/verifier/precise.c  | 25 +++++++++++++++++++
 2 files changed, 28 insertions(+)

-- 
2.34.1

[PATCH bpf v1 1/2] bpf: Do mark_chain_precision for ARG_CONST_ALLOC_SIZE_OR_ZERO

[Kumar Kartikeya Dwivedi]

Precision markers need to be propagated whenever we have an ARG_CONST_*
style argument, as the verifier cannot consider imprecise scalars to be
equivalent for the purposes of states_equal check when such arguments
refine the return value (in this case, set mem_size for PTR_TO_MEM). The
resultant mem_size for the R0 is derived from the constant value, and if
the verifier incorrectly prunes states considering them equivalent where
such arguments exist (by seeing that both registers have reg->precise as
false in regsafe), we can end up with invalid programs passing the
verifier which can do access beyond what should have been the correct
mem_size in that explored state.

To show a concrete example of the problem:

0000000000000000 <prog>:
       0:       r2 = *(u32 *)(r1 + 80)
       1:       r1 = *(u32 *)(r1 + 76)
       2:       r3 = r1
       3:       r3 += 4
       4:       if r3 > r2 goto +18 <LBB5_5>
       5:       w2 = 0
       6:       *(u32 *)(r1 + 0) = r2
       7:       r1 = *(u32 *)(r1 + 0)
       8:       r2 = 1
       9:       if w1 == 0 goto +1 <LBB5_3>
      10:       r2 = -1

0000000000000058 <LBB5_3>:
      11:       r1 = 0 ll
      13:       r3 = 0
      14:       call bpf_ringbuf_reserve
      15:       if r0 == 0 goto +7 <LBB5_5>
      16:       r1 = r0
      17:       r1 += 16777215
      18:       w2 = 0
      19:       *(u8 *)(r1 + 0) = r2
      20:       r1 = r0
      21:       r2 = 0
      22:       call bpf_ringbuf_submit

00000000000000b8 <LBB5_5>:
      23:       w0 = 0
      24:       exit

For the first case, the single line execution's exploration will prune
the search at insn 14 for the branch insn 9's second leg as it will be
verified first using r2 = -1 (UINT_MAX), while as w1 at insn 9 will
always be 0 so at runtime we don't get error for being greater than
UINT_MAX/4 from bpf_ringbuf_reserve. The verifier during regsafe just
sees reg->precise as false for both r2 registers in both states, hence
considers them equal for purposes of states_equal.

If we propagated precise markers using the backtracking support, we
would use the precise marking to then ensure that old r2 (UINT_MAX) was
within the new r2 (1) and this would never be true, so the verification
would rightfully fail.

The end result is that the out of bounds access at instruction 19 would
be permitted without this fix.

Note that reg->precise is always set to true when user does not have
CAP_BPF (or when subprog count is greater than 1 (i.e. use of any static
or global functions)), hence this is only a problem when precision marks
need to be explicitly propagated (i.e. privileged users with CAP_BPF).

A simplified test case has been included in the next patch to prevent
future regressions.

Fixes: 457f44363a88 ("bpf: Implement BPF ring buffer and verifier support for it")
Signed-off-by: Kumar Kartikeya Dwivedi <memxor@gmail.com>
---
 kernel/bpf/verifier.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index 096fdac70165..30c6eebce146 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -6066,6 +6066,9 @@ static int check_func_arg(struct bpf_verifier_env *env, u32 arg,
 			return -EACCES;
 		}
 		meta->mem_size = reg->var_off.value;
+		err = mark_chain_precision(env, regno);
+		if (err)
+			return err;
 		break;
 	case ARG_PTR_TO_INT:
 	case ARG_PTR_TO_LONG:
-- 
2.34.1

[PATCH bpf v1 2/2] selftests/bpf: Add regression test for pruning fix

[Kumar Kartikeya Dwivedi]

Add a test to ensure we do mark_chain_precision for the argument type
ARG_CONST_ALLOC_SIZE_OR_ZERO. For other argument types, this was already
done, but propagation for missing for this case. Without the fix, this
test case loads successfully.

Signed-off-by: Kumar Kartikeya Dwivedi <memxor@gmail.com>
---
 .../testing/selftests/bpf/verifier/precise.c  | 25 +++++++++++++++++++
 1 file changed, 25 insertions(+)

diff --git a/tools/testing/selftests/bpf/verifier/precise.c b/tools/testing/selftests/bpf/verifier/precise.c
index 9e754423fa8b..6c03a7d805f9 100644
--- a/tools/testing/selftests/bpf/verifier/precise.c
+++ b/tools/testing/selftests/bpf/verifier/precise.c
@@ -192,3 +192,28 @@
 	.result = VERBOSE_ACCEPT,
 	.retval = -1,
 },
+{
+	"precise: mark_chain_precision for ARG_CONST_ALLOC_SIZE_OR_ZERO",
+	.insns = {
+	BPF_LDX_MEM(BPF_W, BPF_REG_4, BPF_REG_1, offsetof(struct xdp_md, ingress_ifindex)),
+	BPF_LD_MAP_FD(BPF_REG_6, 0),
+	BPF_MOV64_REG(BPF_REG_1, BPF_REG_6),
+	BPF_MOV64_IMM(BPF_REG_2, 1),
+	BPF_MOV64_IMM(BPF_REG_3, 0),
+	BPF_JMP_IMM(BPF_JEQ, BPF_REG_4, 0, 1),
+	BPF_MOV64_IMM(BPF_REG_2, 0x1000),
+	BPF_RAW_INSN(BPF_JMP | BPF_CALL, 0, 0, 0, BPF_FUNC_ringbuf_reserve),
+	BPF_JMP_IMM(BPF_JNE, BPF_REG_0, 0, 1),
+	BPF_EXIT_INSN(),
+	BPF_MOV64_REG(BPF_REG_1, BPF_REG_0),
+	BPF_LDX_MEM(BPF_DW, BPF_REG_2, BPF_REG_0, 42),
+	BPF_RAW_INSN(BPF_JMP | BPF_CALL, 0, 0, 0, BPF_FUNC_ringbuf_submit),
+	BPF_MOV64_IMM(BPF_REG_0, 0),
+	BPF_EXIT_INSN(),
+	},
+	.fixup_map_ringbuf = { 1 },
+	.prog_type = BPF_PROG_TYPE_XDP,
+	.flags = BPF_F_TEST_STATE_FREQ,
+	.errstr = "invalid access to memory, mem_size=1 off=42 size=8",
+	.result = REJECT,
+},
-- 
2.34.1

Re: [PATCH bpf v1 0/2] Fix incorrect pruning for ARG_CONST_ALLOC_SIZE_OR_ZERO

[Andrii Nakryiko]

On Tue, Aug 23, 2022 at 11:53 AM Kumar Kartikeya Dwivedi
<memxor@gmail.com> wrote:
>
> A fix for a missing mark_chain_precision call that leads to eager pruning and
> loading of invalid programs when the more permissive case is in the straight
> line exploration. Please see the commit log for details, and selftest for an
> example.
>
> Kumar Kartikeya Dwivedi (2):
>   bpf: Do mark_chain_precision for ARG_CONST_ALLOC_SIZE_OR_ZERO
>   selftests/bpf: Add regression test for pruning fix
>
>  kernel/bpf/verifier.c                         |  3 +++
>  .../testing/selftests/bpf/verifier/precise.c  | 25 +++++++++++++++++++
>  2 files changed, 28 insertions(+)
>
> --
> 2.34.1
>


Makes sense and LGTM. Thanks!

Acked-by: Andrii Nakryiko <andrii@kernel.org>

Re: [PATCH bpf v1 0/2] Fix incorrect pruning for ARG_CONST_ALLOC_SIZE_OR_ZERO

[patchwork-bot+netdevbpf]

Hello:

This series was applied to bpf/bpf.git (master)
by Alexei Starovoitov <ast@kernel.org>:

On Tue, 23 Aug 2022 20:52:58 +0200 you wrote:
> A fix for a missing mark_chain_precision call that leads to eager pruning and
> loading of invalid programs when the more permissive case is in the straight
> line exploration. Please see the commit log for details, and selftest for an
> example.
> 
> Kumar Kartikeya Dwivedi (2):
>   bpf: Do mark_chain_precision for ARG_CONST_ALLOC_SIZE_OR_ZERO
>   selftests/bpf: Add regression test for pruning fix
> 
> [...]

Here is the summary with links:
  - [bpf,v1,1/2] bpf: Do mark_chain_precision for ARG_CONST_ALLOC_SIZE_OR_ZERO
    https://git.kernel.org/bpf/bpf/c/2fc31465c537
  - [bpf,v1,2/2] selftests/bpf: Add regression test for pruning fix
    https://git.kernel.org/bpf/bpf/c/1800b2ac96d8

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html