[PATCH bpf v2 0/2] bpf: Fix incorrect immediate spill

[PATCH bpf v2 0/2] bpf: Fix incorrect immediate spill

[Hao Sun]

Immediate is incorrectly cast to u32 before being spilled, losing sign
information. The range information is incorrect after load again. Fix
immediate spill by remove the cast. The second patch add a test case
for this.

Signed-off-by: Hao Sun <sunhao.th@gmail.com>
---
Changes in v2:
- Add fix and cc tags.
- Link to v1: https://lore.kernel.org/r/20231026-fix-check-stack-write-v1-0-6b325ef3ce7e@gmail.com

---
Hao Sun (2):
      bpf: Fix check_stack_write_fixed_off() to correctly spill imm
      selftests/bpf: Add test for immediate spilled to stack

 kernel/bpf/verifier.c                             |  2 +-
 tools/testing/selftests/bpf/verifier/bpf_st_mem.c | 32 +++++++++++++++++++++++
 2 files changed, 33 insertions(+), 1 deletion(-)
---
base-commit: f1c73396133cb3d913e2075298005644ee8dfade
change-id: 20231026-fix-check-stack-write-c40996694dfa

Best regards,
-- 
Hao Sun <sunhao.th@gmail.com>

[PATCH bpf v2 1/2] bpf: Fix check_stack_write_fixed_off() to correctly spill imm

[Hao Sun]

In check_stack_write_fixed_off(), imm value is cast to u32 before being
spilled to the stack. Therefore, the sign information is lost, and the
range information is incorrect when load from the stack again.

For the following prog:
0: r2 = r10
1: *(u64*)(r2 -40) = -44
2: r0 = *(u64*)(r2 - 40)
3: if r0 s<= 0xa goto +2
4: r0 = 1
5: exit
6: r0  = 0
7: exit

The verifier gives:
func#0 @0
0: R1=ctx(off=0,imm=0) R10=fp0
0: (bf) r2 = r10                      ; R2_w=fp0 R10=fp0
1: (7a) *(u64 *)(r2 -40) = -44        ; R2_w=fp0 fp-40_w=4294967252
2: (79) r0 = *(u64 *)(r2 -40)         ; R0_w=4294967252 R2_w=fp0
fp-40_w=4294967252
3: (c5) if r0 s< 0xa goto pc+2
mark_precise: frame0: last_idx 3 first_idx 0 subseq_idx -1
mark_precise: frame0: regs=r0 stack= before 2: (79) r0 = *(u64 *)(r2 -40)
3: R0_w=4294967252
4: (b7) r0 = 1                        ; R0_w=1
5: (95) exit
verification time 7971 usec
stack depth 40
processed 6 insns (limit 1000000) max_states_per_insn 0 total_states 0
peak_states 0 mark_read 0

So remove the incorrect cast, since imm field is declared as s32, and
__mark_reg_known() takes u64, so imm would be correctly sign extended
by compiler.

Fixes: ecdf985d7615 ("bpf: track immediate values written to stack by BPF_ST instruction")
Cc: stable@vger.kernel.org
Signed-off-by: Hao Sun <sunhao.th@gmail.com>
---
 kernel/bpf/verifier.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index 857d76694517..44af69ce1301 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -4674,7 +4674,7 @@ static int check_stack_write_fixed_off(struct bpf_verifier_env *env,
 		   insn->imm != 0 && env->bpf_capable) {
 		struct bpf_reg_state fake_reg = {};
 
-		__mark_reg_known(&fake_reg, (u32)insn->imm);
+		__mark_reg_known(&fake_reg, insn->imm);
 		fake_reg.type = SCALAR_VALUE;
 		save_register_state(state, spi, &fake_reg, size);
 	} else if (reg && is_spillable_regtype(reg->type)) {

-- 
2.34.1

Re: [PATCH bpf v2 1/2] bpf: Fix check_stack_write_fixed_off() to correctly spill imm

[Shung-Hsi Yu]

On Wed, Nov 01, 2023 at 08:33:22AM +0100, Hao Sun wrote:
> In check_stack_write_fixed_off(), imm value is cast to u32 before being
> spilled to the stack. Therefore, the sign information is lost, and the
> range information is incorrect when load from the stack again.
> 
> For the following prog:
> 0: r2 = r10
> 1: *(u64*)(r2 -40) = -44
> 2: r0 = *(u64*)(r2 - 40)
> 3: if r0 s<= 0xa goto +2
> 4: r0 = 1
> 5: exit
> 6: r0  = 0
> 7: exit
> 
> The verifier gives:
> func#0 @0
> 0: R1=ctx(off=0,imm=0) R10=fp0
> 0: (bf) r2 = r10                      ; R2_w=fp0 R10=fp0
> 1: (7a) *(u64 *)(r2 -40) = -44        ; R2_w=fp0 fp-40_w=4294967252
> 2: (79) r0 = *(u64 *)(r2 -40)         ; R0_w=4294967252 R2_w=fp0
> fp-40_w=4294967252
> 3: (c5) if r0 s< 0xa goto pc+2
> mark_precise: frame0: last_idx 3 first_idx 0 subseq_idx -1
> mark_precise: frame0: regs=r0 stack= before 2: (79) r0 = *(u64 *)(r2 -40)
> 3: R0_w=4294967252
> 4: (b7) r0 = 1                        ; R0_w=1
> 5: (95) exit
> verification time 7971 usec
> stack depth 40
> processed 6 insns (limit 1000000) max_states_per_insn 0 total_states 0
> peak_states 0 mark_read 0
> 
> So remove the incorrect cast, since imm field is declared as s32, and
> __mark_reg_known() takes u64, so imm would be correctly sign extended
> by compiler.
> 
> Fixes: ecdf985d7615 ("bpf: track immediate values written to stack by BPF_ST instruction")
> Cc: stable@vger.kernel.org
> Signed-off-by: Hao Sun <sunhao.th@gmail.com>

Acked-by: Shung-Hsi Yu <shung-hsi.yu@suse.com>

Re: [PATCH bpf v2 1/2] bpf: Fix check_stack_write_fixed_off() to correctly spill imm

[Eduard Zingerman]

On Wed, 2023-11-01 at 08:33 +0100, Hao Sun wrote:
> In check_stack_write_fixed_off(), imm value is cast to u32 before being
> spilled to the stack. Therefore, the sign information is lost, and the
> range information is incorrect when load from the stack again.
> 
> For the following prog:
> 0: r2 = r10
> 1: *(u64*)(r2 -40) = -44
> 2: r0 = *(u64*)(r2 - 40)
> 3: if r0 s<= 0xa goto +2
> 4: r0 = 1
> 5: exit
> 6: r0  = 0
> 7: exit
> 
> The verifier gives:
> func#0 @0
> 0: R1=ctx(off=0,imm=0) R10=fp0
> 0: (bf) r2 = r10                      ; R2_w=fp0 R10=fp0
> 1: (7a) *(u64 *)(r2 -40) = -44        ; R2_w=fp0 fp-40_w=4294967252
> 2: (79) r0 = *(u64 *)(r2 -40)         ; R0_w=4294967252 R2_w=fp0
> fp-40_w=4294967252
> 3: (c5) if r0 s< 0xa goto pc+2
> mark_precise: frame0: last_idx 3 first_idx 0 subseq_idx -1
> mark_precise: frame0: regs=r0 stack= before 2: (79) r0 = *(u64 *)(r2 -40)
> 3: R0_w=4294967252
> 4: (b7) r0 = 1                        ; R0_w=1
> 5: (95) exit
> verification time 7971 usec
> stack depth 40
> processed 6 insns (limit 1000000) max_states_per_insn 0 total_states 0
> peak_states 0 mark_read 0
> 
> So remove the incorrect cast, since imm field is declared as s32, and
> __mark_reg_known() takes u64, so imm would be correctly sign extended
> by compiler.
> 
> Fixes: ecdf985d7615 ("bpf: track immediate values written to stack by BPF_ST instruction")
> Cc: stable@vger.kernel.org
> Signed-off-by: Hao Sun <sunhao.th@gmail.com>

Acked-by: Eduard Zingerman <eddyz87@gmail.com>

> ---
>  kernel/bpf/verifier.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index 857d76694517..44af69ce1301 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -4674,7 +4674,7 @@ static int check_stack_write_fixed_off(struct bpf_verifier_env *env,
>  		   insn->imm != 0 && env->bpf_capable) {
>  		struct bpf_reg_state fake_reg = {};
>  
> -		__mark_reg_known(&fake_reg, (u32)insn->imm);
> +		__mark_reg_known(&fake_reg, insn->imm);
>  		fake_reg.type = SCALAR_VALUE;
>  		save_register_state(state, spi, &fake_reg, size);
>  	} else if (reg && is_spillable_regtype(reg->type)) {
> 

[PATCH bpf v2 2/2] selftests/bpf: Add test for immediate spilled to stack

[Hao Sun]

Add a test to check if the verifier correctly reason about the sign
of an immediate spilled to stack by BPF_ST instruction.

Signed-off-by: Hao Sun <sunhao.th@gmail.com>
---
 tools/testing/selftests/bpf/verifier/bpf_st_mem.c | 32 +++++++++++++++++++++++
 1 file changed, 32 insertions(+)

diff --git a/tools/testing/selftests/bpf/verifier/bpf_st_mem.c b/tools/testing/selftests/bpf/verifier/bpf_st_mem.c
index 3af2501082b2..0ba23807c46c 100644
--- a/tools/testing/selftests/bpf/verifier/bpf_st_mem.c
+++ b/tools/testing/selftests/bpf/verifier/bpf_st_mem.c
@@ -65,3 +65,35 @@
 	.expected_attach_type = BPF_SK_LOOKUP,
 	.runs = -1,
 },
+{
+	"BPF_ST_MEM stack imm sign",
+	/* Check if verifier correctly reasons about sign of an
+	 * immediate spilled to stack by BPF_ST instruction.
+	 *
+	 *   fp[-8] = -44;
+	 *   r0 = fp[-8];
+	 *   if r0 s< 0 goto ret0;
+	 *   r0 = -1;
+	 *   exit;
+	 * ret0:
+	 *   r0 = 0;
+	 *   exit;
+	 */
+	.insns = {
+	BPF_ST_MEM(BPF_DW, BPF_REG_10, -8, -44),
+	BPF_LDX_MEM(BPF_DW, BPF_REG_0, BPF_REG_10, -8),
+	BPF_JMP_IMM(BPF_JSLT, BPF_REG_0, 0, 2),
+	BPF_MOV64_IMM(BPF_REG_0, -1),
+	BPF_EXIT_INSN(),
+	BPF_MOV64_IMM(BPF_REG_0, 0),
+	BPF_EXIT_INSN(),
+	},
+	/* Use prog type that requires return value in range [0, 1] */
+	.prog_type = BPF_PROG_TYPE_SK_LOOKUP,
+	.expected_attach_type = BPF_SK_LOOKUP,
+	.result = VERBOSE_ACCEPT,
+	.runs = -1,
+	.errstr = "0: (7a) *(u64 *)(r10 -8) = -44        ; R10=fp0 fp-8_w=-44\
+	2: (c5) if r0 s< 0x0 goto pc+2\
+	2: R0_w=-44",
+},

-- 
2.34.1

Re: [PATCH bpf v2 2/2] selftests/bpf: Add test for immediate spilled to stack

[Eduard Zingerman]

On Wed, 2023-11-01 at 08:33 +0100, Hao Sun wrote:
> Add a test to check if the verifier correctly reason about the sign
> of an immediate spilled to stack by BPF_ST instruction.
> 
> Signed-off-by: Hao Sun <sunhao.th@gmail.com>
> ---
>  tools/testing/selftests/bpf/verifier/bpf_st_mem.c | 32 +++++++++++++++++++++++
>  1 file changed, 32 insertions(+)
> 
> diff --git a/tools/testing/selftests/bpf/verifier/bpf_st_mem.c b/tools/testing/selftests/bpf/verifier/bpf_st_mem.c
> index 3af2501082b2..0ba23807c46c 100644
> --- a/tools/testing/selftests/bpf/verifier/bpf_st_mem.c
> +++ b/tools/testing/selftests/bpf/verifier/bpf_st_mem.c
> @@ -65,3 +65,35 @@
>  	.expected_attach_type = BPF_SK_LOOKUP,
>  	.runs = -1,
>  },
> +{
> +	"BPF_ST_MEM stack imm sign",
> +	/* Check if verifier correctly reasons about sign of an
> +	 * immediate spilled to stack by BPF_ST instruction.
> +	 *
> +	 *   fp[-8] = -44;
> +	 *   r0 = fp[-8];
> +	 *   if r0 s< 0 goto ret0;
> +	 *   r0 = -1;
> +	 *   exit;
> +	 * ret0:
> +	 *   r0 = 0;
> +	 *   exit;
> +	 */
> +	.insns = {
> +	BPF_ST_MEM(BPF_DW, BPF_REG_10, -8, -44),
> +	BPF_LDX_MEM(BPF_DW, BPF_REG_0, BPF_REG_10, -8),
> +	BPF_JMP_IMM(BPF_JSLT, BPF_REG_0, 0, 2),
> +	BPF_MOV64_IMM(BPF_REG_0, -1),
> +	BPF_EXIT_INSN(),
> +	BPF_MOV64_IMM(BPF_REG_0, 0),
> +	BPF_EXIT_INSN(),
> +	},
> +	/* Use prog type that requires return value in range [0, 1] */
> +	.prog_type = BPF_PROG_TYPE_SK_LOOKUP,
> +	.expected_attach_type = BPF_SK_LOOKUP,
> +	.result = VERBOSE_ACCEPT,
> +	.runs = -1,
> +	.errstr = "0: (7a) *(u64 *)(r10 -8) = -44        ; R10=fp0 fp-8_w=-44\
> +	2: (c5) if r0 s< 0x0 goto pc+2\
> +	2: R0_w=-44",
> +},
> 

Please note that this test case fails on CI [0], full log below:

2023-11-01T07:49:51.2841702Z #116/p BPF_ST_MEM stack imm sign FAIL
2023-11-01T07:49:51.2843456Z Unexpected verifier log!
2023-11-01T07:49:51.2844968Z EXP: 2: R0_w=-44
2023-11-01T07:49:51.2845583Z RES:
2023-11-01T07:49:51.2846693Z func#0 @0
2023-11-01T07:49:51.2848932Z 0: R1=ctx(off=0,imm=0) R10=fp0
2023-11-01T07:49:51.2853045Z 0: (7a) *(u64 *)(r10 -8) = -44        ; R10=fp0 fp-8_w=-44
2023-11-01T07:49:51.2857391Z 1: (79) r0 = *(u64 *)(r10 -8)         ; R0_w=-44 R10=fp0 fp-8_w=-44
2023-11-01T07:49:51.2859127Z 2: (c5) if r0 s< 0x0 goto pc+2
2023-11-01T07:49:51.2862943Z mark_precise: frame0: last_idx 2 first_idx 0 subseq_idx -1 
2023-11-01T07:49:51.2867511Z mark_precise: frame0: regs=r0 stack= before 1: (79) r0 = *(u64 *)(r10 -8)
2023-11-01T07:49:51.2872217Z mark_precise: frame0: regs= stack=-8 before 0: (7a) *(u64 *)(r10 -8) = -44
2023-11-01T07:49:51.2872816Z 5: R0_w=-44
2023-11-01T07:49:51.2875653Z 5: (b7) r0 = 0                        ; R0_w=0
2023-11-01T07:49:51.2876493Z 6: (95) exit

I suspect that after recent logging fixes instruction number printed
after jump changed and that's why test case no longer passes.

Note: you can check CI status for submitted patch-sets using link [1].

[0] https://github.com/kernel-patches/bpf/actions/runs/6717053909/job/18254330860
[1] https://patchwork.kernel.org/project/netdevbpf/list/

Re: [PATCH bpf v2 2/2] selftests/bpf: Add test for immediate spilled to stack

[Hao Sun]

On Wed, Nov 1, 2023 at 12:05 PM Eduard Zingerman <eddyz87@gmail.com> wrote:
>
> On Wed, 2023-11-01 at 08:33 +0100, Hao Sun wrote:
> > Add a test to check if the verifier correctly reason about the sign
> > of an immediate spilled to stack by BPF_ST instruction.
> >
> > Signed-off-by: Hao Sun <sunhao.th@gmail.com>
> > ---
> >  tools/testing/selftests/bpf/verifier/bpf_st_mem.c | 32 +++++++++++++++++++++++
> >  1 file changed, 32 insertions(+)
> >
> > diff --git a/tools/testing/selftests/bpf/verifier/bpf_st_mem.c b/tools/testing/selftests/bpf/verifier/bpf_st_mem.c
> > index 3af2501082b2..0ba23807c46c 100644
> > --- a/tools/testing/selftests/bpf/verifier/bpf_st_mem.c
> > +++ b/tools/testing/selftests/bpf/verifier/bpf_st_mem.c
> > @@ -65,3 +65,35 @@
> >       .expected_attach_type = BPF_SK_LOOKUP,
> >       .runs = -1,
> >  },
> > +{
> > +     "BPF_ST_MEM stack imm sign",
> > +     /* Check if verifier correctly reasons about sign of an
> > +      * immediate spilled to stack by BPF_ST instruction.
> > +      *
> > +      *   fp[-8] = -44;
> > +      *   r0 = fp[-8];
> > +      *   if r0 s< 0 goto ret0;
> > +      *   r0 = -1;
> > +      *   exit;
> > +      * ret0:
> > +      *   r0 = 0;
> > +      *   exit;
> > +      */
> > +     .insns = {
> > +     BPF_ST_MEM(BPF_DW, BPF_REG_10, -8, -44),
> > +     BPF_LDX_MEM(BPF_DW, BPF_REG_0, BPF_REG_10, -8),
> > +     BPF_JMP_IMM(BPF_JSLT, BPF_REG_0, 0, 2),
> > +     BPF_MOV64_IMM(BPF_REG_0, -1),
> > +     BPF_EXIT_INSN(),
> > +     BPF_MOV64_IMM(BPF_REG_0, 0),
> > +     BPF_EXIT_INSN(),
> > +     },
> > +     /* Use prog type that requires return value in range [0, 1] */
> > +     .prog_type = BPF_PROG_TYPE_SK_LOOKUP,
> > +     .expected_attach_type = BPF_SK_LOOKUP,
> > +     .result = VERBOSE_ACCEPT,
> > +     .runs = -1,
> > +     .errstr = "0: (7a) *(u64 *)(r10 -8) = -44        ; R10=fp0 fp-8_w=-44\
> > +     2: (c5) if r0 s< 0x0 goto pc+2\
> > +     2: R0_w=-44",
> > +},
> >
>
> Please note that this test case fails on CI [0], full log below:
>
> 2023-11-01T07:49:51.2841702Z #116/p BPF_ST_MEM stack imm sign FAIL
> 2023-11-01T07:49:51.2843456Z Unexpected verifier log!
> 2023-11-01T07:49:51.2844968Z EXP: 2: R0_w=-44
> 2023-11-01T07:49:51.2845583Z RES:
> 2023-11-01T07:49:51.2846693Z func#0 @0
> 2023-11-01T07:49:51.2848932Z 0: R1=ctx(off=0,imm=0) R10=fp0
> 2023-11-01T07:49:51.2853045Z 0: (7a) *(u64 *)(r10 -8) = -44        ; R10=fp0 fp-8_w=-44
> 2023-11-01T07:49:51.2857391Z 1: (79) r0 = *(u64 *)(r10 -8)         ; R0_w=-44 R10=fp0 fp-8_w=-44
> 2023-11-01T07:49:51.2859127Z 2: (c5) if r0 s< 0x0 goto pc+2
> 2023-11-01T07:49:51.2862943Z mark_precise: frame0: last_idx 2 first_idx 0 subseq_idx -1
> 2023-11-01T07:49:51.2867511Z mark_precise: frame0: regs=r0 stack= before 1: (79) r0 = *(u64 *)(r10 -8)
> 2023-11-01T07:49:51.2872217Z mark_precise: frame0: regs= stack=-8 before 0: (7a) *(u64 *)(r10 -8) = -44
> 2023-11-01T07:49:51.2872816Z 5: R0_w=-44
> 2023-11-01T07:49:51.2875653Z 5: (b7) r0 = 0                        ; R0_w=0
> 2023-11-01T07:49:51.2876493Z 6: (95) exit
>
> I suspect that after recent logging fixes instruction number printed
> after jump changed and that's why test case no longer passes.
>

Yes, so I guess we can just drop the line number there, will send patch v3.

> Note: you can check CI status for submitted patch-sets using link [1].
>
> [0] https://github.com/kernel-patches/bpf/actions/runs/6717053909/job/18254330860
> [1] https://patchwork.kernel.org/project/netdevbpf/list/

Thanks.