Messages in this thread | | | Date | Wed, 28 Jun 2017 19:09:57 +0200 | From | Daniel Borkmann <> | Subject | Re: [PATCH v3 net-next 02/12] bpf/verifier: rework value tracking |
| |
On 06/27/2017 02:56 PM, Edward Cree wrote: > Tracks value alignment by means of tracking known & unknown bits. > Tightens some min/max value checks and fixes a couple of bugs therein. > If pointer leaks are allowed, and adjust_ptr_min_max_vals returns -EACCES, > treat the pointer as an unknown scalar and try again, because we might be > able to conclude something about the result (e.g. pointer & 0x40 is either > 0 or 0x40). > > Signed-off-by: Edward Cree <ecree@solarflare.com> [...] > +static int adjust_reg_min_max_vals(struct bpf_verifier_env *env, > + struct bpf_insn *insn) > +{ > + struct bpf_reg_state *regs = env->cur_state.regs, *dst_reg, *src_reg; > + struct bpf_reg_state *ptr_reg = NULL, off_reg = {0}; > + u8 opcode = BPF_OP(insn->code); > + int rc; > + > + dst_reg = ®s[insn->dst_reg]; > + check_reg_overflow(dst_reg); > + src_reg = NULL; > + if (dst_reg->type != SCALAR_VALUE) > + ptr_reg = dst_reg; > + if (BPF_SRC(insn->code) == BPF_X) { > + src_reg = ®s[insn->src_reg]; > + check_reg_overflow(src_reg); > + > + if (src_reg->type != SCALAR_VALUE) { > + if (dst_reg->type != SCALAR_VALUE) { > + /* Combining two pointers by any ALU op yields > + * an arbitrary scalar. > + */ > + if (!env->allow_ptr_leaks) { > + verbose("R%d pointer %s pointer prohibited\n", > + insn->dst_reg, > + bpf_alu_string[opcode >> 4]); > + return -EACCES; > + } > + mark_reg_unknown(regs, insn->dst_reg); > + return 0; > + } else { > + /* scalar += pointer > + * This is legal, but we have to reverse our > + * src/dest handling in computing the range > + */ > + rc = adjust_ptr_min_max_vals(env, insn, > + src_reg, dst_reg); > + if (rc == -EACCES && env->allow_ptr_leaks) { > + /* scalar += unknown scalar */ > + __mark_reg_unknown(&off_reg); > + return adjust_scalar_min_max_vals( > + env, insn, > + dst_reg, &off_reg);
Could you elaborate on this one? If I understand it correctly, then the scalar += pointer case would mean the following: given I have one of the allowed pointer types in adjust_ptr_min_max_vals() then the prior scalar type inherits the ptr type/id. I would then 'destroy' the pointer value so we get a -EACCES on it. We mark the tmp off_reg as scalar type, but shouldn't also actual dst_reg be marked as such like in below pointer += scalar case, such that we undo the prior ptr_type inheritance?
> + } > + return rc; > + } > + } else if (ptr_reg) { > + /* pointer += scalar */ > + rc = adjust_ptr_min_max_vals(env, insn, > + dst_reg, src_reg); > + if (rc == -EACCES && env->allow_ptr_leaks) { > + /* unknown scalar += scalar */ > + __mark_reg_unknown(dst_reg); > + return adjust_scalar_min_max_vals( > + env, insn, dst_reg, src_reg); > + } > + return rc; > + } > + } else { [...]
| |