netdev - Re: [PATCH v4 net] af_unix: Update unix_sk(sk)->oob_skb under sk_receive

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] [day] [month] [year] [list]

Message-ID: <b11c0bdf-a7ea-4fdf-b07c-3034229aaa5f@rbox.co>
Date: Wed, 15 May 2024 11:28:49 +0200
From: Michal Luczaj <mhal@...x.co>
To: Kuniyuki Iwashima <kuniyu@...zon.com>
Cc: billy@...rlabs.sg, davem@...emloft.net, edumazet@...gle.com,
 kuba@...nel.org, kuni1840@...il.com, netdev@...r.kernel.org,
 pabeni@...hat.com
Subject: Re: [PATCH v4 net] af_unix: Update unix_sk(sk)->oob_skb under
 sk_receive_queue lock.

On 5/15/24 02:07, Kuniyuki Iwashima wrote:
> From: Michal Luczaj <mhal@...x.co>
> Date: Tue, 14 May 2024 12:13:36 +0200
>> On 5/14/24 04:52, Kuniyuki Iwashima wrote:
>>> ...
>>> diff --git a/net/unix/garbage.c b/net/unix/garbage.c
>>> index 0104be9d4704..b87e48e2b51b 100644
>>> --- a/net/unix/garbage.c
>>> +++ b/net/unix/garbage.c
>>> @@ -342,10 +342,12 @@ static void __unix_gc(struct work_struct *work)
>>>  		scan_children(&u->sk, inc_inflight, &hitlist);
>>>  
>>>  #if IS_ENABLED(CONFIG_AF_UNIX_OOB)
>>> +		spin_lock(&u->sk.sk_receive_queue.lock);
>>>  		if (u->oob_skb) {
>>> -			kfree_skb(u->oob_skb);
>>> +			WARN_ON_ONCE(skb_unref(u->oob_skb));
>>>  			u->oob_skb = NULL;
>>>  		}
>>> +		spin_unlock(&u->sk.sk_receive_queue.lock);
>>>  #endif
>>>  	}
>>
>> I've realised this part of GC is broken for embryos. And adding a rq lock
>> here turns a warning into a possible deadlock, so below is my attempt at
>> fixing the underlying problem.
> 
> Exactly, I missed that case.  It's memleak rather than deadlock.
>
> We need to traverse embryos from listener to drop OOB skb refcount
> in embroy recvq to drop listener fd's refcount.

In a way, yeah. See below.

>> It's based it on top of your patch, so should I post it now or wait until
>> your patch lands in net?
> 
> I'll post your patch within v5 that will minimise the delay given
> we are in rush for the merge window.

Awesome!

>> Subject: [PATCH] af_unix: Fix garbage collection of embryos carrying
>>  OOB/SCM_RIGHTS
>>
>> GC attempts to explicitly drop oob_skb before purging the hit list. The
> 
> s/oob_skb/oob_skb's refcount/

Ah, yeah, you're right.

>> problem is with embryos: instead of trying to kfree_skb(u->oob_skb) of an
>> embryo socket, GC goes for its parent-listener socket, which never carries
>> u->oob_skb. Effectively oob_skb is removed from the receive queue, but
>> remains reachable via u->oob_skb.
> 
> The last sentence is not correct as the listener does not have oob_skb and
> kfree_skb() is not called.

I was referring to embryo's oob_skb. Anyway, I took a look at your v5
series and I see you've changed my commit message in ways I disagree
with, so I'll comment there.

> I'll post this patch with some modification of commit message.
> 
> Thanks!