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