[bug report] bpf: Allow narrow loads with offset > 0

[bug report] bpf: Allow narrow loads with offset > 0

[Dan Carpenter]

Hello Andrey Ignatov,

The patch 46f53a65d2de: "bpf: Allow narrow loads with offset > 0"
from Nov 10, 2018, leads to the following
Smatch static checker warning:

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 }

regards,
dan carpenter

Re: [bug report] bpf: Allow narrow loads with offset > 0

[Andrey Ignatov]

Dan Carpenter <dan.carpenter@oracle.com> [Mon, 2021-08-16 22:09 -0700]:
> Hello Andrey Ignatov,
> 
> The patch 46f53a65d2de: "bpf: Allow narrow loads with offset > 0"
> from Nov 10, 2018, leads to the following
> Smatch static checker warning:
> 
> 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

Makes sense. I'll send the fix this week. Thanks for report.

-- 
Andrey Ignatov