netdev - Re: [PATCH v1 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] [thread-next>] [day] [month] [year] [list]

Message-ID: <8015f2f2fec7d5a5a7164e1480d0e0c18b925f61.camel@redhat.com>
Date: Thu, 09 May 2024 11:12:38 +0200
From: Paolo Abeni <pabeni@...hat.com>
To: Kuniyuki Iwashima <kuniyu@...zon.com>, "David S. Miller"
	 <davem@...emloft.net>, Eric Dumazet <edumazet@...gle.com>, Jakub Kicinski
	 <kuba@...nel.org>
Cc: Kuniyuki Iwashima <kuni1840@...il.com>, netdev@...r.kernel.org, Billy
	Jheng Bing-Jhong
	 <billy@...rlabs.sg>
Subject: Re: [PATCH v1 net] af_unix: Update unix_sk(sk)->oob_skb under
 sk_receive_queue lock.

On Tue, 2024-05-07 at 10:00 -0700, Kuniyuki Iwashima wrote:
> Billy Jheng Bing-Jhong reported a race between __unix_gc() and
> queue_oob().
> 
> __unix_gc() tries to garbage-collect close()d inflight sockets,
> and then if the socket has MSG_OOB in unix_sk(sk)->oob_skb, GC
> will drop the reference and set NULL to it locklessly.
> 
> However, the peer socket still can send MSG_OOB message to the
> GC candidate and queue_oob() can update unix_sk(sk)->oob_skb
> concurrently, resulting in NULL pointer dereference. [0]
> 
> To avoid the race, let's update unix_sk(sk)->oob_skb under the
> sk_receive_queue's lock.

I'm sorry to delay this fix but...

AFAICS every time AF_UNIX touches the ooo_skb, it's under the receiver
unix_state_lock. The only exception is __unix_gc. What about just
acquiring such lock there? 

Otherwise there are other chunk touching the ooo_skb is touched where
this patch does not add the receive queue spin lock protection e.g. in
unix_stream_recv_urg(), making the code a bit inconsistent.

Thanks

Paolo