On Mon, 2024-09-30 at 18:21 -0700, Alexei Starovoitov wrote: > On Mon, Sep 30, 2024 at 11:01 AM Kees Bakker <kees@xxxxxxxxxxxx> wrote: > > > > Hi, > > > > In the following commit you added a few lines to kernel/bpf/verifier.c > > > > commit 1f1e864b65554e33fe74e3377e58b12f4302f2eb > > Author: Yonghong Song <yonghong.song@xxxxxxxxx> > > Date: Thu Jul 27 18:12:07 2023 -0700 > > > > bpf: Handle sign-extenstin ctx member accesses > > > > Currently, if user accesses a ctx member with signed types, > > the compiler will generate an unsigned load followed by > > necessary left and right shifts. > > > > With the introduction of sign-extension load, compiler may > > just emit a ldsx insn instead. Let us do a final movsx sign > > extension to the final unsigned ctx load result to > > satisfy original sign extension requirement. > > > > Acked-by: Eduard Zingerman <eddyz87@xxxxxxxxx> > > Signed-off-by: Yonghong Song <yonghong.song@xxxxxxxxx> > > Link: > > https://lore.kernel.org/r/20230728011207.3712528-1-yonghong.song@xxxxxxxxx > > Signed-off-by: Alexei Starovoitov <ast@xxxxxxxxxx> > > ... > > > > + if (mode == BPF_MEMSX) > > + insn_buf[cnt++] = BPF_RAW_INSN(BPF_ALU64 | > > BPF_MOV | BPF_X, > > + insn->dst_reg, insn->dst_reg, > > + size * 8, 0); > > > > However, you forgot to check for array out-of-bounds check. In the if > > statement > > right above it, it is possible that insn_buf is filled up to the max. > > I don't think it's possible. > There is no need for such a check. > > Next time pls cc bpf@vger right away. It shouldn't be possible, but the code above does the same check: if (is_narrower_load && size < target_size) { u8 shift = bpf_ctx_narrow_access_offset( off, size, size_default) * 8; if (shift && cnt + 1 >= INSN_BUF_SIZE) { verbose(env, "bpf verifier narrow ctx load misconfigured\n"); return -EINVAL; } if (ctx_field_size <= 4) { if (shift) insn_buf[cnt++] = BPF_ALU32_IMM(BPF_RSH, insn->dst_reg, shift); ... } } So we are a bit inconsistent here.
