Re: [PATCH net 2/2] net/iucv: send the window update outside message_q.lock

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

New issues:
- [High] Moving iucv_send_ctrl() outside of message_q.lock introduces a severe TOCTOU race between concurrent recvmsg calls, allowing unprivileged userspace to bypass flow control and trigger a WARN_ON DoS.

Pre-existing issues:
- [High] Executing iucv_send_ctrl() and modifying sk_state outside of lock protection causes spurious -EPIPE connection drops and corrupted socket states.
- [High] Concurrent execution of iucv_sock_close() and iucv_send_ctrl() leads to a NULL pointer dereference on iucv->hs_dev.
- [High] Unbounded backlog_skb_q allows a malicious peer to exhaust host memory (OOM).
- [High] Missing lock acquisition in afiucv_netdev_event() allows list corruption and kernel crashes.
--

--- Patch [2]: [PATCH net 2/2] net/iucv: send the window update outside message_q.lock ---
commit 77dbc2e0a9dfbb0a0cd09842d89b0c9e75001a8e
Author: Bryam Vargas <[email protected]>

net/iucv: send the window update outside message_q.lock

This commit moves iucv_send_ctrl() outside the message_q.lock in
iucv_sock_recvmsg() to avoid sleeping in an atomic context. The allocation
in iucv_send_ctrl() uses GFP_KERNEL which can sleep, whereas the lock
section is protected by spin_lock_bh.

> diff --git a/net/iucv/af_iucv.c b/net/iucv/af_iucv.c
> index 0bc4a15f4b565..e31ef3a87ec50 100644
> --- a/net/iucv/af_iucv.c
> +++ b/net/iucv/af_iucv.c
> @@ -1332,16 +1333,18 @@ static int iucv_sock_recvmsg(struct socket *sock, struct msghdr *msg,

[ ... ]

> -			if (iucv->transport == AF_IUCV_TRANS_HIPER &&
> -			    atomic_read(&iucv->msg_recv) >=
> -							iucv->msglimit / 2) {
> -				err = iucv_send_ctrl(sk, AF_IUCV_FLAG_WIN);
> -				if (err) {
> -					sk->sk_state = IUCV_DISCONN;
> -					sk->sk_state_change(sk);
> -				}
> -			}
> +			if (iucv->transport == AF_IUCV_TRANS_HIPER &&
> +			    atomic_read(&iucv->msg_recv) >= iucv->msglimit / 2)
> +				send_win = true;
>  		}
>  		spin_unlock_bh(&iucv->message_q.lock);
> +
> +		if (send_win) {
> +			err = iucv_send_ctrl(sk, AF_IUCV_FLAG_WIN);

[Severity: High]
Does moving this outside the lock introduce a race condition between
concurrent iucv_sock_recvmsg() calls?

Since send_win is evaluated under the lock but executed after dropping it,
could two threads concurrently evaluate send_win to true and both call
iucv_send_ctrl()?

Looking at afiucv_hs_send(), it locklessly reads and subtracts msg_recv:

confirm_recv = atomic_read(&iucv->msg_recv);
...
atomic_sub(confirm_recv, &iucv->msg_recv);
WARN_ON(atomic_read(&iucv->msg_recv) < 0);

If two threads execute this concurrently, could they read the exact same
value, send multiple window updates to the peer, and subtract the same
value multiple times? It appears this would drive msg_recv negative and
trigger the WARN_ON, allowing unprivileged userspace to bypass flow control.

[Severity: High]
This isn't a bug introduced by this patch, but does calling iucv_send_ctrl()
without the socket lock expose a race with device teardown?

Looking at iucv_sock_close(), it nullifies hs_dev concurrently:

if (iucv->hs_dev) {
        dev_put(iucv->hs_dev);
        iucv->hs_dev = NULL;

Inside iucv_send_ctrl(), it accesses hs_dev without checking for NULL:

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

Could a concurrent close() racing with iucv_sock_recvmsg() lead to a NULL
pointer dereference here?

> +			if (err) {
> +				sk->sk_state = IUCV_DISCONN;
> +				sk->sk_state_change(sk);
> +			}
> +		}
>  	}

[Severity: High]
This is a pre-existing issue, but does modifying sk_state without holding
lock_sock() corrupt the socket state machine?

If iucv_send_ctrl() fails, we locklessly set sk->sk_state = IUCV_DISCONN.
Could this corrupt state transitions if racing with a concurrent close()
call?

Additionally, iucv_send_ctrl() performs a non-atomic read-modify-write on
sk_shutdown:

shutdown = sk->sk_shutdown;
sk->sk_shutdown &= RCV_SHUTDOWN;
...
if (shutdown)
        sk->sk_shutdown = shutdown;

Because it is called from iucv_sock_recvmsg() without lock_sock(), could a
concurrent shutdown() call that sets RCV_SHUTDOWN have its update
permanently overwritten and lost when the old state is restored?

[Severity: High]
This is a pre-existing issue, but does the queueing in afiucv_hs_callback_rx()
lack bounds checking?

In afiucv_hs_callback_rx():

if (__sock_queue_rcv_skb(sk, skb))
        /* handle rcv queue full */
        skb_queue_tail(&iucv->backlog_skb_q, skb);

If __sock_queue_rcv_skb() rejects the packet because the receive buffer is
full, it is unconditionally appended to backlog_skb_q. Without length
limits or memory accounting on this queue, could a remote peer exhaust
host memory by sending an unlimited stream of packets?

[Severity: High]
This isn't a bug introduced by this patch, but does afiucv_netdev_event()
iterate the socket list safely?

In afiucv_netdev_event():

sk_for_each(sk, &iucv_sk_list.head) {
        iucv = iucv_sk(sk);

This iterates over the global iucv_sk_list.head, but it fails to acquire
the required lock before iterating. Could concurrent calls to
iucv_sock_bind() or iucv_sock_kill() modifying the list trigger corruption?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=2