• Andrey Ignatov's avatar
    bpf: Fix possible out of bound write in narrow load handling · d7af7e49
    Andrey Ignatov authored
    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: 46f53a65 ("bpf: Allow narrow loads with offset > 0")
    Reported-by: default avatarDan Carpenter <dan.carpenter@oracle.com>
    Signed-off-by: default avatarAndrey Ignatov <rdna@fb.com>
    Signed-off-by: default avatarAlexei Starovoitov <ast@kernel.org>
    Link: https://lore.kernel.org/bpf/20210820163935.1902398-1-rdna@fb.com
    d7af7e49
verifier.c 395 KB