netdev - Re: [PATCH stable 5.15/6.1/6.6] af_unix: Clear oob_skb in scan

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: <20250305181050.17199-1-kuniyu@amazon.com>
Date: Wed, 5 Mar 2025 10:10:41 -0800
From: Kuniyuki Iwashima <kuniyu@...zon.com>
To: Paolo Abeni <pabeni@...hat.com>, <gregkh@...uxfoundation.org>
CC: <kuni1840@...il.com>, <kuniyu@...zon.com>, <llfamsec@...il.com>,
	<netdev@...r.kernel.org>, <sashal@...nel.org>, <stable@...r.kernel.org>
Subject: Re: [PATCH stable 5.15/6.1/6.6] af_unix: Clear oob_skb in scan_inflight().

+Paolo

From: Greg Kroah-Hartman <gregkh@...uxfoundation.org>
Date: Wed, 5 Mar 2025 15:08:26 +0100
> On Mon, Mar 03, 2025 at 07:01:49PM -0800, Kuniyuki Iwashima wrote:
> > Embryo socket is not queued in gc_candidates, so we can't drop
> > a reference held by its oob_skb.
> > 
> > Let's say we create listener and embryo sockets, send the
> > listener's fd to the embryo as OOB data, and close() them
> > without recv()ing the OOB data.
> > 
> > There is a self-reference cycle like
> > 
> >   listener -> embryo.oob_skb -> listener
> > 
> > , so this must be cleaned up by GC.  Otherwise, the listener's
> > refcnt is not released and sockets are leaked:
> > 
> >   # unshare -n
> >   # cat /proc/net/protocols | grep UNIX-STREAM
> >   UNIX-STREAM 1024      0      -1   NI       0   yes  kernel ...
> > 
> >   # python3
> >   >>> from array import array
> >   >>> from socket import *
> >   >>>
> >   >>> s = socket(AF_UNIX, SOCK_STREAM)
> >   >>> s.bind('\0test\0')
> >   >>> s.listen()
> >   >>>
> >   >>> c = socket(AF_UNIX, SOCK_STREAM)
> >   >>> c.connect(s.getsockname())
> >   >>> c.sendmsg([b'x'], [(SOL_SOCKET, SCM_RIGHTS, array('i', [s.fileno()]))], MSG_OOB)
> >   1
> >   >>> quit()
> > 
> >   # cat /proc/net/protocols | grep UNIX-STREAM
> >   UNIX-STREAM 1024      3      -1   NI       0   yes  kernel ...
> >                         ^^^
> >                         3 sockets still in use after FDs are close()d
> > 
> > Let's drop the embryo socket's oob_skb ref in scan_inflight().
> > 
> > This also fixes a racy access to oob_skb that commit 9841991a446c
> > ("af_unix: Update unix_sk(sk)->oob_skb under sk_receive_queue
> > lock.") fixed for the new Tarjan's algo-based GC.
> > 
> > Fixes: 314001f0bf92 ("af_unix: Add OOB support")
> > Reported-by: Lei Lu <llfamsec@...il.com>
> > Signed-off-by: Kuniyuki Iwashima <kuniyu@...zon.com>
> > ---
> > This has no upstream commit because I replaced the entire GC in
> > 6.10 and the new GC does not have this bug, and this fix is only
> > applicable to the old GC (<= 6.9), thus for 5.15/6.1/6.6.
> 
> You need to get the networking maintainers to review and agree that this
> is ok for us to take, as we really don't want to take "custom" stuff
> like thi s at all.

Paolo, could you take a look at this patch ?
https://lore.kernel.org/netdev/20250304030149.82265-1-kuniyu@amazon.com/

> Why not just take the commits that are in newer
> kernels instead?

That will be about 20 patches that rewrite the most lines of
net/unix/garbage.c and cannot be applied cleanly.

I think backporting these commits is overkill to fix a small
bug that can be fixed with a much smaller diff.

927fa5b3e4f5 af_unix: Fix uninit-value in __unix_walk_scc()
041933a1ec7b af_unix: Fix garbage collection of embryos carrying OOB with SCM_RIGHTS
7172dc93d621 af_unix: Add dead flag to struct scm_fp_list.
1af2dface5d2 af_unix: Don't access successor in unix_del_edges() during GC.
fd86344823b5 af_unix: Try not to hold unix_gc_lock during accept().
118f457da9ed af_unix: Remove lock dance in unix_peek_fds().
4090fa373f0e af_unix: Replace garbage collection algorithm.
a15702d8b3aa af_unix: Detect dead SCC.
bfdb01283ee8 af_unix: Assign a unique index to SCC.
ad081928a8b0 af_unix: Avoid Tarjan's algorithm if unnecessary.
77e5593aebba af_unix: Skip GC if no cycle exists.
ba31b4a4e101 af_unix: Save O(n) setup of Tarjan's algo.
dcf70df2048d af_unix: Fix up unix_edge.successor for embryo socket.
3484f063172d af_unix: Detect Strongly Connected Components.
6ba76fd2848e af_unix: Iterate all vertices by DFS.
22c3c0c52d32 af_unix: Bulk update unix_tot_inflight/unix_inflight when queuing skb.
42f298c06b30 af_unix: Link struct unix_edge when queuing skb.
29b64e354029 af_unix: Allocate struct unix_edge for each inflight AF_UNIX fd.
1fbfdfaa5902 af_unix: Allocate struct unix_vertex for each inflight AF_UNIX fd.