[PATCH net 0/2] net/iucv: fix the recvmsg window update

[PATCH net 0/2] net/iucv: fix the recvmsg window update

[Bryam Vargas via B4 Relay]

Two independent defects at the same call site, found while accounting for the
socket-locking rework and kept apart because they fail differently.

The first is a missing transport test: recvmsg sends AF_IUCV_FLAG_WIN, which
only exists on HiperSockets, without checking that the socket is on that
transport. Reaching it takes an application that sets SO_MSGLIMIT to 1 on its
own socket, so it is a correctness fix rather than a security one, and it is
first only because the second patch reads better on top of it.

The second is a sleeping allocation under spin_lock_bh: iucv_send_ctrl()
allocates with sk->sk_allocation, which is GFP_KERNEL for these sockets, inside
a section whose other allocation uses GFP_ATOMIC for exactly that reason.

By inspection; not reproduced. Compile-tested for s390x. Both are tagged
for stable: the second's context comes from the first, so tagging only
the second would hand the stable team a patch that does not apply.

---
Bryam Vargas (2):
      net/iucv: only send the window update on HiperSockets sockets
      net/iucv: send the window update outside message_q.lock

 net/iucv/af_iucv.c | 20 ++++++++++++--------
 1 file changed, 12 insertions(+), 8 deletions(-)
---
base-commit: a59f57e2aa127c5354168d2ec4bac920df1be4f4
change-id: 20260815-b4-disp-8a791503-027a9a4c2617

Best regards,
--  
Bryam Vargas <hexlabsecurity@proton.me>

[PATCH net 1/2] net/iucv: only send the window update on HiperSockets sockets

[Bryam Vargas via B4 Relay]

From: Bryam Vargas <hexlabsecurity@proton.me>

iucv_sock_recvmsg() sends AF_IUCV_FLAG_WIN without testing the transport,
but that flag exists only on HiperSockets. On a classic z/VM socket
iucv->hs_dev is NULL and iucv_send_ctrl() sizes the skb from
LL_RESERVED_SPACE(iucv->hs_dev), so the read goes through NULL;
afiucv_hs_send() then returns -ENODEV and recvmsg() moves the socket to
IUCV_DISCONN. Reaching it takes an application that sets SO_MSGLIMIT to 1
on its own socket, since msg_recv is incremented only on the HiperSockets
path, so this is a correctness fix rather than a security one.

Test the transport. Three of the other four iucv_send_ctrl() callers do;
the fourth, afiucv_netdev_event(), selects on hs_dev instead. Without
relocate_lowcore the NULL read lands in mapped lowcore and the symptom is
the spurious disconnect; with lowcore relocation it faults.

Fixes: 3881ac441f64 ("af_iucv: add HiperSockets transport")
Cc: stable@vger.kernel.org
Signed-off-by: Bryam Vargas <hexlabsecurity@proton.me>
---
 net/iucv/af_iucv.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/net/iucv/af_iucv.c b/net/iucv/af_iucv.c
index ea047bab65e7..0bc4a15f4b56 100644
--- a/net/iucv/af_iucv.c
+++ b/net/iucv/af_iucv.c
@@ -1331,7 +1331,8 @@ static int iucv_sock_recvmsg(struct socket *sock, struct msghdr *msg,
 		if (skb_queue_empty(&iucv->backlog_skb_q)) {
 			if (!list_empty(&iucv->message_q.list))
 				iucv_process_message_q(sk);
-			if (atomic_read(&iucv->msg_recv) >=
+			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) {

-- 
2.55.0

Re: [PATCH net 1/2] net/iucv: only send the window update on HiperSockets sockets

[sashiko-bot]

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

Pre-existing issues:
- [High] Calling a sleeping memory allocation function (`sock_alloc_send_skb`) while holding a spinlock (`spin_lock_bh`) causes a sleep-in-atomic bug.
- [High] Concurrent `recvmsg` calls can trigger a Use-After-Free or NULL pointer dereference of the network device.
- [High] Iterating over the global socket list `iucv_sk_list` without holding the required read lock leads to list corruption and data races.
--

--- Patch [1]: [PATCH net 1/2] net/iucv: only send the window update on HiperSockets sockets ---
commit 8751dd7a500553e40dc5fbd7b9476a646ff45fb5
Author: Bryam Vargas <hexlabsecurity@proton.me>

net/iucv: only send the window update on HiperSockets sockets

> iucv_sock_recvmsg() sends AF_IUCV_FLAG_WIN without testing the transport,
> but that flag exists only on HiperSockets. On a classic z/VM socket
> iucv->hs_dev is NULL and iucv_send_ctrl() sizes the skb from
> LL_RESERVED_SPACE(iucv->hs_dev), so the read goes through NULL;
> afiucv_hs_send() then returns -ENODEV and recvmsg() moves the socket to
> IUCV_DISCONN. Reaching it takes an application that sets SO_MSGLIMIT to 1
> on its own socket, since msg_recv is incremented only on the HiperSockets
> path, so this is a correctness fix rather than a security one.
> 
> Test the transport. Three of the other four iucv_send_ctrl() callers do;
> the fourth, afiucv_netdev_event(), selects on hs_dev instead. Without
> relocate_lowcore the NULL read lands in mapped lowcore and the symptom is
> the spurious disconnect; with lowcore relocation it faults.

[Severity: High]
This is a pre-existing issue, but does iterating over the global socket list
iucv_sk_list locklessly in afiucv_netdev_event() lead to list corruption and
data races?

afiucv_netdev_event() traverses the iucv_sk_list.head list using sk_for_each()
while holding only the RTNL lock:

net/iucv/af_iucv.c:afiucv_netdev_event() {
	...
	sk_for_each(sk, &iucv_sk_list.head) {
		iucv = iucv_sk(sk);
		if ((iucv->hs_dev == event_dev) &&
	...
}

However, sockets can be created (via iucv_sock_bind() -> iucv_sock_link()) or
destroyed (via iucv_sock_kill() -> iucv_sock_unlink()) concurrently. These
modify the list while holding write_lock_bh(&iucv_sk_list.lock), but NOT the
RTNL lock.

Iterating a mutated hlist locklessly without acquiring
read_lock(&iucv_sk_list.lock) or rcu_read_lock() could violate locking
invariants and lead to list corruption, infinite loops, or a Use-After-Free.

> 
> Fixes: 3881ac441f64 ("af_iucv: add HiperSockets transport")
> Cc: stable@vger.kernel.org
> Signed-off-by: Bryam Vargas <hexlabsecurity@proton.me>
> 
> diff --git a/net/iucv/af_iucv.c b/net/iucv/af_iucv.c
> index ea047bab65e7c..0bc4a15f4b565 100644
> --- a/net/iucv/af_iucv.c
> +++ b/net/iucv/af_iucv.c
> @@ -1331,7 +1331,8 @@ static int iucv_sock_recvmsg(struct socket *sock, struct msghdr *msg,
>  		if (skb_queue_empty(&iucv->backlog_skb_q)) {
>  			if (!list_empty(&iucv->message_q.list))
>  				iucv_process_message_q(sk);
> -			if (atomic_read(&iucv->msg_recv) >=
> +			if (iucv->transport == AF_IUCV_TRANS_HIPER &&
> +			    atomic_read(&iucv->msg_recv) >=
>  							iucv->msglimit / 2) {
>  				err = iucv_send_ctrl(sk, AF_IUCV_FLAG_WIN);

[Severity: High]
This isn't a bug introduced by this patch, but doesn't calling
iucv_send_ctrl() here cause a sleep-in-atomic bug?

Since iucv_sock_recvmsg() is holding spin_lock_bh(&iucv->message_q.lock):

net/iucv/af_iucv.c:iucv_sock_recvmsg() {
	...
	spin_lock_bh(&iucv->message_q.lock);
	...
	if (iucv->transport == AF_IUCV_TRANS_HIPER && ...) {
		err = iucv_send_ctrl(sk, AF_IUCV_FLAG_WIN);
	...
}

And iucv_send_ctrl() calls sock_alloc_send_skb():

net/iucv/af_iucv.c:iucv_send_ctrl() {
	...
	skb = sock_alloc_send_skb(sk, blen, 1, &err);
	...
}

Which can sleep because sk->sk_allocation defaults to GFP_KERNEL (retaining
__GFP_DIRECT_RECLAIM).

Could this trigger a kernel panic from might_sleep() or a soft lockup?


[Severity: High]
This is also a pre-existing issue, but could concurrent recvmsg calls trigger
a Use-After-Free or NULL pointer dereference of the network device?

sys_recvmsg() can be called concurrently by multiple threads on the same
socket since iucv_sock_recvmsg() does not hold the socket lock.

If Thread A reads enough messages to hit
atomic_read(&iucv->msg_recv) > iucv->msglimit, it calls iucv_sock_close(sk).
This acquires the socket lock, drops the reference to iucv->hs_dev, and sets
it to NULL:

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

Concurrently, if Thread B hits the check below and calls iucv_send_ctrl():

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

It reads and dereferences iucv->hs_dev locklessly, which could result in a UAF
or NULL pointer dereference.

>  				if (err) {

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260815-b4-disp-8a791503-v1-0-fbae9a511144@proton.me?part=1

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

[Bryam Vargas via B4 Relay]

From: Bryam Vargas <hexlabsecurity@proton.me>

iucv_sock_recvmsg() calls iucv_send_ctrl() with message_q.lock held, and
iucv_send_ctrl() allocates through sock_alloc_send_skb() with
sk->sk_allocation -- GFP_KERNEL here -- so the allocation may sleep inside
the spin_lock_bh() section; noblock suppresses only the wait for send
buffer space, not the allocation flags. The section's other allocation,
alloc_iucv_recv_skb() under iucv_process_message_q(), uses GFP_ATOMIC.

Note that the update is due and send it after the lock is dropped. The
lock protects backlog_skb_q and message_q.list, neither of which the send
touches. It does widen an existing msg_recv race -- afiucv_hs_send() reads
the counter and subtracts it later, and recvmsg holds no socket lock, so
two recvmsg can now interleave where before only recvmsg and sendmsg
could. Both trip the same WARN_ON.

Fixes: 3881ac441f64 ("af_iucv: add HiperSockets transport")
Cc: stable@vger.kernel.org
Signed-off-by: Bryam Vargas <hexlabsecurity@proton.me>
---
 net/iucv/af_iucv.c | 19 +++++++++++--------
 1 file changed, 11 insertions(+), 8 deletions(-)

diff --git a/net/iucv/af_iucv.c b/net/iucv/af_iucv.c
index 0bc4a15f4b56..e31ef3a87ec5 100644
--- a/net/iucv/af_iucv.c
+++ b/net/iucv/af_iucv.c
@@ -1241,6 +1241,7 @@ static int iucv_sock_recvmsg(struct socket *sock, struct msghdr *msg,
 	struct iucv_sock *iucv = iucv_sk(sk);
 	unsigned int copied, rlen;
 	struct sk_buff *skb, *rskb, *cskb;
+	bool send_win = false;
 	int err = 0;
 	u32 offset;
 
@@ -1332,16 +1333,18 @@ static int iucv_sock_recvmsg(struct socket *sock, struct msghdr *msg,
 			if (!list_empty(&iucv->message_q.list))
 				iucv_process_message_q(sk);
 			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);
-				}
-			}
+			    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);
+			if (err) {
+				sk->sk_state = IUCV_DISCONN;
+				sk->sk_state_change(sk);
+			}
+		}
 	}
 
 done:

-- 
2.55.0

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

[sashiko-bot]

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 <hexlabsecurity@proton.me>

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/20260815-b4-disp-8a791503-v1-0-fbae9a511144@proton.me?part=2