| Message ID | 0fcddb2bab6bde5632dcd4889961ebce1ec8bb8f.1599997051.git.lucien.xin@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | [net] tipc: use skb_unshare() instead in tipc_buf_append() | expand |
From: Xin Long <lucien.xin@gmail.com> Date: Sun, 13 Sep 2020 19:37:31 +0800 > In tipc_buf_append() it may change skb's frag_list, and it causes > problems when this skb is cloned. skb_unclone() doesn't really > make this skb's flag_list available to change. > > Shuang Li has reported an use-after-free issue because of this > when creating quite a few macvlan dev over the same dev, where > the broadcast packets will be cloned and go up to the stack: ... > So fix it by using skb_unshare() instead, which would create a new > skb for the cloned frag and it'll be safe to change its frag_list. > The similar things were also done in sctp_make_reassembled_event(), > which is using skb_copy(). > > Reported-by: Shuang Li <shuali@redhat.com> > Fixes: 37e22164a8a3 ("tipc: rename and move message reassembly function") > Signed-off-by: Xin Long <lucien.xin@gmail.com> Applied and queued up for -stable, thanks.
diff --git a/net/tipc/msg.c b/net/tipc/msg.c index 848fae6..52e93ba 100644 --- a/net/tipc/msg.c +++ b/net/tipc/msg.c @@ -150,7 +150,8 @@ int tipc_buf_append(struct sk_buff **headbuf, struct sk_buff **buf) if (fragid == FIRST_FRAGMENT) { if (unlikely(head)) goto err; - if (unlikely(skb_unclone(frag, GFP_ATOMIC))) + frag = skb_unshare(frag, GFP_ATOMIC); + if (unlikely(!frag)) goto err; head = *headbuf = frag; *buf = NULL;
In tipc_buf_append() it may change skb's frag_list, and it causes problems when this skb is cloned. skb_unclone() doesn't really make this skb's flag_list available to change. Shuang Li has reported an use-after-free issue because of this when creating quite a few macvlan dev over the same dev, where the broadcast packets will be cloned and go up to the stack: [ ] BUG: KASAN: use-after-free in pskb_expand_head+0x86d/0xea0 [ ] Call Trace: [ ] dump_stack+0x7c/0xb0 [ ] print_address_description.constprop.7+0x1a/0x220 [ ] kasan_report.cold.10+0x37/0x7c [ ] check_memory_region+0x183/0x1e0 [ ] pskb_expand_head+0x86d/0xea0 [ ] process_backlog+0x1df/0x660 [ ] net_rx_action+0x3b4/0xc90 [ ] [ ] Allocated by task 1786: [ ] kmem_cache_alloc+0xbf/0x220 [ ] skb_clone+0x10a/0x300 [ ] macvlan_broadcast+0x2f6/0x590 [macvlan] [ ] macvlan_process_broadcast+0x37c/0x516 [macvlan] [ ] process_one_work+0x66a/0x1060 [ ] worker_thread+0x87/0xb10 [ ] [ ] Freed by task 3253: [ ] kmem_cache_free+0x82/0x2a0 [ ] skb_release_data+0x2c3/0x6e0 [ ] kfree_skb+0x78/0x1d0 [ ] tipc_recvmsg+0x3be/0xa40 [tipc] So fix it by using skb_unshare() instead, which would create a new skb for the cloned frag and it'll be safe to change its frag_list. The similar things were also done in sctp_make_reassembled_event(), which is using skb_copy(). Reported-by: Shuang Li <shuali@redhat.com> Fixes: 37e22164a8a3 ("tipc: rename and move message reassembly function") Signed-off-by: Xin Long <lucien.xin@gmail.com> --- net/tipc/msg.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)