From patchwork Mon May 22 16:27:42 2017 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: David Miller X-Patchwork-Id: 765478 Return-Path: X-Original-To: patchwork-incoming@ozlabs.org Delivered-To: patchwork-incoming@ozlabs.org Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by ozlabs.org (Postfix) with ESMTP id 3wWkbr1mQDz9s7q for ; Tue, 23 May 2017 02:28:08 +1000 (AEST) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S933761AbdEVQ1v (ORCPT ); Mon, 22 May 2017 12:27:51 -0400 Received: from shards.monkeyblade.net ([184.105.139.130]:53922 "EHLO shards.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S933461AbdEVQ1t (ORCPT ); Mon, 22 May 2017 12:27:49 -0400 Received: from localhost (unknown [38.140.131.194]) (Authenticated sender: davem-davemloft) by shards.monkeyblade.net (Postfix) with ESMTPSA id 47B0D12320D82; Mon, 22 May 2017 08:46:10 -0700 (PDT) Date: Mon, 22 May 2017 12:27:42 -0400 (EDT) Message-Id: <20170522.122742.1326733219925627993.davem@davemloft.net> To: daniel@iogearbox.net Cc: garsilva@embeddedor.com, ast@kernel.org, netdev@vger.kernel.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH] kernel: bpf: remove dead code From: David Miller In-Reply-To: <5922FB28.5070303@iogearbox.net> References: <20170522140746.GA10113@embeddedgus> <20170522.103800.1354089494827582585.davem@davemloft.net> <5922FB28.5070303@iogearbox.net> X-Mailer: Mew version 6.7 on Emacs 24.5 / Mule 6.0 (HANACHIRUSATO) Mime-Version: 1.0 X-Greylist: Sender succeeded SMTP AUTH, not delayed by milter-greylist-4.5.12 (shards.monkeyblade.net [149.20.54.216]); Mon, 22 May 2017 08:46:10 -0700 (PDT) Sender: netdev-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: netdev@vger.kernel.org From: Daniel Borkmann Date: Mon, 22 May 2017 16:52:24 +0200 > On 05/22/2017 04:38 PM, David Miller wrote: >> From: "Gustavo A. R. Silva" >> Date: Mon, 22 May 2017 09:07:46 -0500 >> >>> Execution cannot reach NET_IP_ALIGN inside the following statement: >>> ip_align = strict ? 2 : NET_IP_ALIGN >>> >>> Addresses-Coverity-ID: 1409762 >>> Signed-off-by: Gustavo A. R. Silva >>> --- >>> NOTE: variable ip_align could also be removed and use value 2 >>> directly. >> >> Incorrect. >> >> Some platforms define NET_IP_ALIGN to zero, so the code must remain >> as is. > > In the check_pkt_ptr_alignment(), when !strict you would already > return earlier from that function. > > So, above test in ip_align will always give 2, meaning technically > the patch is correct, although hard-coded value less clean. > > Perhaps something like the below to keep intentions more clear (and > it will get resolved during compile time anyway ...): Ok I understand the issue now. Thanks for explaining. I guess a hard-coded value of 2 and an adjusted comment above the assignment of ip_align is the way to go. I'll push the following, thanks everyone: Acked-by: Daniel Borkmann ==================== net: Make IP alignment calulations clearer. The assignmnet: ip_align = strict ? 2 : NET_IP_ALIGN; in compare_pkt_ptr_alignment() trips up Coverity because we can only get to this code when strict is true, therefore ip_align will always be 2 regardless of NET_IP_ALIGN's value. So just assign directly to '2' and explain the situation in the comment above. Reported-by: "Gustavo A. R. Silva" Signed-off-by: David S. Miller --- kernel/bpf/verifier.c | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c index 1eddb71..c72cd41 100644 --- a/kernel/bpf/verifier.c +++ b/kernel/bpf/verifier.c @@ -808,11 +808,15 @@ static int check_pkt_ptr_alignment(const struct bpf_reg_state *reg, reg_off += reg->aux_off; } - /* skb->data is NET_IP_ALIGN-ed, but for strict alignment checking - * we force this to 2 which is universally what architectures use - * when they don't set CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS. + /* For platforms that do not have a Kconfig enabling + * CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS the value of + * NET_IP_ALIGN is universally set to '2'. And on platforms + * that do set CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS, we get + * to this code only in strict mode where we want to emulate + * the NET_IP_ALIGN==2 checking. Therefore use an + * unconditional IP align value of '2'. */ - ip_align = strict ? 2 : NET_IP_ALIGN; + ip_align = 2; if ((ip_align + reg_off + off) % size != 0) { verbose("misaligned packet access off %d+%d+%d size %d\n", ip_align, reg_off, off, size);