Re: Possible out-of-bounds writing at kernel/bpf/verifier.c:19927

[Eduard Zingerman <eddyz87@gmail.com>]

On Mon, 2024-09-30 at 18:21 -0700, Alexei Starovoitov wrote:
> On Mon, Sep 30, 2024 at 11:01 AM Kees Bakker <kees@ijzerbout.nl> wrote:
> > 
> > Hi,
> > 
> > In the following commit you added a few lines to kernel/bpf/verifier.c
> > 
> > commit 1f1e864b65554e33fe74e3377e58b12f4302f2eb
> > Author: Yonghong Song <yonghong.song@linux.dev>
> > 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@gmail.com>
> >      Signed-off-by: Yonghong Song <yonghong.song@linux.dev>
> >      Link:
> > https://lore.kernel.org/r/20230728011207.3712528-1-yonghong.song@linux.dev
> >      Signed-off-by: Alexei Starovoitov <ast@kernel.org>
> > ...
> > 
> > +               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.