RE: [PATCH] tipc: unshare skb in tipc_l2_rcv_msg() before decrypting

[Tung Quang Nguyen]

>Subject: [PATCH] tipc: unshare skb in tipc_l2_rcv_msg() before decrypting

net tree needs to be specified for bug fix.

>
>Packet taps such as AF_PACKET sockets bound to ETH_P_TIPC take an extra
>reference on the skb before handing it to the TIPC packet handler.
>tipc_l2_rcv_msg() passes it on as is, and for an encrypted message
>tipc_aead_decrypt() calls skb_cow_data(), which may end up in
>pskb_expand_head() and hit BUG_ON(skb_shared(skb)):
>
>  kernel BUG at net/core/skbuff.c:2308!
>  pskb_expand_head
>  __pskb_pull_tail
>  skb_cow_data
>  tipc_aead_decrypt
>  tipc_crypto_rcv
>  tipc_rcv
>  tipc_l2_rcv_msg
>  __netif_receive_skb_core
>
>A packet_type handler must not modify a shared skb. Use
>skb_share_check() on entry, which clones a shared skb and frees the original
>reference.
>
>Reported-by: syzbot+0017742620a9d06f06ef@xxxxxxxxxxxxxxxxxxxxxxxxx
>Closes: https://syzkaller.appspot.com/bug?extid=0017742620a9d06f06ef
>Fixes: fc1b6d6de220 ("tipc: introduce TIPC encryption & authentication")

Fixes tag does not seem correct.

>Cc: stable@xxxxxxxxxxxxxxx
>Signed-off-by: Nguyen Ngoc Thang <ngocthang2710.1999@xxxxxxxxx>
>---
> net/tipc/bearer.c | 4 ++++
> 1 file changed, 4 insertions(+)
>
>diff --git a/net/tipc/bearer.c b/net/tipc/bearer.c index
>05dcd2f9e887..5e7e220fa7f5 100644
>--- a/net/tipc/bearer.c
>+++ b/net/tipc/bearer.c
>@@ -659,6 +659,10 @@ static int tipc_l2_rcv_msg(struct sk_buff *skb, struct
>net_device *dev,  {
> 	struct tipc_bearer *b;
>
>+	skb = skb_share_check(skb, GFP_ATOMIC);

This solution is incorrect and incomplete because:
1. skb_share_check() clones a new skb that shares data part with TIPC while TIPC needs to modify the data part.
2. Checking shared skb in ethernet bearer callback is not enough as we still have udp callback.

As   I was working on this issue at the time you posted your patch, I will send the correct and complete patch.

>+	if (!skb)
>+		return NET_RX_DROP;
>+
> 	rcu_read_lock();
> 	b = rcu_dereference(dev->tipc_ptr) ?:
> 		rcu_dereference(orig_dev->tipc_ptr);
>--
>2.43.0