netdev - RE: [tipc-discussion] [net v3 1/1] tipc: fix memory leak caused by tipc_buf

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Wed, 28 Oct 2020 05:23:23 +0000
From:   Tung Quang Nguyen <tung.q.nguyen@...tech.com.au>
To:     Cong Wang <xiyou.wangcong@...il.com>,
        "tipc-discussion@...ts.sourceforge.net" 
        <tipc-discussion@...ts.sourceforge.net>,
        Xin Long <lucien.xin@...il.com>,
        David Miller <davem@...emloft.net>,
        Linux Kernel Network Developers <netdev@...r.kernel.org>
Subject: RE: [tipc-discussion] [net v3 1/1] tipc: fix memory leak caused by
 tipc_buf_append()

Hi Cong,

No, I have never ignored any comment from reviewers. I sent v2 on Oct 26 after discussing with Xin Long, and v3 on Oct 27 after receiving comment from Jakub.
I received your 3 emails nearly at the same time on Oct 28. It's weird. Your emails did not appear in this email archive either: https://sourceforge.net/p/tipc/mailman/tipc-discussion/

Anyway, I answer your questions:
1/ Why it is not correct if not decreasing the data reference counter in tipc_buf_append()
In my changelog, I just pinpointed the place where the leak would happen. I show you the details here:
tipc_msg_reassemble(list,-)
{
 ...
 frag = skb_clone(skb, GFP_ATOMIC); // each data reference counter of the original skb has the value of 2.
 ...
 If (tipc_buf_append(&head, &frag)) // each data reference counter of the original skb STILL has the value of 2 because the usage of skb_copy() instead of skb_unshare()
 ...
}
The original skb list then is passed to tipc_bcast_xmit() which in turn calls tipc_link_xmit():
tipc_link_xmit(-, list, -)
{
 ...
 _skb = skb_clone(skb, GFP_ATOMIC); // each data reference counter of the original skb has the value of 3.
...
}

When each cloned skb is sent out by the driver, it is freed by the driver. That leads to each data reference counter of the original skb has the value of 2.
After receiving ACK from another peer, the original skb needs to be freed:
tipc_link_advance_transmq()
{
 ...
 kfree_skb(skb);  // memory exists after being freed because the data reference counter still has the value of 2.
}

This indeed causes memory leak.

2/ Why previously-used skb_unclone() works.
The purpose of skb_unclone() is to unclone the cloned skb. So, it does not make any sense to say that " skb_unclone() expects refcnt == 1" as I understand
you implied the data reference counter.
pskb_expand_head() inside skb_unclone() requires that the user reference counter has the value of 1 as implemented:
pskb_expand_head()
{
 ...
 BUG_ON(skb_shared(skb)); // User reference counter must be 1
...
atomic_set(&skb_shinfo(skb)->dataref, 1); // The data reference counter of the original skb has the value of 1
...
}
That explains why after being passed to tipc_link_xmit(), each data reference counter of each original skb has the value of 2 and can be freed in tipc_link_advance_transmq().

Best regards,
Tung Nguyen

-----Original Message-----
From: Cong Wang <xiyou.wangcong@...il.com> 
Sent: Wednesday, October 28, 2020 3:50 AM
To: Tung Quang Nguyen <tung.q.nguyen@...tech.com.au>
Cc: David Miller <davem@...emloft.net>; Linux Kernel Network Developers <netdev@...r.kernel.org>; tipc-discussion@...ts.sourceforge.net
Subject: Re: [tipc-discussion] [net v3 1/1] tipc: fix memory leak caused by tipc_buf_append()

On Tue, Oct 27, 2020 at 1:09 PM Tung Nguyen
<tung.q.nguyen@...tech.com.au> 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:

This does not make sense at all.

skb_unclone() expects refcnt == 1, as stated in the comments
above pskb_expand_head(). skb_unclone() was used prior to
Xin Long's commit.

So either the above is wrong, or something important is still missing
in your changelog. None of them is addressed in your V3.

I also asked you two questions before you sent V3, you seem to
intentionally ignore them. This is not how we collaborate.

Thanks.