linux-kernel - Re: [PATCH bpf-next 1/2] bpf: Fix check_stack_write_fixed

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Fri, 27 Oct 2023 15:44:10 +0800
From:   Shung-Hsi Yu <shung-hsi.yu@...e.com>
To:     Hao Sun <sunhao.th@...il.com>
Cc:     Alexei Starovoitov <ast@...nel.org>,
        Daniel Borkmann <daniel@...earbox.net>,
        John Fastabend <john.fastabend@...il.com>,
        Andrii Nakryiko <andrii@...nel.org>,
        Martin KaFai Lau <martin.lau@...ux.dev>,
        Song Liu <song@...nel.org>,
        Yonghong Song <yonghong.song@...ux.dev>,
        KP Singh <kpsingh@...nel.org>,
        Stanislav Fomichev <sdf@...gle.com>,
        Hao Luo <haoluo@...gle.com>, Jiri Olsa <jolsa@...nel.org>,
        Mykola Lysenko <mykolal@...com>, Shuah Khan <shuah@...nel.org>,
        Eduard Zingerman <eddyz87@...il.com>, bpf@...r.kernel.org,
        linux-kernel@...r.kernel.org, linux-kselftest@...r.kernel.org
Subject: Re: [PATCH bpf-next 1/2] bpf: Fix check_stack_write_fixed_off() to
 correctly spill imm

On Fri, Oct 27, 2023 at 03:14:10PM +0800, Shung-Hsi Yu wrote:
> On Thu, Oct 26, 2023 at 05:13:10PM +0200, 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.
> > 
> > Signed-off-by: Hao Sun <sunhao.th@...il.com>
> 
> Acked-by: Shung-Hsi Yu <shung-hsi.yu@...e.com>
> 
> The acked-by applies to future version of the patchset as well.

Oh and since this is a fix it would be great to have the fixes tag[1] to
specify when the bug was introduced

Fixes: ecdf985d7615 ("bpf: track immediate values written to stack by BPF_ST instruction")

Add Cc tag for stable[2] so stable kernels pick up the fix as well

Cc: stable@...r.kernel.org

And ideally specify that the patch should be applied to the bpf tree rather
than bpf-next[3] (though the BPF maintainers has the final say on which tree
this patch should be applied).

I'd owe you a big thank as well since this helps with our internal process
at my company. So thank you in advance!

1: https://docs.kernel.org/process/submitting-patches.html#describe-your-changes
2: https://www.kernel.org/doc/html/latest/process/stable-kernel-rules.html#option-1
3: https://docs.kernel.org/bpf/bpf_devel_QA.html#q-how-do-the-changes-make-their-way-into-linux

> FWIW I think we'd also need the same treatment for the (BPF_ALU | BPF_MOV |
> BPF_K) case in check_alu_op().
> 
> > ---
> >  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
> >