[Xenial,REGN] brcmfmac: add eth_type_trans back for PCIe full dongle
diff mbox series

Message ID 20190717085707.9529-1-stefan.bader@canonical.com
State New
Headers show
Series
  • [Xenial,REGN] brcmfmac: add eth_type_trans back for PCIe full dongle
Related show

Commit Message

Stefan Bader July 17, 2019, 8:57 a.m. UTC
From: Franky Lin <franky.lin@broadcom.com>

BugLink: https://bugs.launchpad.net/bugs/1836801

A regression was introduced in commit 9c349892ccc9 ("brcmfmac: revise
handling events in receive path") which moves eth_type_trans() call
to brcmf_rx_frame(). Msgbuf layer doesn't use brcmf_rx_frame() but invokes
brcmf_netif_rx() directly. In such case the Ethernet header was not
stripped out resulting in null pointer dereference in the networking
stack.

BUG: unable to handle kernel NULL pointer dereference at 0000000000000048
IP: [<ffffffff814c3ce6>] enqueue_to_backlog+0x56/0x260
PGD 0
Oops: 0000 [#1] PREEMPT SMP
Modules linked in: fuse ipt_MASQUERADE nf_nat_masquerade_ipv4
iptable_nat nf_conntrack_ipv4 nf_defrag_ipv4 nf_nat_ipv4 xt_addrtype
[...]
rtsx_pci scsi_mod usbcore usb_common i8042 serio nvme nvme_core
CPU: 7 PID: 1340 Comm: irq/136-brcmf_p Not tainted 4.7.0-rc1-mainline #1
Hardware name: Dell Inc. XPS 15 9550/0N7TVV, BIOS 01.02.00 04/07/2016
task: ffff8804a0c5bd00 ti: ffff88049e124000 task.ti: ffff88049e124000
RIP: 0010:[<ffffffff814c3ce6>] [<ffffffff814c3ce6>]
enqueue_to_backlog+0x56/0x260
RSP: 0018:ffff88049e127ca0 EFLAGS: 00010046
RAX: 0000000000000000 RBX: ffff8804bddd7c40 RCX: 000000000000002f
RDX: 0000000000000000 RSI: 0000000000000007 RDI: ffff8804bddd7d4c
RBP: ffff88049e127ce8 R08: 0000000000000000 R09: 0000000000000000
R10: ffff8804bddd12c0 R11: 000000000000149e R12: 0000000000017c40
R13: ffff88049e127d08 R14: ffff8804a9bd6d00 R15: ffff8804bddd7d4c
FS: 0000000000000000(0000) GS:ffff8804bddc0000(0000) knlGS:0000000000000000
CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: 0000000000000048 CR3: 0000000001806000 CR4: 00000000003406e0
DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
Stack:
ffff8804bdddad00 ffff8804ad089e00 0000000000000000 0000000000000282
0000000000000000 ffff8804a9bd6d00 ffff8804a1b27e00 ffff8804a9bd6d00
ffff88002ee88000 ffff88049e127d28 ffffffff814c3f3b ffffffff81311fc3
Call Trace:
[<ffffffff814c3f3b>] netif_rx_internal+0x4b/0x170
[<ffffffff81311fc3>] ? swiotlb_tbl_unmap_single+0xf3/0x120
[<ffffffff814c5467>] netif_rx_ni+0x27/0xc0
[<ffffffffa08519e9>] brcmf_netif_rx+0x49/0x70 [brcmfmac]
[<ffffffffa08564d4>] brcmf_msgbuf_process_rx+0x2b4/0x570 [brcmfmac]
[<ffffffff81020017>] ? __xen_set_pgd_hyper+0x57/0xd0
[<ffffffff810d60b0>] ? irq_forced_thread_fn+0x70/0x70
[<ffffffffa0857381>] brcmf_proto_msgbuf_rx_trigger+0x31/0xe0 [brcmfmac]
[<ffffffffa0861e8f>] brcmf_pcie_isr_thread+0x7f/0x110 [brcmfmac]
[<ffffffff810d60d0>] irq_thread_fn+0x20/0x50
[<ffffffff810d63ad>] irq_thread+0x12d/0x1c0
[<ffffffff815d07d5>] ? __schedule+0x2f5/0x7a0
[<ffffffff810d61d0>] ? wake_threads_waitq+0x30/0x30
[<ffffffff810d6280>] ? irq_thread_dtor+0xb0/0xb0
[<ffffffff81098ea8>] kthread+0xd8/0xf0
[<ffffffff815d4b7f>] ret_from_fork+0x1f/0x40
[<ffffffff81098dd0>] ? kthread_worker_fn+0x170/0x170
Code: 1c f5 60 9a 8e 81 9c 58 0f 1f 44 00 00 48 89 45 d0 fa 66 0f 1f
44 00 00 4c 8d bb 0c 01 00 00 4c 89 ff e8 5e 08 11 00 49 8b 56 20 <48>
8b 52 48 83 e2 01 74 10 8b 8b 08 01 00 00 8b 15 59 c5 42 00
RIP [<ffffffff814c3ce6>] enqueue_to_backlog+0x56/0x260
RSP <ffff88049e127ca0>
CR2: 0000000000000048

Fixes: 9c349892ccc9 ("brcmfmac: revise handling events in receive path")
Reported-by: Rafal Milecki <zajec5@gmail.com>
Reported-by: Grey Christoforo <grey@christoforo.net>
Reviewed-by: Pieter-Paul Giesberts <pieter-paul.giesberts@broadcom.com>
Reviewed-by: Arend Van Spriel <arend@broadcom.com>
Reviewed-by: Hante Meuleman <hante.meuleman@broadcom.com>
Signed-off-by: Franky Lin <franky.lin@broadcom.com>
[arend@broadcom.com: rephrased the commit message]
Signed-off-by: Arend van Spriel <arend@broadcom.com>
Signed-off-by: Kalle Valo <kvalo@codeaurora.org>

(backported from commit 31143e2933d1675c4c1ba6ce125cdd95870edd85)
[smb: file -> drivers/net/wireless/brcm80211/brcmfmac/msgbuf.c
      brcmf_msgbuf_process_rx_complete -> brcmf_msgbuf_process_rx]
Signed-off-by: Stefan Bader <stefan.bader@canonical.com>
---
 drivers/net/wireless/brcm80211/brcmfmac/msgbuf.c | 2 ++
 1 file changed, 2 insertions(+)

Comments

Kleber Souza July 17, 2019, 9:29 a.m. UTC | #1
On 7/17/19 10:57 AM, Stefan Bader wrote:
> From: Franky Lin <franky.lin@broadcom.com>
> 
> BugLink: https://bugs.launchpad.net/bugs/1836801
> 
> A regression was introduced in commit 9c349892ccc9 ("brcmfmac: revise
> handling events in receive path") which moves eth_type_trans() call
> to brcmf_rx_frame(). Msgbuf layer doesn't use brcmf_rx_frame() but invokes
> brcmf_netif_rx() directly. In such case the Ethernet header was not
> stripped out resulting in null pointer dereference in the networking
> stack.
> 
> BUG: unable to handle kernel NULL pointer dereference at 0000000000000048
> IP: [<ffffffff814c3ce6>] enqueue_to_backlog+0x56/0x260
> PGD 0
> Oops: 0000 [#1] PREEMPT SMP
> Modules linked in: fuse ipt_MASQUERADE nf_nat_masquerade_ipv4
> iptable_nat nf_conntrack_ipv4 nf_defrag_ipv4 nf_nat_ipv4 xt_addrtype
> [...]
> rtsx_pci scsi_mod usbcore usb_common i8042 serio nvme nvme_core
> CPU: 7 PID: 1340 Comm: irq/136-brcmf_p Not tainted 4.7.0-rc1-mainline #1
> Hardware name: Dell Inc. XPS 15 9550/0N7TVV, BIOS 01.02.00 04/07/2016
> task: ffff8804a0c5bd00 ti: ffff88049e124000 task.ti: ffff88049e124000
> RIP: 0010:[<ffffffff814c3ce6>] [<ffffffff814c3ce6>]
> enqueue_to_backlog+0x56/0x260
> RSP: 0018:ffff88049e127ca0 EFLAGS: 00010046
> RAX: 0000000000000000 RBX: ffff8804bddd7c40 RCX: 000000000000002f
> RDX: 0000000000000000 RSI: 0000000000000007 RDI: ffff8804bddd7d4c
> RBP: ffff88049e127ce8 R08: 0000000000000000 R09: 0000000000000000
> R10: ffff8804bddd12c0 R11: 000000000000149e R12: 0000000000017c40
> R13: ffff88049e127d08 R14: ffff8804a9bd6d00 R15: ffff8804bddd7d4c
> FS: 0000000000000000(0000) GS:ffff8804bddc0000(0000) knlGS:0000000000000000
> CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> CR2: 0000000000000048 CR3: 0000000001806000 CR4: 00000000003406e0
> DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
> DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
> Stack:
> ffff8804bdddad00 ffff8804ad089e00 0000000000000000 0000000000000282
> 0000000000000000 ffff8804a9bd6d00 ffff8804a1b27e00 ffff8804a9bd6d00
> ffff88002ee88000 ffff88049e127d28 ffffffff814c3f3b ffffffff81311fc3
> Call Trace:
> [<ffffffff814c3f3b>] netif_rx_internal+0x4b/0x170
> [<ffffffff81311fc3>] ? swiotlb_tbl_unmap_single+0xf3/0x120
> [<ffffffff814c5467>] netif_rx_ni+0x27/0xc0
> [<ffffffffa08519e9>] brcmf_netif_rx+0x49/0x70 [brcmfmac]
> [<ffffffffa08564d4>] brcmf_msgbuf_process_rx+0x2b4/0x570 [brcmfmac]
> [<ffffffff81020017>] ? __xen_set_pgd_hyper+0x57/0xd0
> [<ffffffff810d60b0>] ? irq_forced_thread_fn+0x70/0x70
> [<ffffffffa0857381>] brcmf_proto_msgbuf_rx_trigger+0x31/0xe0 [brcmfmac]
> [<ffffffffa0861e8f>] brcmf_pcie_isr_thread+0x7f/0x110 [brcmfmac]
> [<ffffffff810d60d0>] irq_thread_fn+0x20/0x50
> [<ffffffff810d63ad>] irq_thread+0x12d/0x1c0
> [<ffffffff815d07d5>] ? __schedule+0x2f5/0x7a0
> [<ffffffff810d61d0>] ? wake_threads_waitq+0x30/0x30
> [<ffffffff810d6280>] ? irq_thread_dtor+0xb0/0xb0
> [<ffffffff81098ea8>] kthread+0xd8/0xf0
> [<ffffffff815d4b7f>] ret_from_fork+0x1f/0x40
> [<ffffffff81098dd0>] ? kthread_worker_fn+0x170/0x170
> Code: 1c f5 60 9a 8e 81 9c 58 0f 1f 44 00 00 48 89 45 d0 fa 66 0f 1f
> 44 00 00 4c 8d bb 0c 01 00 00 4c 89 ff e8 5e 08 11 00 49 8b 56 20 <48>
> 8b 52 48 83 e2 01 74 10 8b 8b 08 01 00 00 8b 15 59 c5 42 00
> RIP [<ffffffff814c3ce6>] enqueue_to_backlog+0x56/0x260
> RSP <ffff88049e127ca0>
> CR2: 0000000000000048
> 
> Fixes: 9c349892ccc9 ("brcmfmac: revise handling events in receive path")
> Reported-by: Rafal Milecki <zajec5@gmail.com>
> Reported-by: Grey Christoforo <grey@christoforo.net>
> Reviewed-by: Pieter-Paul Giesberts <pieter-paul.giesberts@broadcom.com>
> Reviewed-by: Arend Van Spriel <arend@broadcom.com>
> Reviewed-by: Hante Meuleman <hante.meuleman@broadcom.com>
> Signed-off-by: Franky Lin <franky.lin@broadcom.com>
> [arend@broadcom.com: rephrased the commit message]
> Signed-off-by: Arend van Spriel <arend@broadcom.com>
> Signed-off-by: Kalle Valo <kvalo@codeaurora.org>
> 
> (backported from commit 31143e2933d1675c4c1ba6ce125cdd95870edd85)
> [smb: file -> drivers/net/wireless/brcm80211/brcmfmac/msgbuf.c
>       brcmf_msgbuf_process_rx_complete -> brcmf_msgbuf_process_rx]

Did the function name really need adjustment?

Acked-by: Kleber Sacilotto de Souza <kleber.souza@canonical.com>


> Signed-off-by: Stefan Bader <stefan.bader@canonical.com>
> ---
>  drivers/net/wireless/brcm80211/brcmfmac/msgbuf.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/net/wireless/brcm80211/brcmfmac/msgbuf.c b/drivers/net/wireless/brcm80211/brcmfmac/msgbuf.c
> index 6f7138cea555..f944f356d9c5 100644
> --- a/drivers/net/wireless/brcm80211/brcmfmac/msgbuf.c
> +++ b/drivers/net/wireless/brcm80211/brcmfmac/msgbuf.c
> @@ -1155,6 +1155,8 @@ brcmf_msgbuf_process_rx_complete(struct brcmf_msgbuf *msgbuf, void *buf)
>  		brcmu_pkt_buf_free_skb(skb);
>  		return;
>  	}
> +
> +	skb->protocol = eth_type_trans(skb, ifp->ndev);
>  	brcmf_netif_rx(ifp, skb);
>  }
>  
>
Stefan Bader July 17, 2019, 9:32 a.m. UTC | #2
On 17.07.19 11:29, Kleber Souza wrote:
> On 7/17/19 10:57 AM, Stefan Bader wrote:
>> From: Franky Lin <franky.lin@broadcom.com>
>>
>> BugLink: https://bugs.launchpad.net/bugs/1836801
>>
>> A regression was introduced in commit 9c349892ccc9 ("brcmfmac: revise
>> handling events in receive path") which moves eth_type_trans() call
>> to brcmf_rx_frame(). Msgbuf layer doesn't use brcmf_rx_frame() but invokes
>> brcmf_netif_rx() directly. In such case the Ethernet header was not
>> stripped out resulting in null pointer dereference in the networking
>> stack.
>>
>> BUG: unable to handle kernel NULL pointer dereference at 0000000000000048
>> IP: [<ffffffff814c3ce6>] enqueue_to_backlog+0x56/0x260
>> PGD 0
>> Oops: 0000 [#1] PREEMPT SMP
>> Modules linked in: fuse ipt_MASQUERADE nf_nat_masquerade_ipv4
>> iptable_nat nf_conntrack_ipv4 nf_defrag_ipv4 nf_nat_ipv4 xt_addrtype
>> [...]
>> rtsx_pci scsi_mod usbcore usb_common i8042 serio nvme nvme_core
>> CPU: 7 PID: 1340 Comm: irq/136-brcmf_p Not tainted 4.7.0-rc1-mainline #1
>> Hardware name: Dell Inc. XPS 15 9550/0N7TVV, BIOS 01.02.00 04/07/2016
>> task: ffff8804a0c5bd00 ti: ffff88049e124000 task.ti: ffff88049e124000
>> RIP: 0010:[<ffffffff814c3ce6>] [<ffffffff814c3ce6>]
>> enqueue_to_backlog+0x56/0x260
>> RSP: 0018:ffff88049e127ca0 EFLAGS: 00010046
>> RAX: 0000000000000000 RBX: ffff8804bddd7c40 RCX: 000000000000002f
>> RDX: 0000000000000000 RSI: 0000000000000007 RDI: ffff8804bddd7d4c
>> RBP: ffff88049e127ce8 R08: 0000000000000000 R09: 0000000000000000
>> R10: ffff8804bddd12c0 R11: 000000000000149e R12: 0000000000017c40
>> R13: ffff88049e127d08 R14: ffff8804a9bd6d00 R15: ffff8804bddd7d4c
>> FS: 0000000000000000(0000) GS:ffff8804bddc0000(0000) knlGS:0000000000000000
>> CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
>> CR2: 0000000000000048 CR3: 0000000001806000 CR4: 00000000003406e0
>> DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
>> DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
>> Stack:
>> ffff8804bdddad00 ffff8804ad089e00 0000000000000000 0000000000000282
>> 0000000000000000 ffff8804a9bd6d00 ffff8804a1b27e00 ffff8804a9bd6d00
>> ffff88002ee88000 ffff88049e127d28 ffffffff814c3f3b ffffffff81311fc3
>> Call Trace:
>> [<ffffffff814c3f3b>] netif_rx_internal+0x4b/0x170
>> [<ffffffff81311fc3>] ? swiotlb_tbl_unmap_single+0xf3/0x120
>> [<ffffffff814c5467>] netif_rx_ni+0x27/0xc0
>> [<ffffffffa08519e9>] brcmf_netif_rx+0x49/0x70 [brcmfmac]
>> [<ffffffffa08564d4>] brcmf_msgbuf_process_rx+0x2b4/0x570 [brcmfmac]
>> [<ffffffff81020017>] ? __xen_set_pgd_hyper+0x57/0xd0
>> [<ffffffff810d60b0>] ? irq_forced_thread_fn+0x70/0x70
>> [<ffffffffa0857381>] brcmf_proto_msgbuf_rx_trigger+0x31/0xe0 [brcmfmac]
>> [<ffffffffa0861e8f>] brcmf_pcie_isr_thread+0x7f/0x110 [brcmfmac]
>> [<ffffffff810d60d0>] irq_thread_fn+0x20/0x50
>> [<ffffffff810d63ad>] irq_thread+0x12d/0x1c0
>> [<ffffffff815d07d5>] ? __schedule+0x2f5/0x7a0
>> [<ffffffff810d61d0>] ? wake_threads_waitq+0x30/0x30
>> [<ffffffff810d6280>] ? irq_thread_dtor+0xb0/0xb0
>> [<ffffffff81098ea8>] kthread+0xd8/0xf0
>> [<ffffffff815d4b7f>] ret_from_fork+0x1f/0x40
>> [<ffffffff81098dd0>] ? kthread_worker_fn+0x170/0x170
>> Code: 1c f5 60 9a 8e 81 9c 58 0f 1f 44 00 00 48 89 45 d0 fa 66 0f 1f
>> 44 00 00 4c 8d bb 0c 01 00 00 4c 89 ff e8 5e 08 11 00 49 8b 56 20 <48>
>> 8b 52 48 83 e2 01 74 10 8b 8b 08 01 00 00 8b 15 59 c5 42 00
>> RIP [<ffffffff814c3ce6>] enqueue_to_backlog+0x56/0x260
>> RSP <ffff88049e127ca0>
>> CR2: 0000000000000048
>>
>> Fixes: 9c349892ccc9 ("brcmfmac: revise handling events in receive path")
>> Reported-by: Rafal Milecki <zajec5@gmail.com>
>> Reported-by: Grey Christoforo <grey@christoforo.net>
>> Reviewed-by: Pieter-Paul Giesberts <pieter-paul.giesberts@broadcom.com>
>> Reviewed-by: Arend Van Spriel <arend@broadcom.com>
>> Reviewed-by: Hante Meuleman <hante.meuleman@broadcom.com>
>> Signed-off-by: Franky Lin <franky.lin@broadcom.com>
>> [arend@broadcom.com: rephrased the commit message]
>> Signed-off-by: Arend van Spriel <arend@broadcom.com>
>> Signed-off-by: Kalle Valo <kvalo@codeaurora.org>
>>
>> (backported from commit 31143e2933d1675c4c1ba6ce125cdd95870edd85)
>> [smb: file -> drivers/net/wireless/brcm80211/brcmfmac/msgbuf.c
>>       brcmf_msgbuf_process_rx_complete -> brcmf_msgbuf_process_rx]
> 
> Did the function name really need adjustment?

I meant to say I applied the code to a different file and a different function
than in the original patch.

-Stefan

> 
> Acked-by: Kleber Sacilotto de Souza <kleber.souza@canonical.com>
> 
> 
>> Signed-off-by: Stefan Bader <stefan.bader@canonical.com>
>> ---
>>  drivers/net/wireless/brcm80211/brcmfmac/msgbuf.c | 2 ++
>>  1 file changed, 2 insertions(+)
>>
>> diff --git a/drivers/net/wireless/brcm80211/brcmfmac/msgbuf.c b/drivers/net/wireless/brcm80211/brcmfmac/msgbuf.c
>> index 6f7138cea555..f944f356d9c5 100644
>> --- a/drivers/net/wireless/brcm80211/brcmfmac/msgbuf.c
>> +++ b/drivers/net/wireless/brcm80211/brcmfmac/msgbuf.c
>> @@ -1155,6 +1155,8 @@ brcmf_msgbuf_process_rx_complete(struct brcmf_msgbuf *msgbuf, void *buf)
>>  		brcmu_pkt_buf_free_skb(skb);
>>  		return;
>>  	}
>> +
>> +	skb->protocol = eth_type_trans(skb, ifp->ndev);
>>  	brcmf_netif_rx(ifp, skb);
>>  }
>>  
>>
>
Andy Whitcroft July 17, 2019, 9:44 a.m. UTC | #3
On Wed, Jul 17, 2019 at 10:57:07AM +0200, Stefan Bader wrote:
> From: Franky Lin <franky.lin@broadcom.com>
> 
> BugLink: https://bugs.launchpad.net/bugs/1836801
> 
> A regression was introduced in commit 9c349892ccc9 ("brcmfmac: revise
> handling events in receive path") which moves eth_type_trans() call
> to brcmf_rx_frame(). Msgbuf layer doesn't use brcmf_rx_frame() but invokes
> brcmf_netif_rx() directly. In such case the Ethernet header was not
> stripped out resulting in null pointer dereference in the networking
> stack.
> 
> BUG: unable to handle kernel NULL pointer dereference at 0000000000000048
> IP: [<ffffffff814c3ce6>] enqueue_to_backlog+0x56/0x260
> PGD 0
> Oops: 0000 [#1] PREEMPT SMP
> Modules linked in: fuse ipt_MASQUERADE nf_nat_masquerade_ipv4
> iptable_nat nf_conntrack_ipv4 nf_defrag_ipv4 nf_nat_ipv4 xt_addrtype
> [...]
> rtsx_pci scsi_mod usbcore usb_common i8042 serio nvme nvme_core
> CPU: 7 PID: 1340 Comm: irq/136-brcmf_p Not tainted 4.7.0-rc1-mainline #1
> Hardware name: Dell Inc. XPS 15 9550/0N7TVV, BIOS 01.02.00 04/07/2016
> task: ffff8804a0c5bd00 ti: ffff88049e124000 task.ti: ffff88049e124000
> RIP: 0010:[<ffffffff814c3ce6>] [<ffffffff814c3ce6>]
> enqueue_to_backlog+0x56/0x260
> RSP: 0018:ffff88049e127ca0 EFLAGS: 00010046
> RAX: 0000000000000000 RBX: ffff8804bddd7c40 RCX: 000000000000002f
> RDX: 0000000000000000 RSI: 0000000000000007 RDI: ffff8804bddd7d4c
> RBP: ffff88049e127ce8 R08: 0000000000000000 R09: 0000000000000000
> R10: ffff8804bddd12c0 R11: 000000000000149e R12: 0000000000017c40
> R13: ffff88049e127d08 R14: ffff8804a9bd6d00 R15: ffff8804bddd7d4c
> FS: 0000000000000000(0000) GS:ffff8804bddc0000(0000) knlGS:0000000000000000
> CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> CR2: 0000000000000048 CR3: 0000000001806000 CR4: 00000000003406e0
> DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
> DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
> Stack:
> ffff8804bdddad00 ffff8804ad089e00 0000000000000000 0000000000000282
> 0000000000000000 ffff8804a9bd6d00 ffff8804a1b27e00 ffff8804a9bd6d00
> ffff88002ee88000 ffff88049e127d28 ffffffff814c3f3b ffffffff81311fc3
> Call Trace:
> [<ffffffff814c3f3b>] netif_rx_internal+0x4b/0x170
> [<ffffffff81311fc3>] ? swiotlb_tbl_unmap_single+0xf3/0x120
> [<ffffffff814c5467>] netif_rx_ni+0x27/0xc0
> [<ffffffffa08519e9>] brcmf_netif_rx+0x49/0x70 [brcmfmac]
> [<ffffffffa08564d4>] brcmf_msgbuf_process_rx+0x2b4/0x570 [brcmfmac]
> [<ffffffff81020017>] ? __xen_set_pgd_hyper+0x57/0xd0
> [<ffffffff810d60b0>] ? irq_forced_thread_fn+0x70/0x70
> [<ffffffffa0857381>] brcmf_proto_msgbuf_rx_trigger+0x31/0xe0 [brcmfmac]
> [<ffffffffa0861e8f>] brcmf_pcie_isr_thread+0x7f/0x110 [brcmfmac]
> [<ffffffff810d60d0>] irq_thread_fn+0x20/0x50
> [<ffffffff810d63ad>] irq_thread+0x12d/0x1c0
> [<ffffffff815d07d5>] ? __schedule+0x2f5/0x7a0
> [<ffffffff810d61d0>] ? wake_threads_waitq+0x30/0x30
> [<ffffffff810d6280>] ? irq_thread_dtor+0xb0/0xb0
> [<ffffffff81098ea8>] kthread+0xd8/0xf0
> [<ffffffff815d4b7f>] ret_from_fork+0x1f/0x40
> [<ffffffff81098dd0>] ? kthread_worker_fn+0x170/0x170
> Code: 1c f5 60 9a 8e 81 9c 58 0f 1f 44 00 00 48 89 45 d0 fa 66 0f 1f
> 44 00 00 4c 8d bb 0c 01 00 00 4c 89 ff e8 5e 08 11 00 49 8b 56 20 <48>
> 8b 52 48 83 e2 01 74 10 8b 8b 08 01 00 00 8b 15 59 c5 42 00
> RIP [<ffffffff814c3ce6>] enqueue_to_backlog+0x56/0x260
> RSP <ffff88049e127ca0>
> CR2: 0000000000000048
> 
> Fixes: 9c349892ccc9 ("brcmfmac: revise handling events in receive path")
> Reported-by: Rafal Milecki <zajec5@gmail.com>
> Reported-by: Grey Christoforo <grey@christoforo.net>
> Reviewed-by: Pieter-Paul Giesberts <pieter-paul.giesberts@broadcom.com>
> Reviewed-by: Arend Van Spriel <arend@broadcom.com>
> Reviewed-by: Hante Meuleman <hante.meuleman@broadcom.com>
> Signed-off-by: Franky Lin <franky.lin@broadcom.com>
> [arend@broadcom.com: rephrased the commit message]
> Signed-off-by: Arend van Spriel <arend@broadcom.com>
> Signed-off-by: Kalle Valo <kvalo@codeaurora.org>
> 
> (backported from commit 31143e2933d1675c4c1ba6ce125cdd95870edd85)
> [smb: file -> drivers/net/wireless/brcm80211/brcmfmac/msgbuf.c
>       brcmf_msgbuf_process_rx_complete -> brcmf_msgbuf_process_rx]
> Signed-off-by: Stefan Bader <stefan.bader@canonical.com>
> ---
>  drivers/net/wireless/brcm80211/brcmfmac/msgbuf.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/net/wireless/brcm80211/brcmfmac/msgbuf.c b/drivers/net/wireless/brcm80211/brcmfmac/msgbuf.c
> index 6f7138cea555..f944f356d9c5 100644
> --- a/drivers/net/wireless/brcm80211/brcmfmac/msgbuf.c
> +++ b/drivers/net/wireless/brcm80211/brcmfmac/msgbuf.c
> @@ -1155,6 +1155,8 @@ brcmf_msgbuf_process_rx_complete(struct brcmf_msgbuf *msgbuf, void *buf)
>  		brcmu_pkt_buf_free_skb(skb);
>  		return;
>  	}
> +
> +	skb->protocol = eth_type_trans(skb, ifp->ndev);
>  	brcmf_netif_rx(ifp, skb);
>  }
>  

This looks to do what is claimed.  With the backport data sorted:

Acked-by: Andy Whitcroft <apw@canonical.com>
Stefan Bader July 17, 2019, 9:51 a.m. UTC | #4
On 17.07.19 11:32, Stefan Bader wrote:
> On 17.07.19 11:29, Kleber Souza wrote:
>> On 7/17/19 10:57 AM, Stefan Bader wrote:
>>> From: Franky Lin <franky.lin@broadcom.com>
>>>
>>> BugLink: https://bugs.launchpad.net/bugs/1836801
>>>
>>> A regression was introduced in commit 9c349892ccc9 ("brcmfmac: revise
>>> handling events in receive path") which moves eth_type_trans() call
>>> to brcmf_rx_frame(). Msgbuf layer doesn't use brcmf_rx_frame() but invokes
>>> brcmf_netif_rx() directly. In such case the Ethernet header was not
>>> stripped out resulting in null pointer dereference in the networking
>>> stack.
>>>
>>> BUG: unable to handle kernel NULL pointer dereference at 0000000000000048
>>> IP: [<ffffffff814c3ce6>] enqueue_to_backlog+0x56/0x260
>>> PGD 0
>>> Oops: 0000 [#1] PREEMPT SMP
>>> Modules linked in: fuse ipt_MASQUERADE nf_nat_masquerade_ipv4
>>> iptable_nat nf_conntrack_ipv4 nf_defrag_ipv4 nf_nat_ipv4 xt_addrtype
>>> [...]
>>> rtsx_pci scsi_mod usbcore usb_common i8042 serio nvme nvme_core
>>> CPU: 7 PID: 1340 Comm: irq/136-brcmf_p Not tainted 4.7.0-rc1-mainline #1
>>> Hardware name: Dell Inc. XPS 15 9550/0N7TVV, BIOS 01.02.00 04/07/2016
>>> task: ffff8804a0c5bd00 ti: ffff88049e124000 task.ti: ffff88049e124000
>>> RIP: 0010:[<ffffffff814c3ce6>] [<ffffffff814c3ce6>]
>>> enqueue_to_backlog+0x56/0x260
>>> RSP: 0018:ffff88049e127ca0 EFLAGS: 00010046
>>> RAX: 0000000000000000 RBX: ffff8804bddd7c40 RCX: 000000000000002f
>>> RDX: 0000000000000000 RSI: 0000000000000007 RDI: ffff8804bddd7d4c
>>> RBP: ffff88049e127ce8 R08: 0000000000000000 R09: 0000000000000000
>>> R10: ffff8804bddd12c0 R11: 000000000000149e R12: 0000000000017c40
>>> R13: ffff88049e127d08 R14: ffff8804a9bd6d00 R15: ffff8804bddd7d4c
>>> FS: 0000000000000000(0000) GS:ffff8804bddc0000(0000) knlGS:0000000000000000
>>> CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
>>> CR2: 0000000000000048 CR3: 0000000001806000 CR4: 00000000003406e0
>>> DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
>>> DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
>>> Stack:
>>> ffff8804bdddad00 ffff8804ad089e00 0000000000000000 0000000000000282
>>> 0000000000000000 ffff8804a9bd6d00 ffff8804a1b27e00 ffff8804a9bd6d00
>>> ffff88002ee88000 ffff88049e127d28 ffffffff814c3f3b ffffffff81311fc3
>>> Call Trace:
>>> [<ffffffff814c3f3b>] netif_rx_internal+0x4b/0x170
>>> [<ffffffff81311fc3>] ? swiotlb_tbl_unmap_single+0xf3/0x120
>>> [<ffffffff814c5467>] netif_rx_ni+0x27/0xc0
>>> [<ffffffffa08519e9>] brcmf_netif_rx+0x49/0x70 [brcmfmac]
>>> [<ffffffffa08564d4>] brcmf_msgbuf_process_rx+0x2b4/0x570 [brcmfmac]
>>> [<ffffffff81020017>] ? __xen_set_pgd_hyper+0x57/0xd0
>>> [<ffffffff810d60b0>] ? irq_forced_thread_fn+0x70/0x70
>>> [<ffffffffa0857381>] brcmf_proto_msgbuf_rx_trigger+0x31/0xe0 [brcmfmac]
>>> [<ffffffffa0861e8f>] brcmf_pcie_isr_thread+0x7f/0x110 [brcmfmac]
>>> [<ffffffff810d60d0>] irq_thread_fn+0x20/0x50
>>> [<ffffffff810d63ad>] irq_thread+0x12d/0x1c0
>>> [<ffffffff815d07d5>] ? __schedule+0x2f5/0x7a0
>>> [<ffffffff810d61d0>] ? wake_threads_waitq+0x30/0x30
>>> [<ffffffff810d6280>] ? irq_thread_dtor+0xb0/0xb0
>>> [<ffffffff81098ea8>] kthread+0xd8/0xf0
>>> [<ffffffff815d4b7f>] ret_from_fork+0x1f/0x40
>>> [<ffffffff81098dd0>] ? kthread_worker_fn+0x170/0x170
>>> Code: 1c f5 60 9a 8e 81 9c 58 0f 1f 44 00 00 48 89 45 d0 fa 66 0f 1f
>>> 44 00 00 4c 8d bb 0c 01 00 00 4c 89 ff e8 5e 08 11 00 49 8b 56 20 <48>
>>> 8b 52 48 83 e2 01 74 10 8b 8b 08 01 00 00 8b 15 59 c5 42 00
>>> RIP [<ffffffff814c3ce6>] enqueue_to_backlog+0x56/0x260
>>> RSP <ffff88049e127ca0>
>>> CR2: 0000000000000048
>>>
>>> Fixes: 9c349892ccc9 ("brcmfmac: revise handling events in receive path")
>>> Reported-by: Rafal Milecki <zajec5@gmail.com>
>>> Reported-by: Grey Christoforo <grey@christoforo.net>
>>> Reviewed-by: Pieter-Paul Giesberts <pieter-paul.giesberts@broadcom.com>
>>> Reviewed-by: Arend Van Spriel <arend@broadcom.com>
>>> Reviewed-by: Hante Meuleman <hante.meuleman@broadcom.com>
>>> Signed-off-by: Franky Lin <franky.lin@broadcom.com>
>>> [arend@broadcom.com: rephrased the commit message]
>>> Signed-off-by: Arend van Spriel <arend@broadcom.com>
>>> Signed-off-by: Kalle Valo <kvalo@codeaurora.org>
>>>
>>> (backported from commit 31143e2933d1675c4c1ba6ce125cdd95870edd85)
>>> [smb: file -> drivers/net/wireless/brcm80211/brcmfmac/msgbuf.c
>>>       brcmf_msgbuf_process_rx_complete -> brcmf_msgbuf_process_rx]

Oh ok, I did get confused by the stack trace mentioning the other function but
applied the hunk to the correct function by luck. So above should be changed into

[smb: file -> drivers/net/wireless/brcm80211/brcmfmac/msgbuf.c]

Thanks for catching this.

Stefan

>>
>> Did the function name really need adjustment?
> 
> I meant to say I applied the code to a different file and a different function
> than in the original patch.
> 
> -Stefan
> 
>>
>> Acked-by: Kleber Sacilotto de Souza <kleber.souza@canonical.com>
>>
>>
>>> Signed-off-by: Stefan Bader <stefan.bader@canonical.com>
>>> ---
>>>  drivers/net/wireless/brcm80211/brcmfmac/msgbuf.c | 2 ++
>>>  1 file changed, 2 insertions(+)
>>>
>>> diff --git a/drivers/net/wireless/brcm80211/brcmfmac/msgbuf.c b/drivers/net/wireless/brcm80211/brcmfmac/msgbuf.c
>>> index 6f7138cea555..f944f356d9c5 100644
>>> --- a/drivers/net/wireless/brcm80211/brcmfmac/msgbuf.c
>>> +++ b/drivers/net/wireless/brcm80211/brcmfmac/msgbuf.c
>>> @@ -1155,6 +1155,8 @@ brcmf_msgbuf_process_rx_complete(struct brcmf_msgbuf *msgbuf, void *buf)
>>>  		brcmu_pkt_buf_free_skb(skb);
>>>  		return;
>>>  	}
>>> +
>>> +	skb->protocol = eth_type_trans(skb, ifp->ndev);
>>>  	brcmf_netif_rx(ifp, skb);
>>>  }
>>>  
>>>
>>
> 
> 
>
Stefan Bader July 18, 2019, 8:23 a.m. UTC | #5
On 17.07.19 10:57, Stefan Bader wrote:
> From: Franky Lin <franky.lin@broadcom.com>
> 
> BugLink: https://bugs.launchpad.net/bugs/1836801
> 
> A regression was introduced in commit 9c349892ccc9 ("brcmfmac: revise
> handling events in receive path") which moves eth_type_trans() call
> to brcmf_rx_frame(). Msgbuf layer doesn't use brcmf_rx_frame() but invokes
> brcmf_netif_rx() directly. In such case the Ethernet header was not
> stripped out resulting in null pointer dereference in the networking
> stack.
> 
> BUG: unable to handle kernel NULL pointer dereference at 0000000000000048
> IP: [<ffffffff814c3ce6>] enqueue_to_backlog+0x56/0x260
> PGD 0
> Oops: 0000 [#1] PREEMPT SMP
> Modules linked in: fuse ipt_MASQUERADE nf_nat_masquerade_ipv4
> iptable_nat nf_conntrack_ipv4 nf_defrag_ipv4 nf_nat_ipv4 xt_addrtype
> [...]
> rtsx_pci scsi_mod usbcore usb_common i8042 serio nvme nvme_core
> CPU: 7 PID: 1340 Comm: irq/136-brcmf_p Not tainted 4.7.0-rc1-mainline #1
> Hardware name: Dell Inc. XPS 15 9550/0N7TVV, BIOS 01.02.00 04/07/2016
> task: ffff8804a0c5bd00 ti: ffff88049e124000 task.ti: ffff88049e124000
> RIP: 0010:[<ffffffff814c3ce6>] [<ffffffff814c3ce6>]
> enqueue_to_backlog+0x56/0x260
> RSP: 0018:ffff88049e127ca0 EFLAGS: 00010046
> RAX: 0000000000000000 RBX: ffff8804bddd7c40 RCX: 000000000000002f
> RDX: 0000000000000000 RSI: 0000000000000007 RDI: ffff8804bddd7d4c
> RBP: ffff88049e127ce8 R08: 0000000000000000 R09: 0000000000000000
> R10: ffff8804bddd12c0 R11: 000000000000149e R12: 0000000000017c40
> R13: ffff88049e127d08 R14: ffff8804a9bd6d00 R15: ffff8804bddd7d4c
> FS: 0000000000000000(0000) GS:ffff8804bddc0000(0000) knlGS:0000000000000000
> CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> CR2: 0000000000000048 CR3: 0000000001806000 CR4: 00000000003406e0
> DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
> DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
> Stack:
> ffff8804bdddad00 ffff8804ad089e00 0000000000000000 0000000000000282
> 0000000000000000 ffff8804a9bd6d00 ffff8804a1b27e00 ffff8804a9bd6d00
> ffff88002ee88000 ffff88049e127d28 ffffffff814c3f3b ffffffff81311fc3
> Call Trace:
> [<ffffffff814c3f3b>] netif_rx_internal+0x4b/0x170
> [<ffffffff81311fc3>] ? swiotlb_tbl_unmap_single+0xf3/0x120
> [<ffffffff814c5467>] netif_rx_ni+0x27/0xc0
> [<ffffffffa08519e9>] brcmf_netif_rx+0x49/0x70 [brcmfmac]
> [<ffffffffa08564d4>] brcmf_msgbuf_process_rx+0x2b4/0x570 [brcmfmac]
> [<ffffffff81020017>] ? __xen_set_pgd_hyper+0x57/0xd0
> [<ffffffff810d60b0>] ? irq_forced_thread_fn+0x70/0x70
> [<ffffffffa0857381>] brcmf_proto_msgbuf_rx_trigger+0x31/0xe0 [brcmfmac]
> [<ffffffffa0861e8f>] brcmf_pcie_isr_thread+0x7f/0x110 [brcmfmac]
> [<ffffffff810d60d0>] irq_thread_fn+0x20/0x50
> [<ffffffff810d63ad>] irq_thread+0x12d/0x1c0
> [<ffffffff815d07d5>] ? __schedule+0x2f5/0x7a0
> [<ffffffff810d61d0>] ? wake_threads_waitq+0x30/0x30
> [<ffffffff810d6280>] ? irq_thread_dtor+0xb0/0xb0
> [<ffffffff81098ea8>] kthread+0xd8/0xf0
> [<ffffffff815d4b7f>] ret_from_fork+0x1f/0x40
> [<ffffffff81098dd0>] ? kthread_worker_fn+0x170/0x170
> Code: 1c f5 60 9a 8e 81 9c 58 0f 1f 44 00 00 48 89 45 d0 fa 66 0f 1f
> 44 00 00 4c 8d bb 0c 01 00 00 4c 89 ff e8 5e 08 11 00 49 8b 56 20 <48>
> 8b 52 48 83 e2 01 74 10 8b 8b 08 01 00 00 8b 15 59 c5 42 00
> RIP [<ffffffff814c3ce6>] enqueue_to_backlog+0x56/0x260
> RSP <ffff88049e127ca0>
> CR2: 0000000000000048
> 
> Fixes: 9c349892ccc9 ("brcmfmac: revise handling events in receive path")
> Reported-by: Rafal Milecki <zajec5@gmail.com>
> Reported-by: Grey Christoforo <grey@christoforo.net>
> Reviewed-by: Pieter-Paul Giesberts <pieter-paul.giesberts@broadcom.com>
> Reviewed-by: Arend Van Spriel <arend@broadcom.com>
> Reviewed-by: Hante Meuleman <hante.meuleman@broadcom.com>
> Signed-off-by: Franky Lin <franky.lin@broadcom.com>
> [arend@broadcom.com: rephrased the commit message]
> Signed-off-by: Arend van Spriel <arend@broadcom.com>
> Signed-off-by: Kalle Valo <kvalo@codeaurora.org>
> 
> (backported from commit 31143e2933d1675c4c1ba6ce125cdd95870edd85)
> [smb: file -> drivers/net/wireless/brcm80211/brcmfmac/msgbuf.c
>       brcmf_msgbuf_process_rx_complete -> brcmf_msgbuf_process_rx]
> Signed-off-by: Stefan Bader <stefan.bader@canonical.com>
> ---

Applied to xenial/master-next and re-spun. Thanks.

-Stefan

>  drivers/net/wireless/brcm80211/brcmfmac/msgbuf.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/net/wireless/brcm80211/brcmfmac/msgbuf.c b/drivers/net/wireless/brcm80211/brcmfmac/msgbuf.c
> index 6f7138cea555..f944f356d9c5 100644
> --- a/drivers/net/wireless/brcm80211/brcmfmac/msgbuf.c
> +++ b/drivers/net/wireless/brcm80211/brcmfmac/msgbuf.c
> @@ -1155,6 +1155,8 @@ brcmf_msgbuf_process_rx_complete(struct brcmf_msgbuf *msgbuf, void *buf)
>  		brcmu_pkt_buf_free_skb(skb);
>  		return;
>  	}
> +
> +	skb->protocol = eth_type_trans(skb, ifp->ndev);
>  	brcmf_netif_rx(ifp, skb);
>  }
>  
>

Patch
diff mbox series

diff --git a/drivers/net/wireless/brcm80211/brcmfmac/msgbuf.c b/drivers/net/wireless/brcm80211/brcmfmac/msgbuf.c
index 6f7138cea555..f944f356d9c5 100644
--- a/drivers/net/wireless/brcm80211/brcmfmac/msgbuf.c
+++ b/drivers/net/wireless/brcm80211/brcmfmac/msgbuf.c
@@ -1155,6 +1155,8 @@  brcmf_msgbuf_process_rx_complete(struct brcmf_msgbuf *msgbuf, void *buf)
 		brcmu_pkt_buf_free_skb(skb);
 		return;
 	}
+
+	skb->protocol = eth_type_trans(skb, ifp->ndev);
 	brcmf_netif_rx(ifp, skb);
 }