Re: [BUG] 4.9 - kernel oops when pptp connection is established and the kernel doesn't have pptp modules compiled

[Ian Kumlien]

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:
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)
                                goto out_bad;

Will send a patch with signed off by n' all.

On Sun, Jan 1, 2017 at 6:31 PM, Ian Kumlien <ian.kumlien@xxxxxxxxx> wrote:
> On Fri, Dec 30, 2016 at 11:48 PM, Ian Kumlien <ian.kumlien@xxxxxxxxx> 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: [<ffffffff88dc02f8>] __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:[<ffffffff88dc02f8>]  [<ffffffff88dc02f8>]
>> __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]  <IRQ>
>> [  109.562108]  [<ffffffff88dfb4fa>] ? eth_get_headlen+0x7a/0xf0
>> [  109.562522]  [<ffffffff88c5a35a>] ? igb_poll+0x96a/0xe80
>> [  109.562737]  [<ffffffff88dc912b>] ? net_rx_action+0x20b/0x350
>> [  109.562953]  [<ffffffff88546d68>] ? __do_softirq+0xe8/0x280
>> [  109.563169]  [<ffffffff8854704a>] ? irq_exit+0xaa/0xb0
>> [  109.563382]  [<ffffffff8847229b>] ? do_IRQ+0x4b/0xc0
>> [  109.563597]  [<ffffffff8902d4ff>] ? common_interrupt+0x7f/0x7f
>> [  109.563810]  <EOI>
>> [  109.563890]  [<ffffffff88d57530>] ? cpuidle_enter_state+0x130/0x2c0
>> [  109.564304]  [<ffffffff88d57520>] ? cpuidle_enter_state+0x120/0x2c0
>> [  109.564520]  [<ffffffff8857eacf>] ? cpu_startup_entry+0x19f/0x1f0
>> [  109.564737]  [<ffffffff8848d55a>] ? 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  [<ffffffff88dc02f8>] __skb_flow_dissect+0xa88/0xce0
>> [  109.570245]  RSP <ffff94087fc83d40>
>> [  109.570453] CR2: 0000000000000080