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

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

[Nguyen Ngoc Thang]

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@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=0017742620a9d06f06ef
Fixes: fc1b6d6de220 ("tipc: introduce TIPC encryption & authentication")
Cc: stable@vger.kernel.org
Signed-off-by: Nguyen Ngoc Thang <ngocthang2710.1999@gmail.com>
---
 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);
+	if (!skb)
+		return NET_RX_DROP;
+
 	rcu_read_lock();
 	b = rcu_dereference(dev->tipc_ptr) ?:
 		rcu_dereference(orig_dev->tipc_ptr);
-- 
2.43.0

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@syzkaller.appspotmail.com
>Closes: https://syzkaller.appspot.com/bug?extid=0017742620a9d06f06ef
>Fixes: fc1b6d6de220 ("tipc: introduce TIPC encryption & authentication")

Fixes tag does not seem correct.

>Cc: stable@vger.kernel.org
>Signed-off-by: Nguyen Ngoc Thang <ngocthang2710.1999@gmail.com>
>---
> 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