* [PATCH net] tipc: skb_linearize the head skb when reassembling msgs
@ 2021-05-07 19:57 Xin Long
2021-05-15 22:57 ` Jon Maloy
2021-05-17 20:50 ` patchwork-bot+netdevbpf
0 siblings, 2 replies; 3+ messages in thread
From: Xin Long @ 2021-05-07 19:57 UTC (permalink / raw)
To: network dev, tipc-discussion; +Cc: davem, kuba, Jon Maloy, Ying Xue
It's not a good idea to append the frag skb to a skb's frag_list if
the frag_list already has skbs from elsewhere, such as this skb was
created by pskb_copy() where the frag_list was cloned (all the skbs
in it were skb_get'ed) and shared by multiple skbs.
However, the new appended frag skb should have been only seen by the
current skb. Otherwise, it will cause use after free crashes as this
appended frag skb are seen by multiple skbs but it only got skb_get
called once.
The same thing happens with a skb updated by pskb_may_pull() with a
skb_cloned skb. Li Shuang has reported quite a few crashes caused
by this when doing testing over macvlan devices:
[] kernel BUG at net/core/skbuff.c:1970!
[] Call Trace:
[] skb_clone+0x4d/0xb0
[] macvlan_broadcast+0xd8/0x160 [macvlan]
[] macvlan_process_broadcast+0x148/0x150 [macvlan]
[] process_one_work+0x1a7/0x360
[] worker_thread+0x30/0x390
[] kernel BUG at mm/usercopy.c:102!
[] Call Trace:
[] __check_heap_object+0xd3/0x100
[] __check_object_size+0xff/0x16b
[] simple_copy_to_iter+0x1c/0x30
[] __skb_datagram_iter+0x7d/0x310
[] __skb_datagram_iter+0x2a5/0x310
[] skb_copy_datagram_iter+0x3b/0x90
[] tipc_recvmsg+0x14a/0x3a0 [tipc]
[] ____sys_recvmsg+0x91/0x150
[] ___sys_recvmsg+0x7b/0xc0
[] kernel BUG at mm/slub.c:305!
[] Call Trace:
[] <IRQ>
[] kmem_cache_free+0x3ff/0x400
[] __netif_receive_skb_core+0x12c/0xc40
[] ? kmem_cache_alloc+0x12e/0x270
[] netif_receive_skb_internal+0x3d/0xb0
[] ? get_rx_page_info+0x8e/0xa0 [be2net]
[] be_poll+0x6ef/0xd00 [be2net]
[] ? irq_exit+0x4f/0x100
[] net_rx_action+0x149/0x3b0
...
This patch is to fix it by linearizing the head skb if it has frag_list
set in tipc_buf_append(). Note that we choose to do this before calling
skb_unshare(), as __skb_linearize() will avoid skb_copy(). Also, we can
not just drop the frag_list either as the early time.
Fixes: 45c8b7b175ce ("tipc: allow non-linear first fragment buffer")
Reported-by: Li Shuang <shuali@redhat•com>
Signed-off-by: Xin Long <lucien.xin@gmail•com>
---
net/tipc/msg.c | 9 ++-------
1 file changed, 2 insertions(+), 7 deletions(-)
diff --git a/net/tipc/msg.c b/net/tipc/msg.c
index 3f0a253..ce6ab54 100644
--- a/net/tipc/msg.c
+++ b/net/tipc/msg.c
@@ -149,18 +149,13 @@ int tipc_buf_append(struct sk_buff **headbuf, struct sk_buff **buf)
if (unlikely(head))
goto err;
*buf = NULL;
+ if (skb_has_frag_list(frag) && __skb_linearize(frag))
+ goto err;
frag = skb_unshare(frag, GFP_ATOMIC);
if (unlikely(!frag))
goto err;
head = *headbuf = frag;
TIPC_SKB_CB(head)->tail = NULL;
- if (skb_is_nonlinear(head)) {
- skb_walk_frags(head, tail) {
- TIPC_SKB_CB(head)->tail = tail;
- }
- } else {
- skb_frag_list_init(head);
- }
return 0;
}
--
2.1.0
^ permalink raw reply related [flat|nested] 3+ messages in thread* Re: [PATCH net] tipc: skb_linearize the head skb when reassembling msgs
2021-05-07 19:57 [PATCH net] tipc: skb_linearize the head skb when reassembling msgs Xin Long
@ 2021-05-15 22:57 ` Jon Maloy
2021-05-17 20:50 ` patchwork-bot+netdevbpf
1 sibling, 0 replies; 3+ messages in thread
From: Jon Maloy @ 2021-05-15 22:57 UTC (permalink / raw)
To: Xin Long, network dev, tipc-discussion; +Cc: davem, kuba, Ying Xue
On 5/7/21 3:57 PM, Xin Long wrote:
> It's not a good idea to append the frag skb to a skb's frag_list if
> the frag_list already has skbs from elsewhere, such as this skb was
> created by pskb_copy() where the frag_list was cloned (all the skbs
> in it were skb_get'ed) and shared by multiple skbs.
>
> However, the new appended frag skb should have been only seen by the
> current skb. Otherwise, it will cause use after free crashes as this
> appended frag skb are seen by multiple skbs but it only got skb_get
> called once.
>
> The same thing happens with a skb updated by pskb_may_pull() with a
> skb_cloned skb. Li Shuang has reported quite a few crashes caused
> by this when doing testing over macvlan devices:
>
> [] kernel BUG at net/core/skbuff.c:1970!
> [] Call Trace:
> [] skb_clone+0x4d/0xb0
> [] macvlan_broadcast+0xd8/0x160 [macvlan]
> [] macvlan_process_broadcast+0x148/0x150 [macvlan]
> [] process_one_work+0x1a7/0x360
> [] worker_thread+0x30/0x390
>
> [] kernel BUG at mm/usercopy.c:102!
> [] Call Trace:
> [] __check_heap_object+0xd3/0x100
> [] __check_object_size+0xff/0x16b
> [] simple_copy_to_iter+0x1c/0x30
> [] __skb_datagram_iter+0x7d/0x310
> [] __skb_datagram_iter+0x2a5/0x310
> [] skb_copy_datagram_iter+0x3b/0x90
> [] tipc_recvmsg+0x14a/0x3a0 [tipc]
> [] ____sys_recvmsg+0x91/0x150
> [] ___sys_recvmsg+0x7b/0xc0
>
> [] kernel BUG at mm/slub.c:305!
> [] Call Trace:
> [] <IRQ>
> [] kmem_cache_free+0x3ff/0x400
> [] __netif_receive_skb_core+0x12c/0xc40
> [] ? kmem_cache_alloc+0x12e/0x270
> [] netif_receive_skb_internal+0x3d/0xb0
> [] ? get_rx_page_info+0x8e/0xa0 [be2net]
> [] be_poll+0x6ef/0xd00 [be2net]
> [] ? irq_exit+0x4f/0x100
> [] net_rx_action+0x149/0x3b0
>
> ...
>
> This patch is to fix it by linearizing the head skb if it has frag_list
> set in tipc_buf_append(). Note that we choose to do this before calling
> skb_unshare(), as __skb_linearize() will avoid skb_copy(). Also, we can
> not just drop the frag_list either as the early time.
>
> Fixes: 45c8b7b175ce ("tipc: allow non-linear first fragment buffer")
> Reported-by: Li Shuang <shuali@redhat•com>
> Signed-off-by: Xin Long <lucien.xin@gmail•com>
> ---
> net/tipc/msg.c | 9 ++-------
> 1 file changed, 2 insertions(+), 7 deletions(-)
>
> diff --git a/net/tipc/msg.c b/net/tipc/msg.c
> index 3f0a253..ce6ab54 100644
> --- a/net/tipc/msg.c
> +++ b/net/tipc/msg.c
> @@ -149,18 +149,13 @@ int tipc_buf_append(struct sk_buff **headbuf, struct sk_buff **buf)
> if (unlikely(head))
> goto err;
> *buf = NULL;
> + if (skb_has_frag_list(frag) && __skb_linearize(frag))
> + goto err;
> frag = skb_unshare(frag, GFP_ATOMIC);
> if (unlikely(!frag))
> goto err;
> head = *headbuf = frag;
> TIPC_SKB_CB(head)->tail = NULL;
> - if (skb_is_nonlinear(head)) {
> - skb_walk_frags(head, tail) {
> - TIPC_SKB_CB(head)->tail = tail;
> - }
> - } else {
> - skb_frag_list_init(head);
> - }
> return 0;
> }
>
Acked-by: Jon Maloy <jmaloy@redhat•com>
^ permalink raw reply [flat|nested] 3+ messages in thread* Re: [PATCH net] tipc: skb_linearize the head skb when reassembling msgs
2021-05-07 19:57 [PATCH net] tipc: skb_linearize the head skb when reassembling msgs Xin Long
2021-05-15 22:57 ` Jon Maloy
@ 2021-05-17 20:50 ` patchwork-bot+netdevbpf
1 sibling, 0 replies; 3+ messages in thread
From: patchwork-bot+netdevbpf @ 2021-05-17 20:50 UTC (permalink / raw)
To: Xin Long; +Cc: netdev, tipc-discussion, davem, kuba, jmaloy, ying.xue
Hello:
This patch was applied to netdev/net.git (refs/heads/master):
On Sat, 8 May 2021 03:57:03 +0800 you wrote:
> It's not a good idea to append the frag skb to a skb's frag_list if
> the frag_list already has skbs from elsewhere, such as this skb was
> created by pskb_copy() where the frag_list was cloned (all the skbs
> in it were skb_get'ed) and shared by multiple skbs.
>
> However, the new appended frag skb should have been only seen by the
> current skb. Otherwise, it will cause use after free crashes as this
> appended frag skb are seen by multiple skbs but it only got skb_get
> called once.
>
> [...]
Here is the summary with links:
- [net] tipc: skb_linearize the head skb when reassembling msgs
https://git.kernel.org/netdev/net/c/b7df21cf1b79
You are awesome, thank you!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2021-05-17 20:50 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2021-05-07 19:57 [PATCH net] tipc: skb_linearize the head skb when reassembling msgs Xin Long
2021-05-15 22:57 ` Jon Maloy
2021-05-17 20:50 ` patchwork-bot+netdevbpf
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox