Menu
▾
▴
Re: [tipc-discussion] [net v1 1/1] tipc: fix memory leak caused by tipc_buf_append()
|
From: Xin L. <luc...@gm...> - 2020-10-26 09:35:33
|
On Mon, Oct 26, 2020 at 5:30 PM Tung Quang Nguyen
<tun...@de...> 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 <luc...@gm...>
> Sent: Monday, October 26, 2020 4:10 PM
> To: Tung Quang Nguyen <tun...@de...>
> Cc: tip...@li...; Jon Maloy <jm...@re...>; ma...@do...; Ying Xue <yin...@wi...>; Cong Wang <xiy...@gm...>
> 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
> <tun...@de...> 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);
> 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
> > tip...@li...
> > https://lists.sourceforge.net/lists/listinfo/tipc-discussion
|
