diff mbox series

[net] tipc: skb_linearize the head skb when reassembling msgs

Message ID c7d752b5522360de0a6886202c59fe49524a9fda.1620417423.git.lucien.xin@gmail.com
State New
Headers show
Series [net] tipc: skb_linearize the head skb when reassembling msgs | expand

Commit Message

Xin Long May 7, 2021, 7:57 p.m. UTC 
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(-)

Comments

Jon Maloy May 15, 2021, 10:57 p.m. UTC | #1 
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>
patchwork-bot+netdevbpf@kernel.org May 17, 2021, 8:50 p.m. UTC | #2 
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 mbox series

Patch

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;
 	}