[PATCH net-next] net/iucv: fix UAF in afiucv_netdev_event()

[PATCH net-next] net/iucv: fix UAF in afiucv_netdev_event()

[Nagamani PV]

afiucv_netdev_event() traverses iucv_sk_list without holding
iucv_sk_list.lock.

A concurrent socket teardown can unlink and free an af_iucv socket via
iucv_sock_kill() while the netdevice notifier path is still traversing
the list, resulting in a use-after-free when dereferencing the socket.

syzbot reported a KASAN slab-use-after-free triggered from the netdevice
notifier path:

  BUG: KASAN: slab-use-after-free in afiucv_netdev_event+0x166/0x570
  Read of size 8 at addr 001a54009dcda368 by task syz.3.24/516

  Call Trace:
    afiucv_netdev_event+0x166/0x570 net/iucv/af_iucv.c:2193
    notifier_call_chain+0x18e/0x510 kernel/notifier.c:85
    call_netdevice_notifiers net/core/dev.c:2301 [inline]
    __dev_close_many+0x1c6/0x680 net/core/dev.c:1747
    unregister_netdevice_many_notify+0x9b6/0x2650 net/core/dev.c:12388
    unregister_netdevice_queue+0x392/0x3d0 net/core/dev.c:12291
    __tun_detach+0xa50/0x1990 drivers/net/tun.c:621

  Allocated by task 521:
    sk_prot_alloc+0xd2/0x230 net/core/sock.c:2247
    sk_alloc+0x48/0x680 net/core/sock.c:2303
    iucv_sock_alloc+0x4e/0x710 net/iucv/af_iucv.c:456
    iucv_sock_create+0x114/0x180 net/iucv/af_iucv.c:2254

  Freed by task 519:
    kfree+0x164/0x500 mm/slub.c:6561
    iucv_sock_release+0x12e/0x150 net/iucv/af_iucv.c:1482
    sock_close+0xa4/0x230 net/socket.c:1514

Protect the list traversal with the existing read-side lock.
Use read_lock_bh()/read_unlock_bh() to synchronize with
write_lock_bh()-protected updates to iucv_sk_list.

Fixes: 9fbd87d41392 ("af_iucv: handle netdev events")
Cc: stable@vger.kernel.org
Suggested-by: Hidayath Khan <hidayath@linux.ibm.com>
Signed-off-by: Nagamani PV <nagamani@linux.ibm.com>

---
 net/iucv/af_iucv.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/net/iucv/af_iucv.c b/net/iucv/af_iucv.c
index ea047bab65e7..034039c50941 100644
--- a/net/iucv/af_iucv.c
+++ b/net/iucv/af_iucv.c
@@ -2213,6 +2213,7 @@ static int afiucv_netdev_event(struct notifier_block *this,
 	switch (event) {
 	case NETDEV_REBOOT:
 	case NETDEV_GOING_DOWN:
+		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) &&
@@ -2223,6 +2224,7 @@ static int afiucv_netdev_event(struct notifier_block *this,
 				sk->sk_state_change(sk);
 			}
 		}
+		read_unlock_bh(&iucv_sk_list.lock);
 		break;
 	case NETDEV_DOWN:
 	case NETDEV_UNREGISTER:
-- 
2.53.0

Re: [PATCH net-next] net/iucv: fix UAF in afiucv_netdev_event()

[Alexandra Winter]

On 03.08.26 20:20, Nagamani PV wrote:
> afiucv_netdev_event() traverses iucv_sk_list without holding
> iucv_sk_list.lock.
> 
> A concurrent socket teardown can unlink and free an af_iucv socket via
> iucv_sock_kill() while the netdevice notifier path is still traversing
> the list, resulting in a use-after-free when dereferencing the socket.
> 
> syzbot reported a KASAN slab-use-after-free triggered from the netdevice
> notifier path:
> 
>   BUG: KASAN: slab-use-after-free in afiucv_netdev_event+0x166/0x570
>   Read of size 8 at addr 001a54009dcda368 by task syz.3.24/516
> 
>   Call Trace:
>     afiucv_netdev_event+0x166/0x570 net/iucv/af_iucv.c:2193
>     notifier_call_chain+0x18e/0x510 kernel/notifier.c:85
>     call_netdevice_notifiers net/core/dev.c:2301 [inline]
>     __dev_close_many+0x1c6/0x680 net/core/dev.c:1747
>     unregister_netdevice_many_notify+0x9b6/0x2650 net/core/dev.c:12388
>     unregister_netdevice_queue+0x392/0x3d0 net/core/dev.c:12291
>     __tun_detach+0xa50/0x1990 drivers/net/tun.c:621
> 
>   Allocated by task 521:
>     sk_prot_alloc+0xd2/0x230 net/core/sock.c:2247
>     sk_alloc+0x48/0x680 net/core/sock.c:2303
>     iucv_sock_alloc+0x4e/0x710 net/iucv/af_iucv.c:456
>     iucv_sock_create+0x114/0x180 net/iucv/af_iucv.c:2254
> 
>   Freed by task 519:
>     kfree+0x164/0x500 mm/slub.c:6561
>     iucv_sock_release+0x12e/0x150 net/iucv/af_iucv.c:1482
>     sock_close+0xa4/0x230 net/socket.c:1514
> 
> Protect the list traversal with the existing read-side lock.
> Use read_lock_bh()/read_unlock_bh() to synchronize with
> write_lock_bh()-protected updates to iucv_sk_list.
> 
> Fixes: 9fbd87d41392 ("af_iucv: handle netdev events")
> Cc: stable@vger.kernel.org
> Suggested-by: Hidayath Khan <hidayath@linux.ibm.com>
> Signed-off-by: Nagamani PV <nagamani@linux.ibm.com>
> 
> ---
>  net/iucv/af_iucv.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/net/iucv/af_iucv.c b/net/iucv/af_iucv.c
> index ea047bab65e7..034039c50941 100644
> --- a/net/iucv/af_iucv.c
> +++ b/net/iucv/af_iucv.c
> @@ -2213,6 +2213,7 @@ static int afiucv_netdev_event(struct notifier_block *this,
>  	switch (event) {
>  	case NETDEV_REBOOT:
>  	case NETDEV_GOING_DOWN:
> +		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) &&
> @@ -2223,6 +2224,7 @@ static int afiucv_netdev_event(struct notifier_block *this,
>  				sk->sk_state_change(sk);
>  			}
>  		}
> +		read_unlock_bh(&iucv_sk_list.lock);
>  		break;
>  	case NETDEV_DOWN:
>  	case NETDEV_UNREGISTER:

Nagmani,
As this is a fix, it should have been prefixed with net and not net-next.


See the discussions with Bryam Vargas [1]:
It is correct that afiucv_netdev_event() is missing lock protection.

However for a complete solution it should call lock_sock()
(because netdev events are called in process context) and handle
owned_by_user and a backlog queue.

So this patch improves the situation, but is not the complete solution.
I'd rather continue to work on a more complete fix than take this one,
but both ways are possible.

Kind regards
Alexandra

[1] https://lore.kernel.org/netdev/20260724222917.134769-1-hexlabsecurity@proton.me/

Re: [PATCH net-next] net/iucv: fix UAF in afiucv_netdev_event()

[Nagamani PV]

On 04/08/26 2:03 PM, Alexandra Winter wrote:
> 
> As this is a fix, it should have been prefixed with net and not net-next.
> 

Apologies for that — you had already corrected this in the internal
review. v2 will use [PATCH net] and include your Reviewed-by.

> 
> See the discussions with Bryam Vargas [1]:
> It is correct that afiucv_netdev_event() is missing lock protection.
> 
> However for a complete solution it should call lock_sock()
> (because netdev events are called in process context) and handle
> owned_by_user and a backlog queue.
> 
> So this patch improves the situation, but is not the complete solution.
> I'd rather continue to work on a more complete fix than take this one,
> but both ways are possible.
> 

Understood. This patch addresses the UAF on iucv_sk_list traversal
specifically. The lock_sock() concern is at a different level and
could be a follow-up.

Could you clarify your preference — take this as an incremental fix
with a follow-up for lock_sock(), or hold and work toward a combined
solution?

Thanks,
Nagamani

> Kind regards
> Alexandra
> 
> [1] https://lore.kernel.org/netdev/20260724222917.134769-1-hexlabsecurity@proton.me/
> 
> 
> 
> 

Re: [PATCH net-next] net/iucv: fix UAF in afiucv_netdev_event()

[Alexandra Winter]

On 04.08.26 14:52, Nagamani PV wrote:
>> I'd rather continue to work on a more complete fix than take this one,
>> but both ways are possible.
>>
> Understood. This patch addresses the UAF on iucv_sk_list traversal
> specifically. The lock_sock() concern is at a different level and
> could be a follow-up.
> 
> Could you clarify your preference — take this as an incremental fix
> with a follow-up for lock_sock(), or hold and work toward a combined
> solution?

I prefer to work on the combined solution

Re: [PATCH net-next] net/iucv: fix UAF in afiucv_netdev_event()

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

New issues:
- [High] The patch introduces a sleep-in-atomic-context bug by calling a sleepable memory allocation function while holding a spinlock (`read_lock_bh`) that disables bottom halves.

Pre-existing issues:
- [High] Socket state, shutdown flags, and transmissions are manipulated locklessly, leading to data races, lost state updates, and a potential NULL pointer dereference if the socket is orphaned concurrently.
--

--- Patch [1]: [PATCH net-next] net/iucv: fix UAF in afiucv_netdev_event() ---
commit 295060a75254f1d540b1b59d065d45ebee09c8e8
Author: Nagamani PV <nagamani@linux.ibm.com>
Subject: net/iucv: fix UAF in afiucv_netdev_event()

This commit aims to fix a KASAN slab-use-after-free in afiucv_netdev_event() by
protecting the socket list traversal with read_lock_bh() / read_unlock_bh().
This prevents concurrent socket teardown from freeing the socket during
iteration.

> diff --git a/net/iucv/af_iucv.c b/net/iucv/af_iucv.c
> index ea047bab65e7c..034039c509418 100644
> --- a/net/iucv/af_iucv.c
> +++ b/net/iucv/af_iucv.c

[ ... ]

> @@ -2213,6 +2213,7 @@ static int afiucv_netdev_event(struct notifier_block *this,
>  	switch (event) {
>  	case NETDEV_REBOOT:
>  	case NETDEV_GOING_DOWN:
> +		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)) {

[Severity: High]
This isn't a bug introduced by this patch, but does checking and modifying
the socket state here without taking the socket lock create a data race?

If a concurrent iucv_sock_release() sets the state to closed and calls
sock_orphan(sk) (which sets sk->sk_socket = NULL) before waiting on
iucv_sk_list.lock, could this event loop read the old state, get delayed,
and invoke iucv_send_ctrl() after the socket is orphaned?

When sock_alloc_send_pskb() is subsequently called inside iucv_send_ctrl(),
if the socket's write buffer is full, it tries to set SOCK_NOSPACE via
set_bit(SOCK_NOSPACE, &sk->sk_socket->flags). Would this result in a NULL
pointer dereference since sk->sk_socket was cleared?

The iucv_send_ctrl() function also directly manipulates sk->sk_shutdown
temporarily without locking the socket:

iucv_send_ctrl() {
    if (sk->sk_shutdown & SEND_SHUTDOWN) {
        shutdown = sk->sk_shutdown;
        sk->sk_shutdown &= RCV_SHUTDOWN;
    }
    ...
}

Does this create a race window that loses concurrent shutdown updates?

>  				if (event == NETDEV_GOING_DOWN)
>  					iucv_send_ctrl(sk, AF_IUCV_FLAG_FIN);

[Severity: High]
Does this introduce a sleep-in-atomic-context bug?

By acquiring read_lock_bh() earlier in afiucv_netdev_event(), the code now
executes in an atomic context. However, iucv_send_ctrl() performs a memory
allocation that can block:

afiucv_netdev_event()
   read_lock_bh(&iucv_sk_list.lock)
   iucv_send_ctrl()
      sock_alloc_send_skb()

The allocation relies on sk->sk_allocation (which defaults to
GFP_KERNEL | GFP_DMA for AF_IUCV sockets). Since these flags contain
__GFP_DIRECT_RECLAIM, will this allow the allocator to sleep and trigger
a "scheduling while atomic" panic?

>  				sk->sk_state = IUCV_DISCONN;
>  				sk->sk_state_change(sk);
>  			}
>  		}
> +		read_unlock_bh(&iucv_sk_list.lock);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260803182053.2355882-1-nagamani@linux.ibm.com?part=1

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

[Nagamani PV]

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

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

[sashiko-bot]

> 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
> [ ... ]
> 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>

Sashiko has reviewed this patch and found no issues. It looks great!

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260917071706.23831-1-nagamani@linux.ibm.com?part=1

Re: [PATCH net-next] net/iucv: fix UAF in afiucv_netdev_event()

[Nagamani PV]

On 04/08/26 11:51 PM, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
> 
> New issues:
> - [High] The patch introduces a sleep-in-atomic-context bug by calling a sleepable memory allocation function while holding a spinlock (`read_lock_bh`) that disables bottom halves.
> 

Correct.  Fixed in v2 by a two-pass design: iucv_send_ctrl() is only
called in Pass 2, after read_unlock_bh().  Pass 1 uses GFP_ATOMIC.

> Pre-existing issues:
> - [High] Socket state, shutdown flags, and transmissions are manipulated locklessly, leading to data races, lost state updates, and a potential NULL pointer dereference if the socket is orphaned concurrently.
> 

Correct.  All three sub-findings confirmed with KCSAN on IBM Z.
Fixed in v2: Pass 2 acquires lock_sock() per socket, serialising
sk->sk_state, sk->sk_shutdown, and iucv_send_ctrl() against
iucv_sock_close() and sock_orphan().

Pass 1 reads iucv->hs_dev under read_lock_bh while iucv_sock_close()
writes hs_dev = NULL under lock_sock() — orthogonal locks. READ_ONCE()
/ WRITE_ONCE() document the intentional concurrent access and suppress
KCSAN reports on those accesses.

v2 posted to netdev:
  https://lore.kernel.org/netdev/20260917071706.23831-1-nagamani@linux.ibm.com/

Validated with KASAN + KCSAN TAP on IBM Z.

Thanks,
Nagamani PV

Re: [PATCH net-next] net/iucv: fix UAF in afiucv_netdev_event()

[Nagamani PV]

On 04/08/26 7:00 PM, Alexandra Winter wrote:
> 
> 
> On 04.08.26 14:52, Nagamani PV wrote:
>>> I'd rather continue to work on a more complete fix than take this one,
>>> but both ways are possible.
>>>
>> Understood. This patch addresses the UAF on iucv_sk_list traversal
>> specifically. The lock_sock() concern is at a different level and
>> could be a follow-up.
>>
>> Could you clarify your preference — take this as an incremental fix
>> with a follow-up for lock_sock(), or hold and work toward a combined
>> solution?
> 
> I prefer to work on the combined solution

The combined solution is now posted as [PATCH net v2]:

  https://lore.kernel.org/netdev/20260917071706.23831-1-nagamani@linux.ibm.com/

The v2 fixes all three races in one patch using a two-pass algorithm:
Pass 1 under read_lock_bh() collects matching sockets with sock_hold();
Pass 2 under lock_sock() per socket calls iucv_send_ctrl() and updates
sk_state, serialising against iucv_sock_close() and sock_orphan().

All three bugs confirmed with KASAN + KCSAN TAP on IBM Z.

Thanks,
Nagamani PV

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

[Alexandra Winter]

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:

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

[Nagamani PV]

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

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

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

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 7 potential
issue(s) to consider.

Critical: 0 · High: 1 · Medium: 5 · Low: 1

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [High] afiucv_netdev_event() Pass 2 calls lock_sock(sk) for every
  collected socket while the netdevice notifier chain is executing with…
- [Medium] Pass 1 of afiucv_netdev_event() now needs a GFP_ATOMIC
  allocation per affected socket (net/iucv/af_iucv.c:2252).
- [Medium] Pass 2 of afiucv_netdev_event() checks only `sk->sk_state ==
  IUCV_CONNECTED` and then calls iucv_send_ctrl(), which dereferences…
- [Medium] The commit claims BUG-3 ("sk_state data race") is eliminated
  by taking lock_sock() in the notifier, but af_iucv's softirq…
- [Medium] The patch documents (and annotates with READ_ONCE/WRITE_ONCE)
  an invariant that iucv->hs_dev is only concurrently written by…
- [Medium] The new Pass-2 comment in afiucv_netdev_event()
  (net/iucv/af_iucv.c:2263-2264) states that "lock_sock() serialises…
- [Low] The patch introduces a marked-access contract for
  iucv_sock->hs_dev — WRITE_ONCE(iucv->hs_dev, NULL) in…

--

net/iucv: fix races in afiucv_netdev_event()

This reworks afiucv_netdev_event() into a two-pass algorithm: pass one walks
iucv_sk_list under read_lock_bh() and collects matching sockets with
sock_hold() into a local list, pass two takes lock_sock() per socket to send
the FIN and update sk_state.  It also converts the hs_dev clearing in
iucv_sock_close() to WRITE_ONCE() and the notifier read to READ_ONCE().

Link: https://lore.kernel.org/netdev/20260724222917.134769-1-hexlabsecurity@proton.me/

> diff --git a/net/iucv/af_iucv.c b/net/iucv/af_iucv.c
> index db261ecd19af1..15f5efb405abd 100644
> --- a/net/iucv/af_iucv.c
> +++ b/net/iucv/af_iucv.c

[ ... ]

> @@ -448,7 +454,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);

[Severity: Low]
Should the other lockless hs_dev accesses be marked as well?  Two of the
four sites that touch hs_dev without the socket lock are left as plain
accesses by this patch:

net/iucv/af_iucv.c:afiucv_hs_rcv() {
	read_lock(&iucv_sk_list.lock);
	sk_for_each(sk, &iucv_sk_list.head) {
		if (iucv_sk(sk)->hs_dev != dev)
			continue;
	...
}

net/iucv/af_iucv.c:afiucv_hs_callback_syn() {
	...
	niucv->hs_dev = iucv->hs_dev;
	dev_hold(niucv->hs_dev);
	...
}

With the plain read in afiucv_hs_rcv() still present, is the KCSAN report
the changelog says this suppresses (marked write in iucv_sock_close() versus
unmarked read) still reachable?  And the new READ_ONCE() in pass one is
paired against the unmarked store in afiucv_hs_callback_syn().

>  		sk->sk_bound_dev_if = 0;
>  	}
>  
> @@ -2217,21 +2223,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.
> +		 */

[Severity: Medium]
Is iucv_sock_close() really the only concurrent writer of hs_dev?
afiucv_hs_callback_syn() also writes it from softirq context:

net/iucv/af_iucv.c:afiucv_hs_callback_syn() {
	nsk = iucv_sock_alloc(NULL, sk->sk_protocol, GFP_ATOMIC, 0);
	bh_lock_sock(sk);
	...
	niucv->hs_dev = iucv->hs_dev;
	dev_hold(niucv->hs_dev);
	...
	nsk->sk_state = IUCV_CONNECTED;
}

iucv_sock_alloc() ends with iucv_sock_link(&iucv_sk_list, sk), so the child
is already visible in iucv_sk_list before hs_dev and sk_state are written,
and the only lock held here is the parent's bh_lock_sock().

That means read_lock_bh() in pass one does not exclude this writer, and
lock_sock(nsk) in pass two does not exclude the softirq that is still
finishing the child's setup.  Can the notifier then send a FIN on a child
socket in parallel with its SYN-ACK handling, or skip a child that turns
IUCV_CONNECTED on the device that is going down?

Could the comment be extended to cover this writer?

> +		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;

[Severity: Medium]
What happens to a matching socket when this GFP_ATOMIC allocation fails?

The continue skips sock_hold() and the todo entry, so pass two never runs
for that socket: no FIN is sent through iucv_send_ctrl() on
NETDEV_GOING_DOWN, sk_state stays IUCV_CONNECTED with iucv->hs_dev still
pointing at the device going down, and sk_state_change() is never called.

Waiters depend on that transition:

net/iucv/af_iucv.c:iucv_sock_recvmsg() {
	if ((sk->sk_state == IUCV_DISCONN) &&
	    skb_queue_empty(&iucv->backlog_skb_q) && ...)
		return 0;
}

net/iucv/af_iucv.c:iucv_sock_poll() {
	if (sk->sk_state == IUCV_DISCONN)
		mask |= EPOLLIN;
}

There is no retry either, since the later notifications do nothing:

	case NETDEV_DOWN:
	case NETDEV_UNREGISTER:
	default:
		break;

The old loop needed no memory at all and could not skip a matching socket.
Could this be done without a per-socket allocation, for example sock_hold()
one socket, drop the list lock, act on it and restart the walk, or with a
list node embedded in struct iucv_sock?

The changelog describes the change purely as a race fix; should it mention
that a socket can now be skipped entirely under memory pressure?

> +			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.

[Severity: Medium]
Does lock_sock() serialise against sock_orphan() here?

net/iucv/af_iucv.c:iucv_sock_release() {
	iucv_sock_close(sk);

	sock_orphan(sk);
	iucv_sock_kill(sk);
}

iucv_sock_close() takes and releases the socket lock internally, so
sock_orphan() runs with the socket lock not held, and sock_orphan() itself
uses a different lock:

include/net/sock.h:sock_orphan() {
	write_lock_bh(&sk->sk_callback_lock);
	sock_set_flag(sk, SOCK_DEAD);
	sk_set_socket(sk, NULL);
	sk->sk_wq = NULL;
	write_unlock_bh(&sk->sk_callback_lock);
}

The sk_socket claim matters because iucv_send_ctrl() ->
sock_alloc_send_pskb() dereferences sk->sk_socket->flags on the no-space
path.  Similarly, sk_state is written by the softirq callbacks under
bh_lock_sock() only, and there is no sock_owned_by_user() check anywhere in
this file.  Could the comment be narrowed to what lock_sock() actually
excludes?

> +		 * 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);

[Severity: High]
Can this stall the netdevice notifier chain while rtnl_lock is held?

afiucv_netdev_event() runs from the notifier chain with rtnl_lock held
(NETDEV_GOING_DOWN via __dev_close_many(), NETDEV_REBOOT via the reboot
notifier), and lock_sock() here waits for whoever currently owns the socket.

An AF_IUCV sender owns the socket lock across the whole send:

net/iucv/af_iucv.c:iucv_sock_sendmsg() {
	lock_sock(sk);
	...
	skb = sock_alloc_send_pskb(sk, headroom + linear, len - linear,
				   noblock, &err, 0);
	...
}

and the wait for send space does not drop that lock:

net/core/sock.c:sock_wait_for_wmem() {
	/* It is almost wait_for_tcp_memory minus release_sock/lock_sock. */
	...
	timeo = schedule_timeout(timeo);
	...
}

sk_sndtimeo starts at IUCV_CONN_TIMEOUT (40 * HZ) for AF_IUCV, and
setsockopt(SO_SNDTIMEO, {0,0}) makes it MAX_SCHEDULE_TIMEOUT:

net/core/sock.c:sock_set_timeout() {
	val = MAX_SCHEDULE_TIMEOUT;
	if ((tv.tv_sec || tv.tv_usec) &&
	    (tv.tv_sec < (MAX_SCHEDULE_TIMEOUT / HZ - 1)))
		val = tv.tv_sec * HZ + ...;
}

The queued TX skbs that hold sk_wmem_alloc belong to the device being torn
down and are only purged by dev_deactivate_many(), which runs after the
notifier chain returns, so the shutdown itself does not release the waiter.

The pre-patch notifier never blocked on a socket lock.  Would a
bh_lock_sock() plus sock_owned_by_user() check, or deferring pass two to a
work item, avoid holding rtnl_lock across an unbounded socket-lock wait?

> +			if (sk->sk_state == IUCV_CONNECTED) {
>  				if (event == NETDEV_GOING_DOWN)
>  					iucv_send_ctrl(sk, AF_IUCV_FLAG_FIN);

[Severity: Medium]
Pass one no longer filters on sk_state, and pass two never re-checks hs_dev
against event_dev under the socket lock.  Can iucv->hs_dev be NULL by the
time iucv_send_ctrl() dereferences it?

net/iucv/af_iucv.c:iucv_send_ctrl() {
	blen = sizeof(struct af_iucv_trans_hdr) +
	       LL_RESERVED_SPACE(iucv->hs_dev);
	...
}

The dereference happens before afiucv_hs_send()'s later "if (!skb->dev)"
check can catch it.  A socket collected in IUCV_BOUND state (for example one
blocked in connect() waiting for the SYN-ACK) can reach pass two as
IUCV_CONNECTED with hs_dev already cleared:

net/iucv/af_iucv.c:iucv_sock_close() {
	default:
		iucv_sever_path(sk, 1);      /* IUCV_BOUND: state unchanged */
	}

	if (iucv->hs_dev) {
		dev_put(iucv->hs_dev);
		WRITE_ONCE(iucv->hs_dev, NULL);
	...
}

net/iucv/af_iucv.c:afiucv_hs_callback_synack() {
	if (!iucv || sk->sk_state != IUCV_BOUND) { ... }
	bh_lock_sock(sk);
	...
	sk->sk_state = IUCV_CONNECTED;
}

afiucv_hs_callback_synack() only tests sk_state, so it promotes the closed
but still IUCV_BOUND socket to IUCV_CONNECTED with hs_dev == NULL, and pass
two then calls iucv_send_ctrl() on it.  The same window also lets pass two
call sock_alloc_send_pskb() on an orphaned socket, which dereferences
sk->sk_socket->flags.

The changelog says "BUG-2: iucv_send_ctrl() is now called under lock_sock(),
not racing with concurrent socket close" - should pass two also re-validate
iucv->hs_dev == event_dev under the socket lock?

>  				sk->sk_state = IUCV_DISCONN;

[Severity: Medium]
The changelog states "BUG-3: sk_state is read and written under lock_sock(),
serialising against iucv_sock_close()".  Is that the full picture for
sk_state?

The softirq handlers in this file write sk_state with only bh_lock_sock():

net/iucv/af_iucv.c:afiucv_hs_callback_fin() {
	bh_lock_sock(sk);
	if (sk->sk_state == IUCV_CONNECTED) {
		sk->sk_state = IUCV_DISCONN;
	...
}

and the TX-completion path takes no socket lock at all:

net/iucv/af_iucv.c:afiucv_hs_callback_txnotify() {
	default:
		atomic_dec(&iucv->skbs_in_xmit);
		if (sk->sk_state == IUCV_CONNECTED) {
			sk->sk_state = IUCV_DISCONN;
	...
}

lock_sock() sets sk_lock.owned and then drops sk_lock.slock:

net/core/sock.c:lock_sock_nested() {
	spin_lock_bh(&sk->sk_lock.slock);
	...
	sk->sk_lock.owned = 1;
	spin_unlock_bh(&sk->sk_lock.slock);
}

so bh_lock_sock() users that never test sock_owned_by_user() are not
excluded, and grepping net/iucv/af_iucv.c for sock_owned_by_user() finds no
match.  Can the transition performed here be lost, or clobber a concurrent
softirq transition, while pass two sleeps inside iucv_send_ctrl() ->
sock_alloc_send_skb(GFP_KERNEL)?  Should the BUG-3 claim be scoped to
iucv_sock_close() only?

>  				sk->sk_state_change(sk);
>  			}
> +			release_sock(sk);
> +			sock_put(sk);
> +			list_del(&entry->list);
> +			kfree(entry);
>  		}
>  		break;
>  	case NETDEV_DOWN:

Thanks for looking at these.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260917071706.23831-1-nagamani%40linux.ibm.com