Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752133AbdF1RKN (ORCPT ); Wed, 28 Jun 2017 13:10:13 -0400 Received: from www62.your-server.de ([213.133.104.62]:41567 "EHLO www62.your-server.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751521AbdF1RKF (ORCPT ); Wed, 28 Jun 2017 13:10:05 -0400 Message-ID: <5953E2E5.7040106@iogearbox.net> Date: Wed, 28 Jun 2017 19:09:57 +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 v3 net-next 02/12] bpf/verifier: rework value tracking References: <2244b48b-f415-3239-6912-cb09f0abc546@solarflare.com> In-Reply-To: <2244b48b-f415-3239-6912-cb09f0abc546@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: 2869 Lines: 80 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 [...] > +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 { [...]