On Sun, Nov 17, 2019 at 10:27:04AM -0800, Yonghong Song wrote:
> Currently, for all helpers with integer return type,
> the verifier permits a single return type, RET_INTEGER,
> which represents 64-bit return value from the helper,
> and the verifier will assign 64-bit value ranges for these
> return values. Such an assumption is different
> from what compiler sees and the generated code with
> llvm alu32 mode, and may lead verification failure.
>
> For example, with latest llvm, selftest test_progs
> failed for program raw_tracepoint/kfree_skb.
> The source code looks like below:
>
> static __always_inline uint64_t read_str_var(...)
> {
> len = bpf_probe_read_user_str(payload, STROBE_MAX_STR_LEN, value->ptr);
> if (len > STROBE_MAX_STR_LEN)
> return 0;
> ...
> return len;
> }
> for (int i = 0; i < STROBE_MAX_STRS; ++i) {
> payload += read_str_var(cfg, i, tls_base, &value, data, payload);
> }
>
> In the above, "cfg" refers to map "strobemeta_cfgs" and the signature
> for bpf_probe_read_user_str() is:
> static int (*bpf_probe_read_user_str)(void *dst, __u32 size, const void *unsafe_ptr) = (void *) 114;
> in tools/lib/bpf/bpf_helper_defs.h.
>
> The code path causing verification failure looks like below:
> 193: call bpf_probe_read_user_str
> 194: if (w0 > 0x1) goto pc + 2
> 195: *(u16 *)(r7 +80) = r0
> 196: r6 = r0
> ...
> 199: r8 = *(u64 *)(r10 -416)
> 200: r8 += r6
> R1 unbounded memory access, make sure to bounds check any array access into a map
>
> After insn 193, the current verifier assumes r0 can be any 64-bit value.
> R0=inv(id=0)
> At insn 194, the compiler assumes bpf_probe_read_user_str() will return
> an __s32 value hences uses subregister w0 at insn 194 and at
> branch false target, the w0 range can be refined to unsigned values <= 1.
> But since insn 193 marks r0 with non-empty upper 32bit value, the new
> umax_value becomes 0xffffffff00000001.
> R0_w=inv(id=0,umax_value=18446744069414584321)
> At insn 196, the register r0 is assigned to r6.
> At insn 199, map pointer to strobemeta_cfgs map is restored and further refined
> at insn 200 and 201.
> At insn 200, the verifier complains r8 + r6 unbounded memory access since r6 has
> R6_rw=invP(id=0,umax_value=18446744069414584321)
>
> The pattern can be roughly described by the following steps:
> . helper return 32bit value, but return value marked conservatively.
> . sub-registeer w0 is refined.
> . r0 is used and the refined w0 range is lost.
> . later use of r0 may trigger some kind of unbound failure.
>
> To remove such failure, r0 range at insn 193 should be more aligned with
> what user expect based on function prototype in bpf_helper_defs.h, i.e.,
> its value range should be with 32-bit value. This patches distinguished
> 32-bit from 64-bit return values in verifier with proper return value
> ranges in r0. In the above case,
> after insn 193, the verifier will give r0 range as
> R0_w=inv(id=0,smin_value=-2147483648,smax_value=2147483647,umax_value=4294967295,var_off=(0x0; 0xffffffff))
> after insn 194, the new r0 range will be
> R0_w=inv(id=0,umax_value=1,var_off=(0x0; 0x1))
> This better register range avoids above verification failure.
conceptually looks good. please refactor the code though.
> +static void mark_ret_reg_unknown(struct bpf_verifier_env *env,
> + struct bpf_reg_state *regs,
> + enum bpf_return_type ret_type)
> +{
> + struct bpf_reg_state *reg = regs + BPF_REG_0;
> +
> + memset(reg, 0, offsetof(struct bpf_reg_state, var_off));
only __mark_reg_known and __mark_reg_unknown should be allowed to do this.
> + reg->type = SCALAR_VALUE;
> +
> + if (ret_type == RET_INT64) {
> + reg->var_off.mask = -1;
This is internal details of tnum_unknown. The code should not assign var_off directly.
> + __mark_reg_unbounded(reg);
> + } else {
> + reg->var_off.mask = 0xffffffffULL;
same thing. Please introduce tnum_unknown32 instead
> + reg->smin_value = S32_MIN;
> + reg->smax_value = S32_MAX;
> + reg->umin_value = 0;
> + reg->umax_value = U32_MAX;
and this can be derived from tnum_unknown32.
> + }
> +
> + regs->precise = env->subprog_cnt > 1 || !env->allow_ptr_leaks ?
> + true : false;
Doesn't belong here. Should really be one helper that does this.
