On Tue, 27 Oct 2020 10:24:03 +0700 Tung Nguyen wrote:
> Commit ed42989eab57 ("tipc: 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, the pointer to the
> fragment is set to NULL before calling skb_unshare() to make sure that
> the original skb is not freed after freeing the fragment 2 times in
> case skb_unshare() returns NULL.
>
> Fixes: ed42989eab57 ("tipc: fix the skb_unshare() in tipc_buf_append()")
> Acked-by: Jon Maloy <[email protected]>
> Reported-by: Thang Hoang Ngo <[email protected]>
> Signed-off-by: Tung Nguyen <[email protected]>
Applied, queued for all the stables.
Thanks everyone!
