lkml.org 
[lkml]   [2021]   [Sep]   [13]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    Patch in this message
    /
    From
    Subject[PATCH 5.14 254/334] bpf: Fix possible out of bound write in narrow load handling
    Date
    From: Andrey Ignatov <rdna@fb.com>

    [ Upstream commit d7af7e497f0308bc97809cc48b58e8e0f13887e1 ]

    Fix a verifier bug found by smatch static checker in [0].

    This problem has never been seen in prod to my best knowledge. Fixing it
    still seems to be a good idea since it's hard to say for sure whether
    it's possible or not to have a scenario where a combination of
    convert_ctx_access() and a narrow load would lead to an out of bound
    write.

    When narrow load is handled, one or two new instructions are added to
    insn_buf array, but before it was only checked that

    cnt >= ARRAY_SIZE(insn_buf)

    And it's safe to add a new instruction to insn_buf[cnt++] only once. The
    second try will lead to out of bound write. And this is what can happen
    if `shift` is set.

    Fix it by making sure that if the BPF_RSH instruction has to be added in
    addition to BPF_AND then there is enough space for two more instructions
    in insn_buf.

    The full report [0] is below:

    kernel/bpf/verifier.c:12304 convert_ctx_accesses() warn: offset 'cnt' incremented past end of array
    kernel/bpf/verifier.c:12311 convert_ctx_accesses() warn: offset 'cnt' incremented past end of array

    kernel/bpf/verifier.c
    12282
    12283 insn->off = off & ~(size_default - 1);
    12284 insn->code = BPF_LDX | BPF_MEM | size_code;
    12285 }
    12286
    12287 target_size = 0;
    12288 cnt = convert_ctx_access(type, insn, insn_buf, env->prog,
    12289 &target_size);
    12290 if (cnt == 0 || cnt >= ARRAY_SIZE(insn_buf) ||
    ^^^^^^^^^^^^^^^^^^^^^^^^^^^
    Bounds check.

    12291 (ctx_field_size && !target_size)) {
    12292 verbose(env, "bpf verifier is misconfigured\n");
    12293 return -EINVAL;
    12294 }
    12295
    12296 if (is_narrower_load && size < target_size) {
    12297 u8 shift = bpf_ctx_narrow_access_offset(
    12298 off, size, size_default) * 8;
    12299 if (ctx_field_size <= 4) {
    12300 if (shift)
    12301 insn_buf[cnt++] = BPF_ALU32_IMM(BPF_RSH,
    ^^^^^
    increment beyond end of array

    12302 insn->dst_reg,
    12303 shift);
    --> 12304 insn_buf[cnt++] = BPF_ALU32_IMM(BPF_AND, insn->dst_reg,
    ^^^^^
    out of bounds write

    12305 (1 << size * 8) - 1);
    12306 } else {
    12307 if (shift)
    12308 insn_buf[cnt++] = BPF_ALU64_IMM(BPF_RSH,
    12309 insn->dst_reg,
    12310 shift);
    12311 insn_buf[cnt++] = BPF_ALU64_IMM(BPF_AND, insn->dst_reg,
    ^^^^^^^^^^^^^^^
    Same.

    12312 (1ULL << size * 8) - 1);
    12313 }
    12314 }
    12315
    12316 new_prog = bpf_patch_insn_data(env, i + delta, insn_buf, cnt);
    12317 if (!new_prog)
    12318 return -ENOMEM;
    12319
    12320 delta += cnt - 1;
    12321
    12322 /* keep walking new program and skip insns we just inserted */
    12323 env->prog = new_prog;
    12324 insn = new_prog->insnsi + i + delta;
    12325 }
    12326
    12327 return 0;
    12328 }

    [0] https://lore.kernel.org/bpf/20210817050843.GA21456@kili/

    v1->v2:
    - clarify that problem was only seen by static checker but not in prod;

    Fixes: 46f53a65d2de ("bpf: Allow narrow loads with offset > 0")
    Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
    Signed-off-by: Andrey Ignatov <rdna@fb.com>
    Signed-off-by: Alexei Starovoitov <ast@kernel.org>
    Link: https://lore.kernel.org/bpf/20210820163935.1902398-1-rdna@fb.com
    Signed-off-by: Sasha Levin <sashal@kernel.org>
    ---
    kernel/bpf/verifier.c | 4 ++++
    1 file changed, 4 insertions(+)

    diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
    index 10084c0600dd..9d94ac6ff50c 100644
    --- a/kernel/bpf/verifier.c
    +++ b/kernel/bpf/verifier.c
    @@ -12013,6 +12013,10 @@ static int convert_ctx_accesses(struct bpf_verifier_env *env)
    if (is_narrower_load && size < target_size) {
    u8 shift = bpf_ctx_narrow_access_offset(
    off, size, size_default) * 8;
    + if (shift && cnt + 1 >= ARRAY_SIZE(insn_buf)) {
    + 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,
    --
    2.30.2


    \
     
     \ /
      Last update: 2021-09-13 16:45    [W:4.188 / U:0.060 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site