From patchwork Sun Jan 1 22:57:56 2017 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Ian Kumlien X-Patchwork-Id: 710090 X-Patchwork-Delegate: davem@davemloft.net 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 3tsFxV3gslz9sR9 for ; Mon, 2 Jan 2017 09:58:38 +1100 (AEDT) Authentication-Results: ozlabs.org; dkim=pass (2048-bit key; unprotected) header.d=gmail.com header.i=@gmail.com header.b="UyqUNVJu"; dkim-atps=neutral Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1755226AbdAAW6S (ORCPT ); Sun, 1 Jan 2017 17:58:18 -0500 Received: from mail-oi0-f66.google.com ([209.85.218.66]:35708 "EHLO mail-oi0-f66.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1755152AbdAAW6R (ORCPT ); Sun, 1 Jan 2017 17:58:17 -0500 Received: by mail-oi0-f66.google.com with SMTP id v84so64401744oie.2; Sun, 01 Jan 2017 14:58:17 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=mime-version:in-reply-to:references:from:date:message-id:subject:to :cc; bh=dsTYnHc5i+vkquvXJkyteVJh6cfHnf50Y8RncvEXRE0=; b=UyqUNVJujctATPGTZhkAhJ0lz0X2ZFYg4qMxklIMt1COB+grVrgt2xLy0RMxFz6m9C dnvBggMw/vrGDtH7VmsftVO1x2dFpdC3tCBJEMCilgiX27FriGXwemm0tA6XQdImsNbe zTEXjj+ZIuhun4yS8tqfhRq07P+ZarDohydFsCm3jS6x92lgvHswSOfxicq1bcMNjcZc wWRo0vHRSVO9TlCUGfi/3tgy3Tsfx1vr1awsnpCr67Jx78PojhIcQbFWcOV52zfSRbgf 5tNNXMcPy7MVPoopczLDbbWZ6ds5GwV7t0MRYir1DTyHBkvPb/WdDeTiByQCqe4QkyNO q9HQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:in-reply-to:references:from:date :message-id:subject:to:cc; bh=dsTYnHc5i+vkquvXJkyteVJh6cfHnf50Y8RncvEXRE0=; b=KNL7Sv7qdQkyFdFUBaxskw4PZa0GfsxCyiedS49nbMt6D1//7mbc05aIjHYk6d1OqH TAqYIxGf2PrUBgG3YvBpgC4eEp88kBW7HSX3qUUW4r1GsxWzTXAjXuOVYtMzab58z1LC rPQhpiMGp03f1IUUS6m2eHC7qeOqCy4ChdKSmNt7lqMEt2KvXIppURUAjJoy2+/PRUr0 oXafUlIXuGao2TQU6I5X0KvguGR1UUFy1qQVYgoQc5D4tRjWF0D6/RBHKGba5b0ayNJ2 W1cEocy1rtuA1NrivEosSjrAyxFjqVRarvloVMr5a12JyoYXPLKtgKBMz3Wt1yogdvrV J7cQ== X-Gm-Message-State: AIkVDXIdpEuY1tslofI5X0vpAd3NCslujkwF/IaLcCwRQuhtt4PaB32j95G7u921pKytseXzupmpq9/2fkmCWg== X-Received: by 10.157.37.241 with SMTP id q104mr27159320ota.195.1483311496578; Sun, 01 Jan 2017 14:58:16 -0800 (PST) MIME-Version: 1.0 Received: by 10.182.215.167 with HTTP; Sun, 1 Jan 2017 14:57:56 -0800 (PST) In-Reply-To: References: From: Ian Kumlien Date: Sun, 1 Jan 2017 23:57:56 +0100 Message-ID: Subject: Re: [BUG] 4.9 - kernel oops when pptp connection is established and the kernel doesn't have pptp modules compiled To: "linux-kernel@vger.kernel.org" Cc: Ian Kumlien , netdev@vger.kernel.org Sender: netdev-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: netdev@vger.kernel.org Sorry, after further inspection, it seems like the choice of skb_header_pointer is wrong and skb isn't always populated. It seems like the code has suffered from bitrot since all the surrounding code is actually fixed. This fixes it as well: goto out_bad; Will send a patch with signed off by n' all. On Sun, Jan 1, 2017 at 6:31 PM, Ian Kumlien wrote: > On Fri, Dec 30, 2016 at 11:48 PM, Ian Kumlien wrote: >> Hi, >> >> Been fighting with "crash" to get it to help me to analyze my crash >> dumps... This is the output from vmcore-dmesg. >> >> This is 100% reproducible... >> >> Config that lets the connection trough but crashes the kernel: >> # CONFIG_NF_CONNTRACK_PPTP is not set >> # CONFIG_NF_NAT_PPTP is not set >> CONFIG_PPTP=y >> >> If I enable the *_NF_* options, it doesn't crash but it also blocks >> the PPTP packets. >> >> The crash is after the negotiation bit... > > So, some of the dumps pointed me, after some coaxing, to > net/core/flow_dissector.c:448 > --- > ppp_hdr = skb_header_pointer(skb, nhoff + offset, > sizeof(_ppp_hdr), > _ppp_hdr); > if (!ppp_hdr) > goto out_bad; > -- > > Ie, copy or get the information from the skb to get more information > on the pptp connection. > > However include/linux/skbuff.h:3109, with my test and debug code added > static inline void * __must_check > __skb_header_pointer(const struct sk_buff *skb, int offset, > int len, void *data, int hlen, void *buffer) > { > if (hlen - offset >= len) > { > if (skb == NULL || data == NULL) > { > printk("WARNING: something is null skb:%p > data:%p - offset: %i hlen: %i len: %i\n", skb, data, offset, hlen, > len); > return NULL; > } > else > return data + offset; > } > > if (!skb || > skb_copy_bits(skb, offset, buffer, len) < 0) > return NULL; > > return buffer; > } > > static inline void * __must_check > skb_header_pointer(const struct sk_buff *skb, int offset, int len, void *buffer) > { > return __skb_header_pointer(skb, offset, len, skb->data, > skb_headlen(skb), buffer); > } > --- > > so skb_header_pointer sends skb->data as data, but we never check if > skb is *NULL* > > This does happen when we do a pptp connection: > [ 89.606712] WARNING: something is null skb: (null) > data:ffff88bccc0d4000 - offset: 14 hlen: 256 len: 20 > [ 89.613264] WARNING: something is null skb: (null) > data:ffff88bccc00f800 - offset: 14 hlen: 256 len: 20 > [ 89.621005] WARNING: something is null skb: (null) > data:ffff88bccc010800 - offset: 14 hlen: 256 len: 20 > [ 89.650479] WARNING: something is null skb: (null) > data:ffff88bccc2cb000 - offset: 14 hlen: 256 len: 20 > > So, the question is if the skb should always be there and always be > valid? In that case something like this should fix it: > static inline void * __must_check > __skb_header_pointer(const struct sk_buff *skb, int offset, > int len, void *data, int hlen, void *buffer) > { > if (!skb) > return NULL; > > if (hlen - offset >= len) > return data + offset; > > if (skb_copy_bits(skb, offset, buffer, len) < 0) > return NULL; > > return buffer; > } > --- > > Else the actual check would have to be moved to skb_header_pointer in > this case - comments? > >> [ 109.556866] BUG: unable to handle kernel NULL pointer dereference >> at 0000000000000080 >> [ 109.557102] IP: [] __skb_flow_dissect+0xa88/0xce0 >> [ 109.557263] PGD 0 >> [ 109.557338] >> [ 109.557484] Oops: 0000 [#1] SMP >> [ 109.557562] Modules linked in: chaoskey >> [ 109.557783] CPU: 2 PID: 0 Comm: swapper/2 Not tainted 4.9.0 #79 >> [ 109.557867] Hardware name: Supermicro >> A1SRM-LN7F/LN5F/A1SRM-LN7F-2758, BIOS 1.0c 11/04/2015 >> [ 109.557957] task: ffff94085c27bc00 task.stack: ffffb745c0068000 >> [ 109.558041] RIP: 0010:[] [] >> __skb_flow_dissect+0xa88/0xce0 >> [ 109.558203] RSP: 0018:ffff94087fc83d40 EFLAGS: 00010206 >> [ 109.558286] RAX: 0000000000000130 RBX: ffffffff8975bf80 RCX: ffff94084fab6800 >> [ 109.558373] RDX: 0000000000000010 RSI: 000000000000000c RDI: 0000000000000000 >> [ 109.558460] RBP: 0000000000000b88 R08: 0000000000000000 R09: 0000000000000022 >> [ 109.558547] R10: 0000000000000008 R11: ffff94087fc83e04 R12: 0000000000000000 >> [ 109.558763] R13: ffff94084fab6800 R14: ffff94087fc83e04 R15: 000000000000002f >> [ 109.558979] FS: 0000000000000000(0000) GS:ffff94087fc80000(0000) >> knlGS:0000000000000000 >> [ 109.559326] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 >> [ 109.559539] CR2: 0000000000000080 CR3: 0000000281809000 CR4: 00000000001026e0 >> [ 109.559753] Stack: >> [ 109.559957] 000000000000000c ffff94084fab6822 0000000000000001 >> ffff94085c2b5fc0 >> [ 109.560578] 0000000000000001 0000000000002000 0000000000000000 >> 0000000000000000 >> [ 109.561200] 0000000000000000 0000000000000000 0000000000000000 >> 0000000000000000 >> [ 109.561820] Call Trace: >> [ 109.562027] >> [ 109.562108] [] ? eth_get_headlen+0x7a/0xf0 >> [ 109.562522] [] ? igb_poll+0x96a/0xe80 >> [ 109.562737] [] ? net_rx_action+0x20b/0x350 >> [ 109.562953] [] ? __do_softirq+0xe8/0x280 >> [ 109.563169] [] ? irq_exit+0xaa/0xb0 >> [ 109.563382] [] ? do_IRQ+0x4b/0xc0 >> [ 109.563597] [] ? common_interrupt+0x7f/0x7f >> [ 109.563810] >> [ 109.563890] [] ? cpuidle_enter_state+0x130/0x2c0 >> [ 109.564304] [] ? cpuidle_enter_state+0x120/0x2c0 >> [ 109.564520] [] ? cpu_startup_entry+0x19f/0x1f0 >> [ 109.564737] [] ? start_secondary+0x12a/0x140 >> [ 109.564950] Code: 83 e2 20 a8 80 0f 84 60 01 00 00 c7 04 24 08 00 >> 00 00 66 85 d2 0f 84 be fe ff ff e9 69 fe ff ff 8b 34 24 89 f2 83 c2 >> 04 66 85 c0 <41> 8b 84 24 80 00 00 00 0f 49 d6 41 8d 31 01 d6 41 2b 84 >> 24 84 >> [ 109.569959] RIP [] __skb_flow_dissect+0xa88/0xce0 >> [ 109.570245] RSP >> [ 109.570453] CR2: 0000000000000080 diff --git a/net/core/flow_dissector.c b/net/core/flow_dissector.c index c6d8207ffa7e..32e4e0158846 100644 --- a/net/core/flow_dissector.c +++ b/net/core/flow_dissector.c @@ -445,8 +445,9 @@ bool __skb_flow_dissect(const struct sk_buff *skb, if (hdr->flags & GRE_ACK) offset += sizeof(((struct pptp_gre_header *)0)->ack); - ppp_hdr = skb_header_pointer(skb, nhoff + offset, - sizeof(_ppp_hdr), _ppp_hdr); + ppp_hdr = __skb_header_pointer(skb, nhoff + offset, + sizeof(_ppp_hdr), + data, hlen, _ppp_hdr); if (!ppp_hdr)