[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

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

[Jakub Kicinski]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
net/iucv: only send the window update on HiperSockets sockets

This adds a transport test before iucv_sock_recvmsg() sends
AF_IUCV_FLAG_WIN, so that iucv_send_ctrl() is no longer called on classic
z/VM sockets where iucv->hs_dev is NULL.  Two questions below, one about
the impact statement in the changelog and one about the surrounding
msg_recv accounting.

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

[Severity: Medium]
The bug being fixed here is pre-existing (3881ac441f64) and is not
introduced by this patch, but is the concluding sentence, "so this is a
correctness fix rather than a security one", accurate for a patch carrying
Cc: stable?

The trigger is entirely under the control of an unprivileged local process
using the ordinary socket API.  SO_MSGLIMIT accepts 1 in iucv_setsockopt():

	if (val < 1 || val > U16_MAX)
		rc = -EINVAL;
	else
		iucv->msglimit = val;

msg_recv stays at 0 on a classic socket, so msglimit / 2 == 0 makes the
old predicate 0 >= 0 on every successful non-PEEK recvmsg(), and
iucv_send_ctrl() then evaluates:

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

with hs_dev == NULL, before afiucv_hs_send() gets a chance to return
-ENODEV on its !skb->dev test.

The changelog itself then says "with lowcore relocation it faults", and
relocate_lowcore is a supported s390 boot parameter.  On such kernels an
ordinary recvmsg() from an unprivileged process oopses, which reads like a
local denial of service rather than a correctness issue.  Would it be worth
restating the impact so stable maintainers can prioritise and tag it
accordingly?

> 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);
>  				if (err) {

[Severity: High]
This is a pre-existing issue and is not introduced by this patch, but since
the patch touches this exact accounting block, can the WARN a few lines
above be reached from remote input on the HiperSockets path?

Just before the hunk, iucv_sock_recvmsg() does:

		consume_skb(skb);
		if (iucv->transport == AF_IUCV_TRANS_HIPER) {
			atomic_inc(&iucv->msg_recv);
			if (atomic_read(&iucv->msg_recv) > iucv->msglimit) {
				WARN_ON(1);
				iucv_sock_close(sk);
				return -EFAULT;
			}
		}

The only place msg_recv is decremented is afiucv_hs_send() with
atomic_sub(confirm_recv, &iucv->msg_recv), reached through the
iucv_send_ctrl(sk, AF_IUCV_FLAG_WIN) call in this hunk, and that call sits
under:

		if (skb_queue_empty(&iucv->backlog_skb_q)) {

Meanwhile afiucv_hs_callback_rx() queues every accepted frame without
enforcing the window it advertised to the peer:

net/iucv/af_iucv.c:afiucv_hs_callback_rx() {
	spin_lock(&iucv->message_q.lock);
	if (skb_queue_empty(&iucv->backlog_skb_q)) {
		if (__sock_queue_rcv_skb(sk, skb))
			skb_queue_tail(&iucv->backlog_skb_q, skb);
	} else
		skb_queue_tail(&iucv_sk(sk)->backlog_skb_q, skb);

So if a non-conforming peer floods the socket, the receive queue hits
sk_rcvbuf, __sock_queue_rcv_skb() keeps failing in the drain loop above,
the skb is re-queued at the head of backlog_skb_q and the loop breaks.
backlog_skb_q then stays non-empty, no window update is sent, and msg_recv
keeps incrementing on each recvmsg() until it passes msglimit
(IUCV_HIPER_MSGLIM_DEFAULT is 128) and the WARN fires.  With panic_on_warn
that would be a panic driven by a remote peer.

The same WARN also looks reachable locally with SO_MSGLIMIT set to 1 and
two concurrent recvmsg() threads, since recvmsg() does not hold the socket
lock across the atomic_inc.

Would a rate-limited message plus a drop or reset be a better response to a
peer protocol violation here than WARN_ON(1)?
-- 
pw-bot: cr

[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

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

[Jakub Kicinski]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
net/iucv: send the window update outside message_q.lock

This moves the AF_IUCV_FLAG_WIN control send in iucv_sock_recvmsg() out of
the spin_lock_bh(&iucv->message_q.lock) section by latching a send_win
boolean under the lock and doing the send after the unlock.  The stated
reason is that iucv_send_ctrl() allocates with sock_alloc_send_skb() and
sk->sk_allocation (GFP_KERNEL), which may sleep inside the spinlock.

> 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
> @@ -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);
> +			}
> +		}
>  	}

[Severity: High]
The changelog notes the widened msg_recv race, but should the patch also
close it rather than leave it, given the Cc: stable tag?

iucv_sock_recvmsg() never takes lock_sock(), so message_q.lock was the only
thing serializing two recvmsg threads through the read-then-subtract of
iucv->msg_recv in afiucv_hs_send():

net/iucv/af_iucv.c:afiucv_hs_send() {
	...
	else if ((flags == AF_IUCV_FLAG_WIN) || !flags) {
		confirm_recv = atomic_read(&iucv->msg_recv);
		phs_hdr->window = confirm_recv;
	...
	err = dev_queue_xmit(skb);
	if (net_xmit_eval(err)) {
		atomic_dec(&iucv->skbs_in_xmit);
	} else {
		atomic_sub(confirm_recv, &iucv->msg_recv);
		WARN_ON(atomic_read(&iucv->msg_recv) < 0);
	}
}

Can two threads doing recvmsg() on the same HiperSockets socket both latch
send_win under the lock, both sample the same value N after the unlock, both
advertise N to the peer, and both subtract N, leaving msg_recv at -N and
tripping that WARN_ON() from unprivileged userspace?  iucv_sock_create()
requires no capability, and panic_on_warn turns the splat into a panic.

Does the negative counter then get truncated into the u16 window field of
struct af_iucv_trans_hdr, so that -1 is advertised as 65535?  On the peer
side afiucv_hs_callback_win() subtracts the wire value directly:

net/iucv/af_iucv.c:afiucv_hs_callback_win() {
	...
	atomic_sub(iucv_trans_hdr(skb)->window, &iucv->msg_sent);
}

so its msg_sent goes far negative and iucv_below_msglim() stops throttling
it.  The over-credited peer can then push more than msglimit messages, and
the receiver either takes the teardown path in iucv_sock_recvmsg():

	if (atomic_read(&iucv->msg_recv) > iucv->msglimit) {
		WARN_ON(1);
		iucv_sock_close(sk);
		return -EFAULT;
	}

or accumulates the excess in iucv->backlog_skb_q, which
afiucv_hs_callback_rx() fills with skb_queue_tail() without any length or
rmem accounting, so the growth is not bounded by SO_RCVBUF.

The same failure mode was already reachable before this patch through the
recvmsg-versus-sendmsg interleaving, since iucv_sock_sendmsg() calls
afiucv_hs_send() under lock_sock() only, a lock recvmsg does not take.  This
patch adds a new recvmsg-versus-recvmsg interleaving on top of that.

Would claiming the credit atomically before the transmit work here, for
example atomic_xchg()/atomic_fetch_sub() on msg_recv with a re-add on xmit
failure?  Alternatively, could the control skb be preallocated before
entering the locked section, or the deferred send be done under the socket
lock, instead of widening the window?

[Severity: High]
This isn't a bug introduced by this patch, but the deferred call appears to
lengthen an existing unlocked window around iucv->hs_dev and sk->sk_shutdown.

iucv_send_ctrl() dereferences hs_dev unconditionally and does an
unserialized read-modify-write of sk->sk_shutdown:

net/iucv/af_iucv.c:iucv_send_ctrl() {
	blen = sizeof(struct af_iucv_trans_hdr) +
	       LL_RESERVED_SPACE(iucv->hs_dev);
	if (sk->sk_shutdown & SEND_SHUTDOWN) {
		shutdown = sk->sk_shutdown;
		sk->sk_shutdown &= RCV_SHUTDOWN;
	}
	skb = sock_alloc_send_skb(sk, blen, 1, &err);
	...
	if (shutdown)
		sk->sk_shutdown = shutdown;
}

LL_RESERVED_SPACE() dereferences dev->hard_header_len and
dev->needed_headroom.  The only writer that clears hs_dev holds lock_sock()
only, which recvmsg never takes, and message_q.lock does not cover hs_dev
either (for HIPER sockets iucv->path is NULL, so iucv_sever_path() returns
without touching message_q.lock):

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

Can a second thread doing recvmsg() on the same socket reach the
WARN_ON(1)/iucv_sock_close() branch above and NULL and dev_put() hs_dev
while this thread is inside the deferred iucv_send_ctrl(), giving
LL_RESERVED_SPACE(NULL) or use of a net_device whose reference was just
dropped?

Separately, iucv_sock_shutdown() sets sk->sk_shutdown |= how under
lock_sock().  Can the unlocked save/clear/restore in iucv_send_ctrl()
overwrite that and drop a concurrently set SEND_SHUTDOWN bit, re-enabling
sends after shutdown?

Since the patch now allows the potentially sleeping sock_alloc_send_skb()
(sk->sk_allocation is GFP_KERNEL) to run inside this unlocked window, would
taking the socket lock around the deferred control send, or pinning hs_dev
with a local dev_hold(), be preferable?