Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751470AbdHFXgM (ORCPT ); Sun, 6 Aug 2017 19:36:12 -0400 Received: from www62.your-server.de ([213.133.104.62]:59740 "EHLO www62.your-server.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751355AbdHFXgK (ORCPT ); Sun, 6 Aug 2017 19:36:10 -0400 Message-ID: <5987A7DF.6080203@iogearbox.net> Date: Mon, 07 Aug 2017 01:35:59 +0200 From: Daniel Borkmann User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.7.0 MIME-Version: 1.0 To: Edward Cree , davem@davemloft.net, Alexei Starovoitov , Alexei Starovoitov CC: netdev@vger.kernel.org, linux-kernel@vger.kernel.org, iovisor-dev Subject: Re: [PATCH v4 net-next 01/13] bpf/verifier: rework value tracking References: <22441d84-0a11-5c00-2d2a-25e7dbafa6c2@solarflare.com> <8a5e37eb-2397-c986-79c5-02908fbbdee0@solarflare.com> In-Reply-To: <8a5e37eb-2397-c986-79c5-02908fbbdee0@solarflare.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit X-Authenticated-Sender: daniel@iogearbox.net Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 1880 Lines: 52 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 [...] > +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) {