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]

Date: Sat, 3 Feb 2024 12:42:12 +0100
From: Eric Dumazet <edumazet@...gle.com>
To: Kuniyuki Iwashima <kuniyu@...zon.com>
Cc: davem@...emloft.net, kuba@...nel.org, kuni1840@...il.com, 
	netdev@...r.kernel.org, pabeni@...hat.com, 
	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 10:15 AM Kuniyuki Iwashima <kuniyu@...zon.com> wrote:
>
> From: Eric Dumazet <edumazet@...gle.com>
> Date: Sat, 3 Feb 2024 10:01:04 +0100
> > 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;
> > }
>
> I wrote that in the inital fix but noticed that this
> kfree_skb() triggers fput(), and later in unix_release_sock()
> we will call the duplicate kfree_skb().
>
>         if (u->oob_skb) {
>                 kfree_skb(u->oob_skb);
>                 u->oob_skb = NULL;
>         }
>
> So, we need to set NULL before kfree_skb() in __unix_gc().

Okay...

But we probably need :

#if IS_ENABLED(CONFIG_AF_UNIX_OOB)


>
>
> >
> > Also we probably can send this later:
>
> Yes, I changed as such in a new GC implementation, this needs
> respin for MSG_OOB support though...
> https://lore.kernel.org/netdev/20240203030058.60750-14-kuniyu@amazon.com/
>
> Thanks!
>
> >
> > 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);