netdev - Re: [PATCH v4 net-next 01/13] bpf/verifier: rework value tracking

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <5987A7DF.6080203@iogearbox.net>
Date:   Mon, 07 Aug 2017 01:35:59 +0200
From:   Daniel Borkmann <daniel@...earbox.net>
To:     Edward Cree <ecree@...arflare.com>, davem@...emloft.net,
        Alexei Starovoitov <alexei.starovoitov@...il.com>,
        Alexei Starovoitov <ast@...com>
CC:     netdev@...r.kernel.org, linux-kernel@...r.kernel.org,
        iovisor-dev <iovisor-dev@...ts.iovisor.org>
Subject: Re: [PATCH v4 net-next 01/13] bpf/verifier: rework value tracking

On 08/03/2017 06:11 PM, Edward Cree wrote:
> Unifies adjusted and unadjusted register value types (e.g. FRAME_POINTER is
>   now just a PTR_TO_STACK with zero offset).
> Tracks value alignment by means of tracking known & unknown bits.  This
>   also replaces the 'reg->imm' (leading zero bits) calculations for (what
>   were) UNKNOWN_VALUEs.
> 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@...arflare.com>
[...]
> +static int adjust_scalar_min_max_vals(struct bpf_verifier_env *env,
> +				      struct bpf_insn *insn,
> +				      struct bpf_reg_state *dst_reg,
> +				      struct bpf_reg_state *src_reg)
>   {
>   	struct bpf_reg_state *regs = env->cur_state.regs;
[...]
> -	} else if (insn->imm < BPF_REGISTER_MAX_RANGE &&
> -		   (s64)insn->imm > BPF_REGISTER_MIN_RANGE) {
> -		min_val = max_val = insn->imm;
> -		src_align = calc_align(insn->imm);
> +	if (BPF_CLASS(insn->code) != BPF_ALU64) {
> +		/* 32-bit ALU ops are (32,32)->64 */
> +		coerce_reg_to_32(dst_reg);
> +		coerce_reg_to_32(src_reg);
>   	}
> -
[...]
> -			dst_reg->max_value = BPF_REGISTER_MAX_RANGE;
> +	if (BPF_CLASS(insn->code) != BPF_ALU64) {
> +		/* 32-bit ALU ops are (32,32)->64 */
> +		coerce_reg_to_32(dst_reg);
> +		coerce_reg_to_32(src_reg);
>   	}

Looks like the same check was added twice here right after
the first one? Shouldn't we just temporarily coerce the src
reg to 32 bit here given in the actual op the src reg is not
being modified?

Thanks,
Daniel

> +	min_val = src_reg->min_value;
> +	max_val = src_reg->max_value;
> +	src_known = tnum_is_const(src_reg->var_off);
> +	dst_known = tnum_is_const(dst_reg->var_off);
>
>   	switch (opcode) {