| Message ID | c7d752b5522360de0a6886202c59fe49524a9fda.1620417423.git.lucien.xin@gmail.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | b7df21cf1b79ab7026f545e7bf837bd5750ac026 |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net] tipc: skb_linearize the head skb when reassembling msgs | expand |
| Context | Check | Description |
|---|---|---|
| netdev/cover_letter | success | Link |
| netdev/fixes_present | success | Link |
| netdev/patch_count | success | Link |
| netdev/tree_selection | success | Clearly marked for net |
| netdev/subject_prefix | success | Link |
| netdev/cc_maintainers | success | CCed 6 of 6 maintainers |
| netdev/source_inline | success | Was 0 now: 0 |
| netdev/verify_signedoff | success | Link |
| netdev/module_param | success | Was 0 now: 0 |
| netdev/build_32bit | success | Errors and warnings before: 0 this patch: 0 |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/verify_fixes | success | Link |
| netdev/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 20 lines checked |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 0 this patch: 0 |
| netdev/header_inline | success | Link |
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>
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
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; }
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(-)