Hi Xin,
Yes, I know that it should not be a problem if skb_free(NULL) is called.
But I relied on your analysis for syzbot report:
"
in tipc_msg_reassemble():
if ((&head, &frag))
break;
if (!head)
goto error; <--- [1]
}
__skb_queue_tail(rcvq, frag);
return true;
error:
pr_warn("Failed do clone local mcast rcv buffer\n");
kfree_skb(head); <---[2]
return false;
when head is NULL at [1], it goes [2] and could cause a crash.
from the log, we can see "Failed do clone local mcast rcv buffer" as well.
"
I will check again your new analysis and create the correct patch.
Thanks.
Tung Nguyen
-----Original Message-----
From: Xin Long <lucien....@gmail.com>
Sent: Monday, October 26, 2020 4:10 PM
To: Tung Quang Nguyen <tung.q.ngu...@dektech.com.au>
Cc: tipc-discussion@lists.sourceforge.net; Jon Maloy <jma...@redhat.com>;
ma...@donjonn.com; Ying Xue <ying....@windriver.com>; Cong Wang
<xiyou.wangc...@gmail.com>
Subject: Re: [tipc-discussion] [net v1 1/1] tipc: fix memory leak caused by
tipc_buf_append()
On Fri, Oct 23, 2020 at 4:20 PM Tung Nguyen
<tung.q.ngu...@dektech.com.au> 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 <thang.h....@dektech.com.au>
> Signed-off-by: Tung Nguyen <tung.q.ngu...@dektech.com.au>
> ---
> 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);
Hi Tung,
kfree_skb(NULL) won't cause any use-after-free issue, as kfree_skb(skb)
will return when skb is NULL.
The root cause of use-after-free is as Cong fixed in
commit ed42989eab57 ("fix the skb_unshare() in tipc_buf_append()"):
When skb_unshare() returns NULL, the 'frag' is freed, and on the err
path the 'buf'(==the 'frag') get freed again, then the original skb
is freed.
But that commit indeed caused the memleak on the success path, and
the right fix should be:
diff --git a/net/tipc/msg.c b/net/tipc/msg.c
index 2a78aa7..73068fb 100644
--- a/net/tipc/msg.c
+++ b/net/tipc/msg.c
@@ -155,6 +155,7 @@ int tipc_buf_append(struct sk_buff **headbuf,
struct sk_buff **buf)
if (unlikely(!frag))
goto err;
head = *headbuf = frag;
+ kfree_skb(*buf)
*buf = NULL;
TIPC_SKB_CB(head)->tail = NULL;
if (skb_is_nonlinear(head)) {
or:
diff --git a/net/tipc/msg.c b/net/tipc/msg.c
index 2a78aa7..32c79c5 100644
--- a/net/tipc/msg.c
+++ b/net/tipc/msg.c
@@ -150,12 +150,11 @@ 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);
+ *buf = NULL;
+ frag = skb_unshare(frag, GFP_ATOMIC);
if (unlikely(!frag))
goto err;
head = *headbuf = frag;
- *buf = NULL;
TIPC_SKB_CB(head)->tail = NULL;
if (skb_is_nonlinear(head)) {
skb_walk_frags(head, tail) {
Thanks.
> return false;
> }
>
> --
> 2.17.1
>
>
>
> _______________________________________________
> tipc-discussion mailing list
> tipc-discussion@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/tipc-discussion
_______________________________________________
tipc-discussion mailing list
tipc-discussion@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/tipc-discussion
