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: <0828c5fb-1e56-4bdf-b7dd-7ec4d7310c72@rbox.co>
Date: Thu, 16 May 2024 12:33:35 +0200
From: Michal Luczaj <mhal@...x.co>
To: Kuniyuki Iwashima <kuniyu@...zon.com>
Cc: davem@...emloft.net, edumazet@...gle.com, kuba@...nel.org,
 kuni1840@...il.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.

On 5/15/24 15:35, Kuniyuki Iwashima wrote:
> 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().

Right, I think I see what you mean.

>> 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".

I wholeheartedly concur with you!

>> 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.

All right, no problem. Does it mean you'll be posting PATCH 2/2 ("af_unix:
Update unix_sk(sk)->oob_skb under sk_receive_queue lock") to stable(s)?

Moving on to the New GC: Python test from this patch shows that the New GC
is memleaking in pretty much the same fashion.

$ grep splat /proc/net/unix
$ ./unix-oob-splat.py
$ rm unix-oob-splat
$ ./unix-oob-splat.py
$ grep splat /proc/net/unix
0000000000000000: 00000002 00000000 00000000 0001 02     0 unix-oob-splat
0000000000000000: 00000002 00000000 00000000 0001 02     0 unix-oob-splat
0000000000000000: 00000002 00000000 00010000 0001 01  6643 unix-oob-splat
0000000000000000: 00000002 00000000 00010000 0001 01  2920 unix-oob-splat

I've posted a patch:
https://lore.kernel.org/netdev/20240516103049.1132040-1-mhal@rbox.co/

I tried to align with your version of the commit message, but feel free to
chime in. Also, I took the liberty to introduce a small sanity check. Would
you prefer if I dropped this hunk or possibly made it a separate patch?

Thanks!