* [PATCH bpf v1 1/2] bpf: sync_linked_regs() must preserve subreg_def
@ 2024-09-24 21:08 Eduard Zingerman
2024-09-24 21:08 ` [PATCH bpf v1 2/2] selftests/bpf: verify that sync_linked_regs preserves subreg_def Eduard Zingerman
` (2 more replies)
0 siblings, 3 replies; 6+ messages in thread
From: Eduard Zingerman @ 2024-09-24 21:08 UTC (permalink / raw)
To: bpf, ast
Cc: andrii, daniel, martin.lau, kernel-team, yonghong.song,
Eduard Zingerman, Lonial Con
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 <kongln9170@gmail.com>
Closes: https://lore.kernel.org/bpf/7e2aa30a62d740db182c170fdd8f81c596df280d.camel@gmail.com/
Signed-off-by: Lonial Con <kongln9170@gmail.com>
Signed-off-by: Eduard Zingerman <eddyz87@gmail.com>
---
kernel/bpf/verifier.c | 5 +++++
1 file changed, 5 insertions(+)
diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index dd86282ccaa4..1aa0c6360a55 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -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);
--
2.46.0
^ permalink raw reply related [flat|nested] 6+ messages in thread* [PATCH bpf v1 2/2] selftests/bpf: verify that sync_linked_regs preserves subreg_def
2024-09-24 21:08 [PATCH bpf v1 1/2] bpf: sync_linked_regs() must preserve subreg_def Eduard Zingerman
@ 2024-09-24 21:08 ` Eduard Zingerman
2024-09-25 9:44 ` [PATCH bpf v1 1/2] bpf: sync_linked_regs() must preserve subreg_def Daniel Borkmann
2024-09-27 23:00 ` patchwork-bot+netdevbpf
2 siblings, 0 replies; 6+ messages in thread
From: Eduard Zingerman @ 2024-09-24 21:08 UTC (permalink / raw)
To: bpf, ast
Cc: andrii, daniel, martin.lau, kernel-team, yonghong.song,
Eduard Zingerman
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 <eddyz87@gmail.com>
---
.../selftests/bpf/progs/verifier_scalar_ids.c | 67 +++++++++++++++++++
1 file changed, 67 insertions(+)
diff --git a/tools/testing/selftests/bpf/progs/verifier_scalar_ids.c b/tools/testing/selftests/bpf/progs/verifier_scalar_ids.c
index 2ecf77b623e0..7c5e5e6d10eb 100644
--- a/tools/testing/selftests/bpf/progs/verifier_scalar_ids.c
+++ b/tools/testing/selftests/bpf/progs/verifier_scalar_ids.c
@@ -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";
--
2.46.0
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH bpf v1 1/2] bpf: sync_linked_regs() must preserve subreg_def
2024-09-24 21:08 [PATCH bpf v1 1/2] bpf: sync_linked_regs() must preserve subreg_def Eduard Zingerman
2024-09-24 21:08 ` [PATCH bpf v1 2/2] selftests/bpf: verify that sync_linked_regs preserves subreg_def Eduard Zingerman
@ 2024-09-25 9:44 ` Daniel Borkmann
2024-09-25 19:48 ` Eduard Zingerman
2024-09-27 23:00 ` patchwork-bot+netdevbpf
2 siblings, 1 reply; 6+ messages in thread
From: Daniel Borkmann @ 2024-09-25 9:44 UTC (permalink / raw)
To: Eduard Zingerman, bpf, ast
Cc: andrii, martin.lau, kernel-team, yonghong.song, Lonial Con
On 9/24/24 11:08 PM, Eduard Zingerman 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
>
> (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 <kongln9170@gmail.com>
> Closes: https://lore.kernel.org/bpf/7e2aa30a62d740db182c170fdd8f81c596df280d.camel@gmail.com/
> Signed-off-by: Lonial Con <kongln9170@gmail.com>
> Signed-off-by: Eduard Zingerman <eddyz87@gmail.com>
Do we have a Fixes tag for stable?
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH bpf v1 1/2] bpf: sync_linked_regs() must preserve subreg_def
2024-09-25 9:44 ` [PATCH bpf v1 1/2] bpf: sync_linked_regs() must preserve subreg_def Daniel Borkmann
@ 2024-09-25 19:48 ` Eduard Zingerman
2024-09-25 20:17 ` Daniel Borkmann
0 siblings, 1 reply; 6+ messages in thread
From: Eduard Zingerman @ 2024-09-25 19:48 UTC (permalink / raw)
To: Daniel Borkmann, bpf, ast
Cc: andrii, martin.lau, kernel-team, yonghong.song, Lonial Con
On Wed, 2024-09-25 at 11:44 +0200, Daniel Borkmann wrote:
[...]
> 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.
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH bpf v1 1/2] bpf: sync_linked_regs() must preserve subreg_def
2024-09-25 19:48 ` Eduard Zingerman
@ 2024-09-25 20:17 ` Daniel Borkmann
0 siblings, 0 replies; 6+ messages in thread
From: Daniel Borkmann @ 2024-09-25 20:17 UTC (permalink / raw)
To: Eduard Zingerman, bpf, ast
Cc: andrii, martin.lau, kernel-team, yonghong.song, Lonial Con
On 9/25/24 9:48 PM, Eduard Zingerman wrote:
> On Wed, 2024-09-25 at 11:44 +0200, Daniel Borkmann wrote:
>
> [...]
>
>> 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.
No need, thanks, this can easily be added while applying.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH bpf v1 1/2] bpf: sync_linked_regs() must preserve subreg_def
2024-09-24 21:08 [PATCH bpf v1 1/2] bpf: sync_linked_regs() must preserve subreg_def Eduard Zingerman
2024-09-24 21:08 ` [PATCH bpf v1 2/2] selftests/bpf: verify that sync_linked_regs preserves subreg_def Eduard Zingerman
2024-09-25 9:44 ` [PATCH bpf v1 1/2] bpf: sync_linked_regs() must preserve subreg_def Daniel Borkmann
@ 2024-09-27 23:00 ` patchwork-bot+netdevbpf
2 siblings, 0 replies; 6+ messages in thread
From: patchwork-bot+netdevbpf @ 2024-09-27 23:00 UTC (permalink / raw)
To: Eduard Zingerman
Cc: bpf, ast, andrii, daniel, martin.lau, kernel-team, yonghong.song,
kongln9170
Hello:
This series was applied to bpf/bpf.git (master)
by Andrii Nakryiko <andrii@kernel.org>:
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
>
> [...]
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!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2024-09-27 23:00 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-09-24 21:08 [PATCH bpf v1 1/2] bpf: sync_linked_regs() must preserve subreg_def Eduard Zingerman
2024-09-24 21:08 ` [PATCH bpf v1 2/2] selftests/bpf: verify that sync_linked_regs preserves subreg_def Eduard Zingerman
2024-09-25 9:44 ` [PATCH bpf v1 1/2] bpf: sync_linked_regs() must preserve subreg_def Daniel Borkmann
2024-09-25 19:48 ` Eduard Zingerman
2024-09-25 20:17 ` Daniel Borkmann
2024-09-27 23:00 ` patchwork-bot+netdevbpf
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox