* Re: [PATCH bpf v2 1/2] bpf: Fix check_stack_write_fixed_off() to correctly spill imm
2023-11-01 7:33 ` [PATCH bpf v2 1/2] bpf: Fix check_stack_write_fixed_off() to correctly spill imm Hao Sun
@ 2023-11-01 8:42 ` Shung-Hsi Yu
2023-11-01 11:07 ` Eduard Zingerman
1 sibling, 0 replies; 4+ messages in thread
From: Shung-Hsi Yu @ 2023-11-01 8:42 UTC (permalink / raw)
To: Hao Sun
Cc: Alexei Starovoitov, Daniel Borkmann, John Fastabend,
Andrii Nakryiko, Martin KaFai Lau, Song Liu, Yonghong Song,
KP Singh, Stanislav Fomichev, Hao Luo, Jiri Olsa, Mykola Lysenko,
Shuah Khan, Eduard Zingerman, bpf, linux-kernel, linux-kselftest,
stable
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>
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH bpf v2 1/2] bpf: Fix check_stack_write_fixed_off() to correctly spill imm
2023-11-01 7:33 ` [PATCH bpf v2 1/2] bpf: Fix check_stack_write_fixed_off() to correctly spill imm Hao Sun
2023-11-01 8:42 ` Shung-Hsi Yu
@ 2023-11-01 11:07 ` Eduard Zingerman
1 sibling, 0 replies; 4+ messages in thread
From: Eduard Zingerman @ 2023-11-01 11:07 UTC (permalink / raw)
To: Hao Sun, Alexei Starovoitov, Daniel Borkmann, John Fastabend,
Andrii Nakryiko, Martin KaFai Lau, Song Liu, Yonghong Song,
KP Singh, Stanislav Fomichev, Hao Luo, Jiri Olsa, Mykola Lysenko,
Shuah Khan, Shung-Hsi Yu
Cc: bpf, linux-kernel, linux-kselftest, stable
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)) {
>
^ permalink raw reply [flat|nested] 4+ messages in thread