[bpf,v1,1/2] bpf: sync_linked_regs() must preserve subreg_def

Commit Message

Eduard Zingerman Sept. 24, 2024, 9:08 p.m. UTC

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 <[e-mail]>
Closes: https://lore.kernel.org/bpf/[e-mail]/
Signed-off-by: Lonial Con <[e-mail]>
Signed-off-by: Eduard Zingerman <[e-mail]>
---
 kernel/bpf/verifier.c | 5 +++++
 1 file changed, 5 insertions(+)

Comments

Daniel Borkmann Sept. 25, 2024, 9:44 a.m. UTC | #1

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 <[e-mail]>
> Closes: https://lore.kernel.org/bpf/[e-mail]/
> Signed-off-by: Lonial Con <[e-mail]>
> Signed-off-by: Eduard Zingerman <[e-mail]>
Do we have a Fixes tag for stable?

Eduard Zingerman Sept. 25, 2024, 7:48 p.m. UTC | #2

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.

Daniel Borkmann Sept. 25, 2024, 8:17 p.m. UTC | #3

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.

[e-mail] Sept. 27, 2024, 11 p.m. UTC | #4

Hello:

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

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!

Patch

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);