* Re: Possible out-of-bounds writing at kernel/bpf/verifier.c:19927
2024-10-01 1:21 ` Possible out-of-bounds writing at kernel/bpf/verifier.c:19927 Alexei Starovoitov
@ 2024-10-01 1:26 ` Eduard Zingerman
2024-10-01 18:29 ` Kees Bakker
1 sibling, 0 replies; 3+ messages in thread
From: Eduard Zingerman @ 2024-10-01 1:26 UTC (permalink / raw)
To: Alexei Starovoitov, Kees Bakker, bpf; +Cc: Yonghong Song, Alexei Starovoitov
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.
^ permalink raw reply [flat|nested] 3+ messages in thread* Re: Possible out-of-bounds writing at kernel/bpf/verifier.c:19927
2024-10-01 1:21 ` Possible out-of-bounds writing at kernel/bpf/verifier.c:19927 Alexei Starovoitov
2024-10-01 1:26 ` Eduard Zingerman
@ 2024-10-01 18:29 ` Kees Bakker
1 sibling, 0 replies; 3+ messages in thread
From: Kees Bakker @ 2024-10-01 18:29 UTC (permalink / raw)
To: Alexei Starovoitov, bpf
Cc: Yonghong Song, Eduard Zingerman, Alexei Starovoitov
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
^ permalink raw reply [flat|nested] 3+ messages in thread