[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

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

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?