Re: [tipc-discussion] [net v1 1/1] tipc: fix memory leak caused by tipc_buf_append()

[Jon M. <jm...@re...>]

On 10/23/20 4:19 AM, Tung Nguyen wrote:
> Commit ed42989eab57 ("fix the skb_unshare() in tipc_buf_append()")
> replaced skb_unshare() with skb_copy() to not reduce the data reference
> counter of the original skb intentionally. This is not the correct
> way to handle the cloned skb because it causes memory leak in 2
> following cases:
>   1/ Sending multicast messages via broadcast link
>    The original skb list is cloned to the local skb list for local
>    destination. After that, the data reference counter of each skb
>    in the original list has the value of 2. This causes each skb not
>    to be freed after receiving ACK:
>    tipc_link_advance_transmq()
>    {
>     ...
>     /* release skb */
>     __skb_unlink(skb, &l->transmq);
>     kfree_skb(skb); <-- memory exists after being freed
>    }
>
>   2/ Sending multicast messages via replicast link
>    Similar to the above case, each skb cannot be freed after purging
>    the skb list:
>    tipc_mcast_xmit()
>    {
>     ...
>     __skb_queue_purge(pkts); <-- memory exists after being freed
>    }
>
> This commit fixes this issue by using skb_unshare() instead. Besides,
> to avoid use-after-free error reported by KASAN, kfree_skb(head) in
> tipc_buf_append() is called only if the pointer to the appending skb
> is not NULL.
>
> v2: improve condition for freeing the appending skb to cover all error
> cases.
>
> Fixes: ed42989eab57 ("fix the skb_unshare() in tipc_buf_append()")
> Reported-by: Thang Hoang Ngo <tha...@de...>
> Signed-off-by: Tung Nguyen <tun...@de...>
> ---
>   net/tipc/msg.c | 6 +++---
>   1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/net/tipc/msg.c b/net/tipc/msg.c
> index 2a78aa701572..46c36c5093de 100644
> --- a/net/tipc/msg.c
> +++ b/net/tipc/msg.c
> @@ -150,8 +150,7 @@ int tipc_buf_append(struct sk_buff **headbuf, struct sk_buff **buf)
>   	if (fragid == FIRST_FRAGMENT) {
>   		if (unlikely(head))
>   			goto err;
> -		if (skb_cloned(frag))
> -			frag = skb_copy(frag, GFP_ATOMIC);
> +		frag = skb_unshare(frag, GFP_ATOMIC);
>   		if (unlikely(!frag))
>   			goto err;
>   		head = *headbuf = frag;
> @@ -797,7 +796,8 @@ bool tipc_msg_reassemble(struct sk_buff_head *list, struct sk_buff_head *rcvq)
>   	return true;
>   error:
>   	pr_warn("Failed do clone local mcast rcv buffer\n");
> -	kfree_skb(head);
> +	if (head)
> +		kfree_skb(head);
>   	return false;
>   }
>   
Acked-by: Jon Maloy <jm...@re...>