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

[Xin L. <luc...@gm...>]

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)) {

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