netdev - Re: [PATCH v1 net] af_unix: Call kfree_skb() for dead unix_(sk)->oob

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]

Message-ID: <CANn89iL+BHiqZko-X0YWTdv9BCYXNY5w8rJsHf=X3NS9W+jkiA@mail.gmail.com>
Date: Sat, 3 Feb 2024 10:01:04 +0100
From: Eric Dumazet <edumazet@...gle.com>
To: Kuniyuki Iwashima <kuniyu@...zon.com>
Cc: "David S. Miller" <davem@...emloft.net>, Jakub Kicinski <kuba@...nel.org>, 
	Paolo Abeni <pabeni@...hat.com>, Kuniyuki Iwashima <kuni1840@...il.com>, netdev@...r.kernel.org, 
	syzbot+fa3ef895554bdbfd1183@...kaller.appspotmail.com
Subject: Re: [PATCH v1 net] af_unix: Call kfree_skb() for dead
 unix_(sk)->oob_skb in GC.

On Sat, Feb 3, 2024 at 9:33 AM Kuniyuki Iwashima <kuniyu@...zon.com> wrote:
>
> syzbot reported a warning [0] in __unix_gc() with a repro, which
> creates a socketpair and sends one socket's fd to itself using the
> peer.
>
>   socketpair(AF_UNIX, SOCK_STREAM, 0, [3, 4]) = 0
>   sendmsg(4, {msg_name=NULL, msg_namelen=0, msg_iov=[{iov_base="\360", iov_len=1}],
>           msg_iovlen=1, msg_control=[{cmsg_len=20, cmsg_level=SOL_SOCKET,
>                                       cmsg_type=SCM_RIGHTS, cmsg_data=[3]}],
>           msg_controllen=24, msg_flags=0}, MSG_OOB|MSG_PROBE|MSG_DONTWAIT|MSG_ZEROCOPY) = 1
>
> This forms a self-cyclic reference that GC should finally untangle
> but does not due to lack of MSG_OOB handling, resulting in memory
> leak.
>
> Recently, commit 11498715f266 ("af_unix: Remove io_uring code for
> GC.") removed io_uring's dead code in GC and revealed the problem.
>
> The code was executed at the final stage of GC and unconditionally
> moved all GC candidates from gc_candidates to gc_inflight_list.
> That papered over the reported problem by always making the following
> WARN_ON_ONCE(!list_empty(&gc_candidates)) false.
>
> The problem has been there since commit 2aab4b969002 ("af_unix: fix
> struct pid leaks in OOB support") added full scm support for MSG_OOB
> while fixing another bug.
>
> To fix this problem, we must call kfree_skb() for unix_sk(sk)->oob_skb
> if the socket still exists in gc_candidates after purging collected skb.
>
> Note that the leaked socket remained being linked to a global list, so
> kmemleak also could not detect it.  We need to check /proc/net/protocol
> to notice the unfreed socket.
>
> [
> Reported-by: syzbot+fa3ef895554bdbfd1183@...kaller.appspotmail.com
> Closes: https://syzkaller.appspot.com/bug?extid=fa3ef895554bdbfd1183
> Fixes: 2aab4b969002 ("af_unix: fix struct pid leaks in OOB support")
> Signed-off-by: Kuniyuki Iwashima <kuniyu@...zon.com>
> ---
> Given the commit disabling SCM_RIGHTS w/ io_uring was backporeted to
> stable trees, we can backport this patch without commit 11498715f266,
> so targeting net tree.
> ---
>  net/unix/garbage.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
>
> diff --git a/net/unix/garbage.c b/net/unix/garbage.c
> index 2405f0f9af31..61f313d4a5dd 100644
> --- a/net/unix/garbage.c
> +++ b/net/unix/garbage.c
> @@ -314,6 +314,15 @@ void unix_gc(void)
>         /* Here we are. Hitlist is filled. Die. */
>         __skb_queue_purge(&hitlist);
>
> +       list_for_each_entry_safe(u, next, &gc_candidates, link) {
> +               struct sk_buff *skb = u->oob_skb;
> +
> +               if (skb) {
> +                       u->oob_skb = NULL;
> +                       kfree_skb(skb);
> +               }
> +       }
> +

Reviewed-by: Eric Dumazet <edumazet@...gle.com>

Note there is already a 'struct sk_buff *skb;" variable in scope.

This could be rewritten

list_for_each_entry_safe(u, next, &gc_candidates, link) {
        kfree_skb(u->oob_skb);
        u->oob_skb = NULL;
}

Also we probably can send this later:

diff --git a/net/unix/garbage.c b/net/unix/garbage.c
index 2405f0f9af31c0ccefe2aa404002cfab8583c090..02466224445c9ec9b1259468d30c89fc5e905a6b
100644
--- a/net/unix/garbage.c
+++ b/net/unix/garbage.c
@@ -283,7 +283,7 @@ void unix_gc(void)
         * inflight counters for these as well, and remove the skbuffs
         * which are creating the cycle(s).
         */
-       skb_queue_head_init(&hitlist);
+       __skb_queue_head_init(&hitlist);
        list_for_each_entry(u, &gc_candidates, link)
                scan_children(&u->sk, inc_inflight, &hitlist);