On Mon, Oct 26, 2020 at 5:30 PM Tung Quang Nguyen <[email protected]> wrote: > > 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. Sorry, I realized it was a false one after double-checking.
> > Thanks. > Tung Nguyen > > -----Original Message----- > From: Xin Long <[email protected]> > Sent: Monday, October 26, 2020 4:10 PM > To: Tung Quang Nguyen <[email protected]> > Cc: [email protected]; Jon Maloy <[email protected]>; > [email protected]; Ying Xue <[email protected]>; Cong Wang > <[email protected]> > 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 > <[email protected]> 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 <[email protected]> > > Signed-off-by: Tung Nguyen <[email protected]> > > --- > > 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)) { This should be: @@ -150,10 +150,12 @@ 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)) + if (skb_cloned(frag)) { frag = skb_copy(frag, GFP_ATOMIC); - if (unlikely(!frag)) - goto err; + if (unlikely(!frag)) + goto err; + kfree_skb(*buf) + } > > 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 > > [email protected] > > https://lists.sourceforge.net/lists/listinfo/tipc-discussion _______________________________________________ tipc-discussion mailing list [email protected] https://lists.sourceforge.net/lists/listinfo/tipc-discussion
