Skip to contentSkip to conversation

bpf: sync_linked_regs() must preserve subreg_def

PATCH Applied Eduard Zingerman 3 participants 6 messages Sep 24, 2024 – Sep 27, 2024

Subsystems (4)
Kselftest BPF BPF selftests BPF core
lore
Share & tools
lore

Eduard Zingerman

author maintainer
#0 lore raw

PATCH 1/2 bpf: sync_linked_regs() must preserve subreg_def

Range propagation must not affect subreg_def marks, otherwise the following example is rewritten by verifier incorrectly when BPF_F_TEST_RND_HI32 flag is set:

  0: call bpf_ktime_get_ns                   call bpf_ktime_get_ns
  1: r0 &= 0x7fffffff       after verifier   r0 &= 0x7fffffff
  2: w1 = w0                rewrites         w1 = w0
  3: if w0 < 10 goto +0     -------------->  r11 = 0x2f5674a6     (r)
  4: r1 >>= 32                               r11 <<= 32           (r)
  5: r0 = r1                                 r1 |= r11            (r)
  6: exit;                                   if w0 < 0xa goto pc+0
                                             r1 >>= 32
                                             r0 = r1
                                             exit

(or zero extension of w1 at (2) is missing for architectures that
require zero extension for upper register half).

The following happens w/o this patch:
- r0 is marked as not a subreg at (0);
- w1 is marked as subreg at (2);
- w1 subreg_def is overridden at (3) by copy_register_state();
- w1 is read at (5) but mark_insn_zext() does not mark (2) for zero extension, because w1 subreg_def is not set;
- because of BPF_F_TEST_RND_HI32 flag verifier inserts random value for hi32 bits of (2) (marked (r));
- this random value is read at (5).

Reported-by Lonial Con <[email protected]>
Closes https://lore.kernel.org/bpf/[email protected]/ open here
Signed-off-by Lonial Con <[email protected]>
Signed-off-by Eduard Zingerman <[email protected]>
---
 kernel/bpf/verifier.c | 5 +++++
 1 file changed, 5 insertions(+)
kernel/bpf/verifier.c +5 -0
index dd86282ccaa4..1aa0c6360a55 100644
@@ -15326,8 +15326,12 @@ static void sync_linked_regs(struct bpf_verifier_state *vstate, struct bpf_reg_s
 			continue;
 		if ((!(reg->id & BPF_ADD_CONST) && !(known_reg->id & BPF_ADD_CONST)) ||
 		    reg->off == known_reg->off) {
+			s32 saved_subreg_def = reg->subreg_def;
+
 			copy_register_state(reg, known_reg);
+			reg->subreg_def = saved_subreg_def;
 		} else {
+			s32 saved_subreg_def = reg->subreg_def;
 			s32 saved_off = reg->off;
 
 			fake_reg.type = SCALAR_VALUE;

@@ -15340,6 +15344,7 @@ static void sync_linked_regs(struct bpf_verifier_state *vstate, struct bpf_reg_s
 			 * otherwise another sync_linked_regs() will be incorrect.
 			 */
 			reg->off = saved_off;
+			reg->subreg_def = saved_subreg_def;
 
 			scalar32_min_max_add(reg, &fake_reg);
 			scalar_min_max_add(reg, &fake_reg);
Signature · 1 line
2.46.0
author maintainer
#1 lore raw

PATCH 2/2 selftests/bpf: verify that sync_linked_regs preserves subreg_def

This test was added because of a bug in verifier.c:sync_linked_regs(), upon range propagation it destroyed subreg_def marks for registers. The test is written in a way to return an upper half of a register that is affected by range propagation and must have it's subreg_def preserved. This gives a return value of 0 and leads to undefined return value if subreg_def mark is not preserved.

Signed-off-by Eduard Zingerman <[email protected]>
---
 .../selftests/bpf/progs/verifier_scalar_ids.c | 67 +++++++++++++++++++
 1 file changed, 67 insertions(+)
tools/testing/selftests/bpf/progs/verifier_scalar_ids.c +67 -0
index 2ecf77b623e0..7c5e5e6d10eb 100644
@@ -760,4 +760,71 @@ __naked void two_old_ids_one_cur_id(void)
 	: __clobber_all);
 }
 
+SEC("socket")
+/* Note the flag, see verifier.c:opt_subreg_zext_lo32_rnd_hi32() */
+__flag(BPF_F_TEST_RND_HI32)
+__success
+/* This test was added because of a bug in verifier.c:sync_linked_regs(),
+ * upon range propagation it destroyed subreg_def marks for registers.
+ * The subreg_def mark is used to decide whether zero extension instructions
+ * are needed when register is read. When BPF_F_TEST_RND_HI32 is set it
+ * also causes generation of statements to randomize upper halves of
+ * read registers.
+ *
+ * The test is written in a way to return an upper half of a register
+ * that is affected by range propagation and must have it's subreg_def
+ * preserved. This gives a return value of 0 and leads to undefined
+ * return value if subreg_def mark is not preserved.
+ */
+__retval(0)
+/* Check that verifier believes r1/r0 are zero at exit */
+__log_level(2)
+__msg("4: (77) r1 >>= 32                     ; R1_w=0")
+__msg("5: (bf) r0 = r1                       ; R0_w=0 R1_w=0")
+__msg("6: (95) exit")
+__msg("from 3 to 4")
+__msg("4: (77) r1 >>= 32                     ; R1_w=0")
+__msg("5: (bf) r0 = r1                       ; R0_w=0 R1_w=0")
+__msg("6: (95) exit")
+/* Verify that statements to randomize upper half of r1 had not been
+ * generated.
+ */
+__xlated("call unknown")
+__xlated("r0 &= 2147483647")
+__xlated("w1 = w0")
+/* This is how disasm.c prints BPF_ZEXT_REG at the moment, x86 and arm
+ * are the only CI archs that do not need zero extension for subregs.
+ */
+#if !defined(__TARGET_ARCH_x86) && !defined(__TARGET_ARCH_arm64)
+__xlated("w1 = w1")
+#endif
+__xlated("if w0 < 0xa goto pc+0")
+__xlated("r1 >>= 32")
+__xlated("r0 = r1")
+__xlated("exit")
+__naked void linked_regs_and_subreg_def(void)
+{
+	asm volatile (
+	"call %[bpf_ktime_get_ns];"
+	/* make sure r0 is in 32-bit range, otherwise w1 = w0 won't
+	 * assign same IDs to registers.
+	 */
+	"r0 &= 0x7fffffff;"
+	/* link w1 and w0 via ID */
+	"w1 = w0;"
+	/* 'if' statement propagates range info from w0 to w1,
+	 * but should not affect w1->subreg_def property.
+	 */
+	"if w0 < 10 goto +0;"
+	/* r1 is read here, on archs that require subreg zero
+	 * extension this would cause zext patch generation.
+	 */
+	"r1 >>= 32;"
+	"r0 = r1;"
+	"exit;"
+	:
+	: __imm(bpf_ktime_get_ns)
+	: __clobber_all);
+}
+
 char _license[] SEC("license") = "GPL";
Signature · 1 line
2.46.0
maintainer
#2 lore raw
In reply to Eduard Zingerman
Quoted 28 lines · Eduard Zingerman

Range propagation must not affect subreg_def marks, otherwise the following example is rewritten by verifier incorrectly when BPF_F_TEST_RND_HI32 flag is set:

   0: call bpf_ktime_get_ns                   call bpf_ktime_get_ns
   1: r0 &= 0x7fffffff       after verifier   r0 &= 0x7fffffff
   2: w1 = w0                rewrites         w1 = w0
   3: if w0 < 10 goto +0     -------------->  r11 = 0x2f5674a6     (r)
   4: r1 >>= 32                               r11 <<= 32           (r)
   5: r0 = r1                                 r1 |= r11            (r)
   6: exit;                                   if w0 < 0xa goto pc+0
                                              r1 >>= 32
                                              r0 = r1
                                              exit

(or zero extension of w1 at (2) is missing for architectures that
require zero extension for upper register half).

The following happens w/o this patch:
- r0 is marked as not a subreg at (0);
- w1 is marked as subreg at (2);
- w1 subreg_def is overridden at (3) by copy_register_state();
- w1 is read at (5) but mark_insn_zext() does not mark (2) for zero extension, because w1 subreg_def is not set;
- because of BPF_F_TEST_RND_HI32 flag verifier inserts random value for hi32 bits of (2) (marked (r));
- this random value is read at (5).

Quoted 4 lines
Reported-by Lonial Con <[email protected]>
Closes https://lore.kernel.org/bpf/[email protected]/ open here
Signed-off-by Lonial Con <[email protected]>
Signed-off-by Eduard Zingerman <[email protected]>

Do we have a Fixes tag for stable?

author maintainer
#3 lore raw

On Wed, 2024-09-25 at 11:44 +0200, Daniel Borkmann wrote:

[...]

Quoted 1 line

Do we have a Fixes tag for stable?

I think this bug persisted from the beginning:
75748837b7e5 ("bpf: Propagate scalar ranges through register assignments.")

E.g. here is original find_equal_scalars():
static void find_equal_scalars(struct bpf_verifier_state *vstate,
struct bpf_reg_state *known_reg)

{
	...
	struct bpf_reg_state *reg;
	...
				*reg = *known_reg;
	...
}

And bpf_reg_state for 75748837b7e5 has subreg_def as a member.

I can post v2 with this "Fixes" tag if you'd like.

maintainer
#4 lore raw
Quoted 19 lines · Eduard Zingerman

On Wed, 2024-09-25 at 11:44 +0200, Daniel Borkmann wrote:

[...]

Quoted 1 line

Do we have a Fixes tag for stable?

I think this bug persisted from the beginning:
75748837b7e5 ("bpf: Propagate scalar ranges through register assignments.")

E.g. here is original find_equal_scalars():
static void find_equal_scalars(struct bpf_verifier_state *vstate,
struct bpf_reg_state *known_reg)

{
	...
	struct bpf_reg_state *reg;
	...
				*reg = *known_reg;
	...
}
Quoted 3 lines

And bpf_reg_state for 75748837b7e5 has subreg_def as a member.

I can post v2 with this "Fixes" tag if you'd like.

No need, thanks, this can easily be added while applying.

CI Applied
#5 lore raw
In reply to Eduard Zingerman
This series was applied to bpf/bpf.git (master) by Andrii Nakryiko <[email protected]>:

Hello:

This series was applied to bpf/bpf.git (master)
by Andrii Nakryiko <[email protected]>:

Quoted 15 lines · On Tue, 24 Sep 2024 14:08:43 -0700 you wrote:

Range propagation must not affect subreg_def marks, otherwise the following example is rewritten by verifier incorrectly when BPF_F_TEST_RND_HI32 flag is set:

  0: call bpf_ktime_get_ns                   call bpf_ktime_get_ns
  1: r0 &= 0x7fffffff       after verifier   r0 &= 0x7fffffff
  2: w1 = w0                rewrites         w1 = w0
  3: if w0 < 10 goto +0     -------------->  r11 = 0x2f5674a6     (r)
  4: r1 >>= 32                               r11 <<= 32           (r)
  5: r0 = r1                                 r1 |= r11            (r)
  6: exit;                                   if w0 < 0xa goto pc+0
                                             r1 >>= 32
                                             r0 = r1
                                             exit
Quoted 1 line

[...]

Here is the summary with links:
- [bpf,v1,1/2] bpf: sync_linked_regs() must preserve subreg_def

    https://git.kernel.org/bpf/bpf/c/27cda47e7819
  - [bpf,v1,2/2] selftests/bpf: verify that sync_linked_regs preserves subreg_def
    https://git.kernel.org/bpf/bpf/c/99a648c951ba

You are awesome, thank you!

Signature · 2 lines
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html