Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751653AbdF1S2p (ORCPT ); Wed, 28 Jun 2017 14:28:45 -0400 Received: from dispatch1-us1.ppe-hosted.com ([67.231.154.164]:54063 "EHLO dispatch1-us1.ppe-hosted.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751541AbdF1S2i (ORCPT ); Wed, 28 Jun 2017 14:28:38 -0400 Subject: Re: [PATCH v3 net-next 02/12] bpf/verifier: rework value tracking To: Daniel Borkmann , , "Alexei Starovoitov" , Alexei Starovoitov References: <2244b48b-f415-3239-6912-cb09f0abc546@solarflare.com> <5953E2E5.7040106@iogearbox.net> CC: , , iovisor-dev From: Edward Cree Message-ID: <7bae321b-c0ac-ddea-3abf-a30d940a9f89@solarflare.com> Date: Wed, 28 Jun 2017 19:28:27 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.1.0 MIME-Version: 1.0 In-Reply-To: <5953E2E5.7040106@iogearbox.net> Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: 7bit X-Originating-IP: [10.17.20.45] X-ClientProxiedBy: ocex03.SolarFlarecom.com (10.20.40.36) To ukex01.SolarFlarecom.com (10.17.10.4) X-TM-AS-Product-Ver: SMEX-11.0.0.1191-8.100.1062-23162.003 X-TM-AS-Result: No--3.355500-0.000000-31 X-TM-AS-User-Approved-Sender: Yes X-TM-AS-User-Blocked-Sender: No X-MDID: 1498674517-QQwB76L45Jpx Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 984 Lines: 17 On 28/06/17 18:09, Daniel Borkmann wrote: > 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? Good catch. The intent was that adjust_ptr_min_max_vals() wouldn't mark dst_reg's type/id in the case when it returned -EACCES, but indeed there are some such paths, and rather than changing those it may be easier to change the type/id back to scalar/0. I don't think we need to go as far as calling __mark_reg_unknown() on it though, its bounds and align shouldn't have been screwed up by adjust_ptr_min_max_vals(). -Ed