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

[Nagamani PV <nagamani@linux.ibm.com>]

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.

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