linux-kernel - Re: bpf: incorrect value spill in 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:   Wed, 25 Oct 2023 15:14:27 +0300
From:   Eduard Zingerman <eddyz87@...il.com>
To:     Hao Sun <sunhao.th@...il.com>, 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>
Cc:     bpf <bpf@...r.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>
Subject: Re: bpf: incorrect value spill in check_stack_write_fixed_off()

On Wed, 2023-10-25 at 11:16 +0200, Hao Sun wrote:
> 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.

This bug is on me.
Thank you for reporting it along with the example program.
Looks like the patch below is sufficient to fix the issue.
Have no idea at the moment why I used u32 cast there.
Let me think a bit more about it and I'll submit an official patch.

Thanks,
Eduard

---

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