From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751361AbdGQRCS (ORCPT ); Mon, 17 Jul 2017 13:02:18 -0400 Received: from dispatch1-us1.ppe-hosted.com ([67.231.154.164]:52027 "EHLO dispatch1-us1.ppe-hosted.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751293AbdGQRCQ (ORCPT ); Mon, 17 Jul 2017 13:02:16 -0400 Subject: Re: [iovisor-dev] [PATCH v3 net-next 02/12] bpf/verifier: rework value tracking To: Nadav Amit References: <2244b48b-f415-3239-6912-cb09f0abc546@solarflare.com> <3A96914E-3009-4E19-B138-7A636A76D9C8@gmail.com> <68a2487e-f706-1b61-5c4c-20ffe6d51127@solarflare.com> <12DCE590-7F67-4639-A825-61A24AC44ED3@gmail.com> CC: "David S. Miller" , Alexei Starovoitov , Alexei Starovoitov , "Daniel Borkmann" , , iovisor-dev , From: Edward Cree Message-ID: Date: Mon, 17 Jul 2017 18:02:02 +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: <12DCE590-7F67-4639-A825-61A24AC44ED3@gmail.com> 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-23200.003 X-TM-AS-Result: No--4.319700-0.000000-31 X-TM-AS-User-Approved-Sender: Yes X-TM-AS-User-Blocked-Sender: No X-MDID: 1500310933-8+wDf2ZrJ4np Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 12/07/17 23:07, Nadav Amit wrote: > Edward Cree wrote: >> In this specific case, there was a bug before: if (say) src and dst were >> both unknown bytes (so range 0 to 255), it would compute the new min and max >> to be 0, so it would think the result is known to be 0. But that's wrong, >> because it could be anything from -255 to +255. The bug's implications are >> that it could be used to construct an out-of-range offset to (say) a map >> pointer which the verifier would think was in-range and thus accept. > This sounds like a serious bug that may need to be backported to stable > versions, no? In this case I would assume it should be in a separate patch > so it could be applied separately. Having looked deeper into this in attempting to create a test that the existing verifier would fail, it turns out that in the existing verifier that BPF_SUB handling is dead code. If (for instance) we subtract an UNKNOWN_VALUE from a PTR_TO_MAP_VALUE_ADJ, that code will be run, but afterwards we will mark_reg_unknown_value() the register (bottom of check_alu_op()) making our previous min/max determination irrelevant. So there's nothing to backport, and if I did change this in its own patch, there'd be no way to test it. (I have, however, added a test covering this codepath in the new verifier.) -Ed From mboxrd@z Thu Jan 1 00:00:00 1970 From: Edward Cree via iovisor-dev Subject: Re: [PATCH v3 net-next 02/12] bpf/verifier: rework value tracking Date: Mon, 17 Jul 2017 18:02:02 +0100 Message-ID: References: <2244b48b-f415-3239-6912-cb09f0abc546@solarflare.com> <3A96914E-3009-4E19-B138-7A636A76D9C8@gmail.com> <68a2487e-f706-1b61-5c4c-20ffe6d51127@solarflare.com> <12DCE590-7F67-4639-A825-61A24AC44ED3@gmail.com> Reply-To: Edward Cree Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Cc: Alexei Starovoitov , netdev-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, iovisor-dev , linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, "David S. Miller" To: Nadav Amit Return-path: In-Reply-To: <12DCE590-7F67-4639-A825-61A24AC44ED3-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: iovisor-dev-bounces-9jONkmmOlFHEE9lA1F8Ukti2O/JbrIOy@public.gmane.org Errors-To: iovisor-dev-bounces-9jONkmmOlFHEE9lA1F8Ukti2O/JbrIOy@public.gmane.org List-Id: netdev.vger.kernel.org On 12/07/17 23:07, Nadav Amit wrote: > Edward Cree wrote: >> In this specific case, there was a bug before: if (say) src and dst were >> both unknown bytes (so range 0 to 255), it would compute the new min and max >> to be 0, so it would think the result is known to be 0. But that's wrong, >> because it could be anything from -255 to +255. The bug's implications are >> that it could be used to construct an out-of-range offset to (say) a map >> pointer which the verifier would think was in-range and thus accept. > This sounds like a serious bug that may need to be backported to stable > versions, no? In this case I would assume it should be in a separate patch > so it could be applied separately. Having looked deeper into this in attempting to create a test that the existing verifier would fail, it turns out that in the existing verifier that BPF_SUB handling is dead code. If (for instance) we subtract an UNKNOWN_VALUE from a PTR_TO_MAP_VALUE_ADJ, that code will be run, but afterwards we will mark_reg_unknown_value() the register (bottom of check_alu_op()) making our previous min/max determination irrelevant. So there's nothing to backport, and if I did change this in its own patch, there'd be no way to test it. (I have, however, added a test covering this codepath in the new verifier.) -Ed