netdev - Re: [PATCH v5 net 1/2] af_unix: Fix garbage collection of embryos carrying OOB/SCM

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: <20240515133547.47276-1-kuniyu@amazon.com>
Date: Wed, 15 May 2024 22:35:47 +0900
From: Kuniyuki Iwashima <kuniyu@...zon.com>
To: <mhal@...x.co>
CC: <davem@...emloft.net>, <edumazet@...gle.com>, <kuba@...nel.org>,
	<kuni1840@...il.com>, <kuniyu@...zon.com>, <netdev@...r.kernel.org>,
	<pabeni@...hat.com>
Subject: Re: [PATCH v5 net 1/2] af_unix: Fix garbage collection of embryos carrying OOB/SCM_RIGHTS.

From: Michal Luczaj <mhal@...x.co>
Date: Wed, 15 May 2024 11:34:51 +0200
> On 5/15/24 02:32, Kuniyuki Iwashima wrote:
> > ...
> > The python script below [0] sends a listener's fd to its embryo as OOB
> > data.  Then, GC does not iterates the embryo from the listener to drop
> > the OOB skb's refcount, and the skb in embryo's receive queue keeps the
> > listener's refcount.  As a result, the listener is leaked and the warning
> > [1] is hit.
> > ...
> 
> Sorry, this does not convey what I wrote. And I think your edit is
> incorrect.
> 
> GC starts from the in-flight listener and *does* iterate the embryo; see
> scan_children() where scan_inflight() is called for all the embryos.

I meant the current code does not call skb_unref() for embryos's OOB skb
because it's done _after_ scan_inflight(), not in scan_inflight().


> The skb in embryo's RQ *does not* keep the listener's refcount; skb from RQ
> ends up in the hit list and is purged.

unix_sk(sk)->oob_skb is a pointer to skb in recvq.  Perhaps I should
have written "the skb which was in embryo's receive queue stays as
unix_sk(sk)->oob_skb and keeps the listener's refcount".


> It is embryo's oob_skb that holds the refcount; see how __unix_gc() goes
> over gc_candidates attempting to kfree_skb(u->oob_skb), notice that `u`
> here is a listener, not an embryo.
> 
> I understand you're "in rush for the merge window", but would it be okay if
> I ask you not to edit my commit messages so heavily?

I noticed the new gc code was merged in Linus' tree.  It's still not
synced with net.git, but I guess it will be done soon and your patch
will not apply on net.git.  Then, I cannot include your patch as a
series, so please feel free to send it to each stable tree.

Thanks