Re: [PATCH v3 net-next 02/12] bpf/verifier: rework value tracking

[Daniel Borkmann]

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@xxxxxxxxxxxxxx>
[...]
> +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 = &regs[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 = &regs[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 {
[...]