Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:7 as permitted sender) client-ip=2620:137:e000::3:7;
MIME-Version: 1.0
From:   Hao Sun <sunhao.th@gmail.com>
Date:   Wed, 25 Oct 2023 11:16:34 +0200
Message-ID: <CACkBjsYXA8myxoP0Naz=ZxB0FWG-xS9e28CSFffGk1bA_n5RXw@mail.gmail.com>
Subject: bpf: incorrect value spill in check_stack_write_fixed_off()
To:     Alexei Starovoitov <ast@kernel.org>,
        Daniel Borkmann <daniel@iogearbox.net>,
        John Fastabend <john.fastabend@gmail.com>,
        Andrii Nakryiko <andrii@kernel.org>,
        Martin KaFai Lau <martin.lau@linux.dev>,
        Song Liu <song@kernel.org>,
        Yonghong Song <yonghong.song@linux.dev>,
        KP Singh <kpsingh@kernel.org>,
        Stanislav Fomichev <sdf@google.com>,
        Hao Luo <haoluo@google.com>, Jiri Olsa <jolsa@kernel.org>
Cc:     bpf <bpf@vger.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

Hi,

In check_stack_write_fixed_off(), the verifier creates a fake reg to store the
imm in a BPF_ST_MEM:
...
else if (!reg && !(off % BPF_REG_SIZE) && is_bpf_st_mem(insn) &&
insn->imm != 0 && env->bpf_capable) {
        struct bpf_reg_state fake_reg = {};

        __mark_reg_known(&fake_reg, (u32)insn->imm);
        fake_reg.type = SCALAR_VALUE;
        save_register_state(state, spi, &fake_reg, size);

Here, insn->imm is cast to u32, and used to mark fake_reg, which is incorrect
and may lose sign information. Consider the following program:

r2 = r10
*(u64*)(r2 -40) = -44
r0 = *(u64*)(r2 - 40)
if r0 s<= 0xa goto +2
r0 = 0
exit
r0  = 1
exit

The verifier gives the following log:

-------- Verifier Log --------
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

Here, the verifier incorrectly thinks R0 is 0xffffffd4, which should
be 0xffffffffffffffd4,
due to the u32 cast in check_stack_write_fixed_off(). This makes the verifier
collect incorrect reg scalar range.

Since insn->imm is i32, we should cast it to the signed integer with
correct size
according to BPF_MEM, then promoting the imm to u64 to mark fake reg as
known, right?

Best
Hao