[PATCH] iucv: fix lockless sk_for_each in afiucv_netdev_event

[PATCH] iucv: fix lockless sk_for_each in afiucv_netdev_event

[Haoran Wang]

afiucv_netdev_event() traverses iucv_sk_list.head with sk_for_each()
without holding iucv_sk_list.lock:

    /* net/iucv/af_iucv.c:2218 */
    sk_for_each(sk, &iucv_sk_list.head) {
        iucv = iucv_sk(sk);
        if ((iucv->hs_dev == event_dev) &&
            (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);
        }
    }

The list is mutated under write_lock_bh() by iucv_sock_link() and
iucv_sock_unlink() (called from the socket create/close paths), so a
concurrent close can unlink an sk while the notifier is traversing the
hlist.  The sk_for_each() next pointer may dereference an sk that is
being freed, and the per-socket work then runs on an sk whose lifetime
is not held. 

The other traversals of the same list already take the lock.
iucv_callback_connreq() and afiucv_hs_rcv() both wrap sk_for_each() in
read_lock(&iucv_sk_list.lock) ... read_unlock(). 

Take iucv_sk_list.lock around the traversal, matching the other two
sites.  The per-socket work (iucv_send_ctrl() reaching
sock_alloc_send_skb(), and the state change) may sleep on allocation
and cannot run under read_lock_bh(), so the fix collects the matching
sockets in two passes under the lock.  Count them first, then on the
second pass take a reference with sock_hold() on each and store it
into a kmalloc'd array, drop the lock, and run iucv_send_ctrl(),
sk_state_change() and sock_put() outside it.  sock_hold() pins each
sk for the out-of-lock work, so a concurrent close can no longer free
it from under the notifier.


Signed-off-by: Haoran Wang <haoranwangsec@gmail.com>
---
 net/iucv/af_iucv.c | 45 ++++++++++++++++++++++++++++++++++++++-------
 1 file changed, 38 insertions(+), 7 deletions(-)

diff --git a/net/iucv/af_iucv.c b/net/iucv/af_iucv.c
index 4e5cc9da6..d2977b018 100644
--- a/net/iucv/af_iucv.c
+++ b/net/iucv/af_iucv.c
@@ -2214,18 +2214,49 @@ static int afiucv_netdev_event(struct notifier_block *this,
 
 	switch (event) {
 	case NETDEV_REBOOT:
-	case NETDEV_GOING_DOWN:
+	case NETDEV_GOING_DOWN: {
+		struct sock **matches;
+		int i, n = 0, count = 0;
+
+		read_lock(&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)
+				count++;
+		}
+		read_unlock(&iucv_sk_list.lock);
+
+		if (!count)
+			break;
+		matches = kmalloc_array(count, sizeof(*matches), GFP_KERNEL);
+		if (!matches)
+			break;
+
+		read_lock(&iucv_sk_list.lock);
 		sk_for_each(sk, &iucv_sk_list.head) {
+			if (n == count)
+				break;
 			iucv = iucv_sk(sk);
-			if ((iucv->hs_dev == event_dev) &&
-			    (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);
+			if (iucv->hs_dev == event_dev &&
+			    sk->sk_state == IUCV_CONNECTED) {
+				sock_hold(sk);
+				matches[n++] = sk;
 			}
 		}
+		read_unlock(&iucv_sk_list.lock);
+
+		for (i = 0; i < n; i++) {
+			sk = matches[i];
+			if (event == NETDEV_GOING_DOWN)
+				iucv_send_ctrl(sk, AF_IUCV_FLAG_FIN);
+			sk->sk_state = IUCV_DISCONN;
+			sk->sk_state_change(sk);
+			sock_put(sk);
+		}
+		kfree(matches);
 		break;
+	}
 	case NETDEV_DOWN:
 	case NETDEV_UNREGISTER:
 	default:

Re: [PATCH] iucv: fix lockless sk_for_each in afiucv_netdev_event

[netdev-bot+sinfo]

Hi!

This is an automated message. This series looks like a fix, but its
commit messages seem to be missing some information:

 - How the issue was discovered, e.g. hit in production, hit during
   development, syzbot report, manual code inspection, LLM or static
   analysis tool scan.

 - Whether the issue was actually triggered, or is only theoretical
   (e.g. found by code inspection). If it was triggered please include
   the symptoms, like the stack trace or error messages.

Please do not repost the series just to address the above. Instead,
reply to this email with the missing information, so that reviewers
can take it into account. If the series needs another revision for
other reasons, please include the information in the commit messages
then.

The evaluation is done by an LLM so it may be wrong, if you think
that is the case please reply and explain.

Re: [PATCH] iucv: fix lockless sk_for_each in afiucv_netdev_event

[Alexandra Winter]

On 19.10.26 15:22, Haoran Wang wrote:
> afiucv_netdev_event() traverses iucv_sk_list.head with sk_for_each()
> without holding iucv_sk_list.lock:
> 
>     /* net/iucv/af_iucv.c:2218 */
>     sk_for_each(sk, &iucv_sk_list.head) {
>         iucv = iucv_sk(sk);
>         if ((iucv->hs_dev == event_dev) &&
>             (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);
>         }
>     }
> 

Hello Haoran Wang,
thank you for your contribution.
I think the issue you are fixing is the same that Nagamani already sent a fix for [1].

We are currently working on hardening the basic socket locking mechanism in af_iucv [2],
and I asked Nagamani to hold off until that work is done.

So I think this issue is already being worked on (please correct me, if I'm wrong).
Feel free to comment on Nagamani's proposal, as you have an understanding of the issue.

Kind regards
Alexandra


Link: https://lore.kernel.org/netdev/20260803182053.2355882-1-nagamani@linux.ibm.com/ [1]
Link: https://lore.kernel.org/netdev/20260724222917.134769-1-hexlabsecurity@proton.me/ [2]

Re: [PATCH] iucv: fix lockless sk_for_each in afiucv_netdev_event

[netdev-bot+sashiko]

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

Critical: 0 · High: 2 · Medium: 2 · 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] Race with concurrent close() that can dereference a NULL hs_dev,
  in afiucv_netdev_event() (net/iucv/af_iucv.c).
- [High] Self-deadlock on iucv_sk_list.lock in afiucv_netdev_event()
  (net/iucv/af_iucv.c, lines 2229 and 2244).
- [Medium] New failure path in afiucv_netdev_event() (net/iucv/af_iucv.c,
  lines 2240-2242) that skips the whole disconnect.
- [Medium] In afiucv_netdev_event() (net/iucv/af_iucv.c), the second pass
  stops at `if (n == count) break;`, but count was taken in an earlier,…
- [Low] The commit fixes a user-reachable use-after-free: the lockless
  sk_for_each() in afiucv_netdev_event() races with iucv_sock_kill() ->…

--

iucv: fix lockless sk_for_each in afiucv_netdev_event

This patch takes iucv_sk_list.lock around the sk_for_each() traversal in
afiucv_netdev_event(). Matching sockets are counted in a first pass. An
array is then allocated and filled with sock_hold() references in a second
pass, and iucv_send_ctrl(), the state change and sock_put() run after the
lock is dropped.

> Signed-off-by: Haoran Wang <haoranwangsec@gmail.com>

[Severity: Low]
This isn't a bug, but this fixes a use-after-free in the lockless
traversal. Should it carry a Fixes: tag, and possibly Cc: stable? The
function and its lockless sk_for_each() were added by:

Fixes: 9fbd87d41392 ("af_iucv: handle netdev events")

The sk_for_each() line itself was changed later by b67bfe0d42cac.

> diff --git a/net/iucv/af_iucv.c b/net/iucv/af_iucv.c
> index db261ecd19af..20c8943b85cd 100644
> --- a/net/iucv/af_iucv.c
> +++ b/net/iucv/af_iucv.c
> @@ -2222,18 +2222,49 @@ static int afiucv_netdev_event(struct notifier_block *this,
>  
>  	switch (event) {
>  	case NETDEV_REBOOT:
> -	case NETDEV_GOING_DOWN:
> +	case NETDEV_GOING_DOWN: {
> +		struct sock **matches;
> +		int i, n = 0, count = 0;
> +
> +		read_lock(&iucv_sk_list.lock);

[Severity: High]
Can this deadlock against a softirq writer on the same CPU?

The notifier runs in process context under RTNL with BH enabled. The same
rwlock is also write-locked from softirq context:

afiucv_hs_rcv()                 /* netif RX softirq */
  afiucv_hs_callback_syn()
    iucv_sock_alloc(NULL, ..., GFP_ATOMIC, 0)
      iucv_sock_link()
        write_lock_bh(&l->lock);

The IUCV tasklet path iucv_callback_connreq()->iucv_sock_alloc() reaches
the same writer. So do the error branches through
iucv_sock_kill()->iucv_sock_unlink().

Suppose a HiperSockets SYN arrives and NET_RX softirq runs on the CPU that
holds the read lock in either pass. Wouldn't the writer then spin forever,
waiting for a reader that can't run again on that CPU?

The commit message says this matches the other two sites, but
iucv_callback_connreq() and afiucv_hs_rcv() already run with BH disabled.
The other process context users, iucv_sock_bind(), iucv_sock_autobind()
and iucv_sock_link()/iucv_sock_unlink(), all use the _bh variants.

Should both passes use read_lock_bh()/read_unlock_bh() instead?

>  		sk_for_each(sk, &iucv_sk_list.head) {
>  			iucv = iucv_sk(sk);
> -			if ((iucv->hs_dev == event_dev) &&
> -			    (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);
> +			if (iucv->hs_dev == event_dev &&
> +			    sk->sk_state == IUCV_CONNECTED)
> +				count++;
> +		}
> +		read_unlock(&iucv_sk_list.lock);
> +
> +		if (!count)
> +			break;
> +		matches = kmalloc_array(count, sizeof(*matches), GFP_KERNEL);
> +		if (!matches)
> +			break;

[Severity: Medium]
If this allocation fails, does every socket bound to event_dev stay in
IUCV_CONNECTED?

The break skips the FIN, the move to IUCV_DISCONN and sk_state_change()
for all matching sockets. __dev_close_many() ignores the notifier's return
value and carries on with the shutdown. NETDEV_DOWN and NETDEV_UNREGISTER
do nothing in this notifier, so nothing retries later.

iucv_sock_poll() only sets EPOLLIN once sk_state is IUCV_DISCONN. Readers
waiting for that would stay blocked.

The previous loop did no allocation, so it could not fail this way.

> +
> +		read_lock(&iucv_sk_list.lock);
> +		sk_for_each(sk, &iucv_sk_list.head) {
> +			if (n == count)
> +				break;

[Severity: Medium]
Can this cap skip sockets that were counted in the first pass?

count comes from a separate critical section, and the lock is dropped
around kmalloc_array(). NETDEV_GOING_DOWN is sent before
__dev_close_many() clears __LINK_STATE_START, so RX is still active.

In that window, afiucv_hs_callback_syn() can link a new socket on the same
device:

	niucv->hs_dev = iucv->hs_dev;
	...
	nsk->sk_state = IUCV_CONNECTED;

Also, afiucv_hs_callback_synack() can move an existing BOUND socket to
IUCV_CONNECTED.

iucv_sock_link() uses sk_add_node(), which inserts at the head, so the new
sockets fill matches[] first. Once n reaches count, the walk stops before
it reaches older sockets that are still IUCV_CONNECTED on event_dev. Those
sockets would get no FIN and no move to IUCV_DISCONN.

> +			iucv = iucv_sk(sk);
> +			if (iucv->hs_dev == event_dev &&
> +			    sk->sk_state == IUCV_CONNECTED) {
> +				sock_hold(sk);
> +				matches[n++] = sk;
>  			}
>  		}
> +		read_unlock(&iucv_sk_list.lock);
> +
> +		for (i = 0; i < n; i++) {
> +			sk = matches[i];
> +			if (event == NETDEV_GOING_DOWN)
> +				iucv_send_ctrl(sk, AF_IUCV_FLAG_FIN);
> +			sk->sk_state = IUCV_DISCONN;
> +			sk->sk_state_change(sk);
> +			sock_put(sk);
> +		}

[Severity: High]
Can a concurrent close() set hs_dev to NULL before this loop runs?

sock_hold() keeps the struct sock allocated, but this loop does not take
lock_sock(). It also does not recheck hs_dev or sk_state after the list
lock is dropped.

close() goes through iucv_sock_release()->iucv_sock_close(). That path
holds only lock_sock(), not RTNL, and ends with:

net/iucv/af_iucv.c:iucv_sock_close() {
    ...
	if (iucv->hs_dev) {
		dev_put(iucv->hs_dev);
		iucv->hs_dev = NULL;
		sk->sk_bound_dev_if = 0;
	}
    ...
}

If that runs between the second pass and this loop, iucv_send_ctrl() then
computes:

	blen = sizeof(struct af_iucv_trans_hdr) +
	       LL_RESERVED_SPACE(iucv->hs_dev);

That reads through a NULL hs_dev before afiucv_hs_send() gets to check
skb->dev.

The loop also writes sk->sk_state = IUCV_DISCONN on a socket that
iucv_sock_close() has already moved to IUCV_CLOSED, which moves its state
backwards. iucv_send_ctrl() also changes sk->sk_shutdown without the
socket lock.

The old code had the same check-then-use race, but the check and the use
were back to back. Here the check happens during collection, and the use
only comes after the whole list has been walked.

For comparison, afiucv_hs_callback_fin() does this transition under the
socket lock:

	bh_lock_sock(sk);
	if (sk->sk_state == IUCV_CONNECTED) {
		sk->sk_state = IUCV_DISCONN;

The commit message says:

  sock_hold() pins each sk for the out-of-lock work, so a concurrent
  close can no longer free it from under the notifier.

Given the above, does that claim hold for the fields used here? Would it
make sense to take lock_sock(sk) in this loop and recheck
sk->sk_state == IUCV_CONNECTED and iucv->hs_dev == event_dev before
acting?

> +		kfree(matches);
>  		break;
> +	}

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/179241612354.5916.12881323209252769976%40whr