Re: [PATCH net v2] net/iucv: fix races in afiucv_netdev_event()

On 17/09/26 7:31 PM, Alexandra Winter wrote:
> 
> 
> On 17.09.26 09:17, Nagamani PV wrote:
>> afiucv_netdev_event() walks iucv_sk_list.head inside a bare
>> sk_for_each() with no lock held, while concurrent paths can modify
>> or destroy sockets in that list:
>>
>> BUG-1  Use-After-Free
>>   iucv_sock_kill() calls iucv_sock_unlink() under write_lock_bh
>>   followed by sock_put() which may free the sk.  If the notifier
>>   is mid-traversal when sock_put() runs, the stale sk pointer
>>   dereference at iucv = iucv_sk(sk) is a use-after-free.
>>   KASAN reports: slab-use-after-free in afiucv_netdev_event.
>>   Confirmed with KASAN on IBM Z.
>>
>> BUG-2  Locking correctness: iucv_send_ctrl() without lock_sock()
>>   iucv_send_ctrl() is called without holding lock_sock(sk).  The
>>   notifier reads iucv->hs_dev without the socket lock, while
>>   iucv_sock_close() writes iucv->hs_dev = NULL under lock_sock().
>>   iucv_send_ctrl() also reads and writes sk->sk_shutdown as bare
>>   accesses, racing with iucv_sock_close() which holds lock_sock()
>>   for the same fields.  KCSAN reports both races.
>>   Confirmed with KCSAN on IBM Z:
>>
>>     write to iucv->hs_dev of 8 bytes by task N on cpu M:
>>       iucv_sock_close+0x196 [af_iucv]    (under lock_sock)
>>
>>     read to iucv->hs_dev by task N on cpu M:
>>       afiucv_netdev_event [af_iucv]      (no lock held)
>>
>> BUG-3  sk_state data race
>>   sk->sk_state is written without lock_sock() in the notifier, racing
>>   with iucv_sock_close() writing the same field under lock_sock().
>>   KCSAN reports: data-race in afiucv_netdev_event / iucv_sock_close.
>>   Confirmed with KCSAN on IBM Z:
>>
>>     write to sk->sk_state by task N on cpu M:
>>       iucv_sock_close+0x196 [af_iucv]   (under lock_sock)
>>
>>     read to sk->sk_state by task N on cpu M:
>>       afiucv_netdev_event+0xa6 [af_iucv] (no lock held)
>>
>> Fix with a two-pass algorithm:
>>
>>   Pass 1 (read_lock_bh): walk iucv_sk_list, sock_hold() each
>>     matching socket, collect into a local list.  The read lock
>>     prevents concurrent write_lock_bh in iucv_sock_link/unlink
>>     from modifying the list while we take references.
>>
>>   Pass 2 (lock_sock per socket): for each collected socket,
>>     acquire lock_sock to serialise against iucv_sock_close(),
>>     check sk_state under the lock, call iucv_send_ctrl() and
>>     update sk_state safely, then release_sock() + sock_put().
>>
>> This eliminates all three races:
>>   - BUG-1: read_lock_bh prevents sk from being unlinked and freed
>>     while we hold a reference to it.
>>   - BUG-2: iucv_send_ctrl() is now called under lock_sock(), not
>>     racing with concurrent socket close.
>>   - BUG-3: sk_state is read and written under lock_sock(),
>>     serialising against iucv_sock_close().
>>
>> Pass 1 reads iucv_sk(sk)->hs_dev under read_lock_bh to filter
>> sockets belonging to the affected device.  iucv_sock_close() writes
>> hs_dev = NULL under lock_sock(), which is orthogonal to read_lock_bh.
>> Use READ_ONCE() for the Pass 1 read and WRITE_ONCE() for the
>> iucv_sock_close() write to document the intentional concurrent access
>> and suppress KCSAN false positives.  The read is safe: read_lock_bh
>> prevents the socket from being freed; if hs_dev is concurrently
>> cleared to NULL it will not match event_dev (a valid pointer) so the
>> socket is correctly skipped.
>>
>> Fixes: 9fbd87d41392 ("af_iucv: handle netdev events")
>> Reported-by: Bryam Vargas <[email protected]>
>> Link: https://lore.kernel.org/netdev/[email protected]/
>> Suggested-by: Hidayath Khan <[email protected]>
>> Cc: [email protected]
>> Signed-off-by: Nagamani PV <[email protected]>
>> ---
>> Changes since v1 (2-line read_lock_bh fix, posted 2026-08-03):
>>   - Redesign as two-pass algorithm to fix all races in one patch:
>>     Pass 1: read_lock_bh + sock_hold() to safely collect matching
>>     sockets; Pass 2: lock_sock() per socket to act on sk_state,
>>     call iucv_send_ctrl() and update sk_state under the lock.
>>   - Fixes Sashiko Finding 1 (New/High): sleep-in-atomic - V1 called
>>     iucv_send_ctrl() inside read_lock_bh(); Pass 2 runs after
>>     read_unlock_bh() so GFP_KERNEL allocation is safe.
>>   - Fixes Sashiko Finding 2 (Pre-existing/High): lockless manipulation
>>     of sk_state, sk_shutdown, sk_socket - all now under lock_sock().
>>   - Add WRITE_ONCE(iucv->hs_dev, NULL) in iucv_sock_close() and
>>     READ_ONCE(iucv_sk(sk)->hs_dev) in Pass 1 to document intentional
>>     concurrent access across orthogonal locks and suppress KCSAN.
>>   - All three bugs confirmed with KASAN + KCSAN on IBM Z with
>>     before/after TAP results.
>>   - Retarget from net-next to net (Fixes: + Cc: stable).
>>   - Update subject from "fix UAF" to "fix races" to reflect full scope.
>>
>> Bryam Vargas: your RFC identified the lockless socket manipulation in
>> afiucv_netdev_event() as part of your 17-context analysis.  I have
>> included Reported-by for that attribution.  Please let me know if you
>> are happy with this, or prefer a different tag.
> 
> 
> Nagamani,
> I have told you before, that I want us to work on a correct usage of
> of lock_sock() and  bh_lock_sock() in af_iucv.c (stage 1 and 2 in Bryam's plan [1])
>  before fixing the callers (stage 3).
> 
> The races you are referring to belong to the group that needs fixing.
> 
> afiucv_netdev_event() needs a lock, as Bryam mentioned. The 2 pass approach looks
> good to me in general. But without a proper implemenation of the lock_sock mechanism,
> it is far from complete.
> 
> Please hold it until Stage 1 (for TRANS_HIPER) is implemented.
> 

I'll hold the afiucv_netdev_event() fix until the Stage 1 locking
rework for TRANS_HIPER is implemented. We can then validate the fix
together with the rework.

> 
>>
>>  net/iucv/af_iucv.c | 57 ++++++++++++++++++++++++++++++++++++++++++----
>>  1 file changed, 52 insertions(+), 5 deletions(-)
>>
>> diff --git a/net/iucv/af_iucv.c b/net/iucv/af_iucv.c
>> index ea047bab65e7..b197f9a254a7 100644
>> --- a/net/iucv/af_iucv.c
>> +++ b/net/iucv/af_iucv.c
>> @@ -320,6 +320,12 @@ static void iucv_sock_unlink(struct iucv_sock_list *l, struct sock *sk)
>>  	write_unlock_bh(&l->lock);
>>  }
>>  
>> +/* Used by afiucv_netdev_event() two-pass algorithm */
>> +struct iucv_netdev_todo {
>> +	struct list_head list;
>> +	struct sock *sk;
>> +};
>> +
>>  /* Kill socket (only if zapped and orphaned) */
>>  static void iucv_sock_kill(struct sock *sk)
>>  {
>> @@ -445,7 +451,7 @@ static void iucv_sock_close(struct sock *sk)
>>  
>>  	if (iucv->hs_dev) {
>>  		dev_put(iucv->hs_dev);
>> -		iucv->hs_dev = NULL;
>> +		WRITE_ONCE(iucv->hs_dev, NULL);
>>  		sk->sk_bound_dev_if = 0;
>>  	}
>>  
>> @@ -2207,21 +2213,62 @@ static int afiucv_netdev_event(struct notifier_block *this,
>>  			       unsigned long event, void *ptr)
>>  {
>>  	struct net_device *event_dev = netdev_notifier_info_to_dev(ptr);
>> +	struct iucv_netdev_todo *entry, *tmp;
>> +	LIST_HEAD(todo);
>>  	struct sock *sk;
>> -	struct iucv_sock *iucv;
>>  
>>  	switch (event) {
>>  	case NETDEV_REBOOT:
>>  	case NETDEV_GOING_DOWN:
>> +		/*
>> +		 * Pass 1: collect matching sockets under read_lock_bh.
>> +		 *
>> +		 * read_lock_bh(&iucv_sk_list.lock) excludes concurrent
>> +		 * write_lock_bh in iucv_sock_link/unlink, so sk cannot
>> +		 * be removed from the list or freed while we walk it.
>> +		 * sock_hold() pins the sk so it survives after we drop
>> +		 * the lock.
>> +		 *
>> +		 * iucv_sock_close() writes hs_dev = NULL under lock_sock,
>> +		 * which is orthogonal to read_lock_bh.  READ_ONCE() documents
>> +		 * the intentional concurrent access: if hs_dev is being
>> +		 * cleared to NULL it will not equal event_dev (a valid
>> +		 * pointer) so the socket is correctly skipped.
>> +		 */
>> +		read_lock_bh(&iucv_sk_list.lock);
>>  		sk_for_each(sk, &iucv_sk_list.head) {
>> -			iucv = iucv_sk(sk);
>> -			if ((iucv->hs_dev == event_dev) &&
>> -			    (sk->sk_state == IUCV_CONNECTED)) {
>> +			if (READ_ONCE(iucv_sk(sk)->hs_dev) != event_dev)
>> +				continue;
>> +			entry = kmalloc_obj(*entry, GFP_ATOMIC);
>> +			if (!entry)
>> +				continue;
>> +			sock_hold(sk);
>> +			entry->sk = sk;
>> +			list_add_tail(&entry->list, &todo);
>> +		}
>> +		read_unlock_bh(&iucv_sk_list.lock);
>> +		/*
>> +		 * Pass 2: act on each socket under lock_sock.
>> +		 *
>> +		 * lock_sock() serialises against iucv_sock_close() and
>> +		 * sock_orphan(), so sk_state and sk_socket are stable.
>> +		 * iucv_send_ctrl() may call sock_alloc_send_skb(GFP_KERNEL)
>> +		 * which requires non-atomic context -- satisfied here because
>> +		 * we are no longer holding read_lock_bh.
>> +		 */
>> +		list_for_each_entry_safe(entry, tmp, &todo, list) {
>> +			sk = entry->sk;
>> +			lock_sock(sk);
>> +			if (sk->sk_state == IUCV_CONNECTED) {
>>  				if (event == NETDEV_GOING_DOWN)
>>  					iucv_send_ctrl(sk, AF_IUCV_FLAG_FIN);
>>  				sk->sk_state = IUCV_DISCONN;
>>  				sk->sk_state_change(sk);
>>  			}
>> +			release_sock(sk);
>> +			sock_put(sk);
>> +			list_del(&entry->list);
>> +			kfree(entry);
>>  		}
>>  		break;
>>  	case NETDEV_DOWN:
>