netdev - Re: [PATCH net v2 1/2] af_unix: Fix garbage collection of embryos carrying OOB with 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: <734273bc-2415-43b7-9873-26416aab8900@rbox.co>
Date: Fri, 17 May 2024 07:59:16 +0200
From: Michal Luczaj <mhal@...x.co>
To: Kuniyuki Iwashima <kuniyu@...zon.com>
Cc: davem@...emloft.net, edumazet@...gle.com, kuba@...nel.org,
 netdev@...r.kernel.org, pabeni@...hat.com, shuah@...nel.org
Subject: Re: [PATCH net v2 1/2] af_unix: Fix garbage collection of embryos
 carrying OOB with SCM_RIGHTS

On 5/17/24 03:45, Kuniyuki Iwashima wrote:
> From: Michal Luczaj <mhal@...x.co>
> Date: Thu, 16 May 2024 16:50:09 +0200
>> GC attempts to explicitly drop oob_skb before purging the hit list.
> 
> Sorry for not catching these in v1,
> 
> nit: s/oob_skb/oob_skb's reference/

Argh, sorry, I've copy-pasted my own misformulation.

>> The problem is with embryos: kfree_skb(u->oob_skb) is never called on an
>> embryo socket, as those sockets are not directly stacked by the SCC walk.
> 
> ", as ..." is not correct and can be just removed.  Here we walk
> through embryos as written in the next paragraph but we forget
> dropping oob_skb's refcnt.

Oh, I agree we walk through embryos. I wrote that embryos are not _stacked_
by the SCC walk, i.e. embryos don't appear on the `vertex_stack`. But I
think you're right, such comment of mine would be incorrect anyway. So,
removing and resending.

>> The python script below [0] sends a listener's fd to its embryo as OOB
>> data.  While GC does collect the embryo's queue, it fails to drop the OOB
>> skb's refcount.  The skb which was in embryo's receive queue stays as
>> unix_sk(sk)->oob_skb and keeps the listener's refcount [1].
>>
>> Tell GC to dispose embryo's oob_skb.
>>
>> [0]:
>> from array import array
>> from socket import *
>>
>> addr = '\x00unix-oob'
>> lis = socket(AF_UNIX, SOCK_STREAM)
>> lis.bind(addr)
>> lis.listen(1)
>>
>> s = socket(AF_UNIX, SOCK_STREAM)
>> s.connect(addr)
>> scm = (SOL_SOCKET, SCM_RIGHTS, array('i', [lis.fileno()]))
>> s.sendmsg([b'x'], [scm], MSG_OOB)
>> lis.close()
>>
>> [1]
>> $ grep unix-oob /proc/net/unix
>> $ ./unix-oob.py
>> $ grep unix-oob /proc/net/unix
>> 0000000000000000: 00000002 00000000 00000000 0001 02     0 @unix-oob
>> 0000000000000000: 00000002 00000000 00010000 0001 01  6072 @unix-oob
>>
>> Fixes: 4090fa373f0e ("af_unix: Replace garbage collection algorithm.")
>> Signed-off-by: Michal Luczaj <mhal@...x.co>
> 
> with the above corrected, you can add
> 
> Reviewed-by: Kuniyuki Iwashima <kuniyu@...zon.com>
> 
> Thanks!

All right, thank you.

One question: git send-email automatically adds my Signed-off-by to your
patch (patch 2/2 in this series). Should I leave it that way?

>> ...