netdev - Re: [tipc-discussion] [net v2 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] [day] [month] [year] [list]

Date:   Tue, 27 Oct 2020 11:49:56 -0700
From:   Cong Wang <xiyou.wangcong@...il.com>
To:     Tung 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 v2 1/1] tipc: fix memory leak caused by tipc_buf_append()

On Tue, Oct 27, 2020 at 11:21 AM Cong Wang <xiyou.wangcong@...il.com> wrote:
>
> On Mon, Oct 26, 2020 at 3:46 AM Tung Nguyen
> <tung.q.nguyen@...tech.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:
>
> Interesting, I can not immediately see how tipc_link_advance_transmq()
> clones the skb. You point out how it is freed but not cloned.
>
> It looks really odd to see the skb is held by some caller, then expected
> to be released by the unshare in tipc_buf_append(). IMHO, the refcnt
> should be released where it is held.

More importantly, prior to Xin Long's change of skb_unshare(),
skb_unclone() was used, which does not touch the skb refcnt either.
So, why does it rely on skb_unshare() to release this refcnt now?

Thanks.