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

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

[Alexei Starovoitov]

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.

> I've attached a patch which will catch that situation. I've used the
> same error
> message from earlier in the code.
>
> Please consider adding my patch.
> --
> Kees Bakker

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

[Eduard Zingerman]

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.

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

[Kees Bakker]

Op 01-10-2024 om 03:21 schreef Alexei Starovoitov:
> 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.
Why do you think it is not possible? Isn't it better to be safe than sorry?
>
> Next time pls cc bpf@vger right away.
>
>> I've attached a patch which will catch that situation. I've used the
>> same error
>> message from earlier in the code.
>>
>> Please consider adding my patch.
>> --
>> Kees Bakker