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

[Alexandra Winter <wintera@linux.ibm.com>]

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 <hexlabsecurity@proton.me>
> Link: https://lore.kernel.org/netdev/20260724222917.134769-1-hexlabsecurity@proton.me/
> Suggested-by: Hidayath Khan <hidayath@linux.ibm.com>
> Cc: stable@vger.kernel.org
> Signed-off-by: Nagamani PV <nagamani@linux.ibm.com>
> ---
> 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.


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