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

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

[Bryam Vargas via B4 Relay]

v2 answers Jakub's reviews of v1 with changes rather than replies.

1/3 keeps its hunk and states the impact properly. SO_MSGLIMIT accepts 1,
which makes msglimit / 2 zero, and msg_recv never leaves zero on a classic
socket, so the NULL read is one recvmsg() away for an unprivileged process
on a socket of its own. With relocate_lowcore it faults.

Its Fixes: tag was wrong as well. The unconditional send does date to
3881ac441f64, but iucv_send_ctrl() sized the skb with a constant ETH_HLEN
until 238965b71b96, so before that it just returned -ENODEV. Backport
window is v5.3, not v3.1.

2/3 is new. v1 admitted it widened the msg_recv race and left it open,
which isn't good enough under Cc: stable. afiucv_hs_send() samples the
counter and settles it after dev_queue_xmit(); sendmsg reaches that under
lock_sock() and recvmsg reaches it under no socket lock, so the two don't
exclude each other and both can subtract the same value. Negative counter,
WARN_ON, and the same credit advertised twice to a peer whose
afiucv_hs_callback_win() subtracts the wire value from msg_sent unchecked.

Taking the socket lock around the deferred send would close it too, since
recvmsg is the only unlocked side. I didn't: recvmsg has never held that
lock, and making it do so changes the receive path for every caller. That
belongs in the locking rework, not in a stable fix.

The claim sits after the last error exit rather than in the header build,
so the counter reads zero only while the transmit is in flight, and a
concurrent sender isn't talked out of its own update.

3/3 is v1's patch, and it must not be applied without 2/3: hoisting the
send out of message_q.lock drops the serialisation that lock gave two
concurrent recvmsg(). Backported alone it recreates what 2/3 fixes. Both
changelogs say so.

Litmus test under LKMM: the counter reaches -2 before 2/3 and cannot after.
No hardware run; I have no IBM Z, and CONFIG_AFIUCV is s390-only.

The unlocked hs_dev and sk_shutdown window, the unbounded backlog_skb_q and
the WARN_ON a flooding peer can reach are pre-existing and wider than this
series. They belong with the locking rework Alexandra has open.

Thanks for the reviews.

---
Bryam Vargas (3):
      net/iucv: only send the window update on HiperSockets sockets
      net/iucv: claim the receive credit atomically
      net/iucv: send the window update outside message_q.lock

 net/iucv/af_iucv.c | 42 +++++++++++++++++++++++++-----------------
 1 file changed, 25 insertions(+), 17 deletions(-)
---
base-commit: 746fc0787f616da418ffc04a110296fe95d53491
change-id: 20260821-b4-disp-3a6e8695-c1f4a6069169

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

[PATCH net v2 1/3] 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. It is one
recvmsg() away for an unprivileged process on a socket of its own:
SO_MSGLIMIT accepts 1, so msglimit / 2 is 0 and msg_recv never leaves 0 on
a classic socket. The read lands in mapped lowcore on a default kernel and
the socket takes a spurious disconnect; with relocate_lowcore it faults.

Test the transport. The unconditional send is older than that, but stayed
harmless while iucv_send_ctrl() used a constant ETH_HLEN.

Fixes: 238965b71b96 ("net/af_iucv: build proper skbs for HiperTransport")
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 v2 2/3] net/iucv: claim the receive credit atomically

[Bryam Vargas via B4 Relay]

From: Bryam Vargas <hexlabsecurity@proton.me>

afiucv_hs_send() samples msg_recv, advertises it to the peer as the window,
and subtracts it once dev_queue_xmit() has returned. Nothing owns the
counter across the two: iucv_sock_sendmsg() reaches it under lock_sock()
and iucv_sock_recvmsg() reaches it under no socket lock, so an unprivileged
process running both on one socket can have them subtract the same value.
msg_recv goes negative and trips the WARN_ON(); the same interleaving puts
that credit on the wire twice, and the peer's afiucv_hs_callback_win()
subtracts the wire value from msg_sent unchecked.

Claim it with atomic_xchg(), after the last error exit so the counter reads
zero only while the transmit is in flight, and hand it back if that fails.
Nothing subtracts now, so the WARN_ON() goes too.

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 | 21 ++++++++++++---------
 1 file changed, 12 insertions(+), 9 deletions(-)

diff --git a/net/iucv/af_iucv.c b/net/iucv/af_iucv.c
index 0bc4a15f4b56..492a45bb2bba 100644
--- a/net/iucv/af_iucv.c
+++ b/net/iucv/af_iucv.c
@@ -210,12 +210,6 @@ static int afiucv_hs_send(struct iucv_message *imsg, struct sock *sock,
 	phs_hdr->flags = flags;
 	if (flags == AF_IUCV_FLAG_SYN)
 		phs_hdr->window = iucv->msglimit;
-	else if ((flags == AF_IUCV_FLAG_WIN) || !flags) {
-		confirm_recv = atomic_read(&iucv->msg_recv);
-		phs_hdr->window = confirm_recv;
-		if (confirm_recv)
-			phs_hdr->flags = phs_hdr->flags | AF_IUCV_FLAG_WIN;
-	}
 	memcpy(phs_hdr->destUserID, iucv->dst_user_id, 8);
 	memcpy(phs_hdr->destAppName, iucv->dst_name, 8);
 	memcpy(phs_hdr->srcUserID, iucv->src_user_id, 8);
@@ -250,13 +244,22 @@ static int afiucv_hs_send(struct iucv_message *imsg, struct sock *sock,
 	}
 	skb->protocol = cpu_to_be16(ETH_P_AF_IUCV);
 
+	/* Claim the receive credit here, not while building the header: every
+	 * way this frame can be dropped has now been ruled out, so the window
+	 * is zeroed only for as long as the transmit itself takes.
+	 */
+	if (flags == AF_IUCV_FLAG_WIN || !flags) {
+		confirm_recv = atomic_xchg(&iucv->msg_recv, 0);
+		phs_hdr->window = confirm_recv;
+		if (confirm_recv)
+			phs_hdr->flags = phs_hdr->flags | AF_IUCV_FLAG_WIN;
+	}
+
 	atomic_inc(&iucv->skbs_in_xmit);
 	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);
+		atomic_add(confirm_recv, &iucv->msg_recv);
 	}
 	return net_xmit_eval(err);
 

-- 
2.55.0

[PATCH net v2 3/3] 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. CONFIG_DEBUG_ATOMIC_SLEEP reports
it.

Note that the update is due and send it once the lock is dropped. That
leaves two recvmsg() able to reach afiucv_hs_send() concurrently, which
message_q.lock used to prevent; the preceding patch is what makes that
safe, so do not apply this one without it.

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 | 18 +++++++++++-------
 1 file changed, 11 insertions(+), 7 deletions(-)

diff --git a/net/iucv/af_iucv.c b/net/iucv/af_iucv.c
index 492a45bb2bba..a7c0f60bb5bf 100644
--- a/net/iucv/af_iucv.c
+++ b/net/iucv/af_iucv.c
@@ -1244,6 +1244,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;
 
@@ -1336,15 +1337,18 @@ static int iucv_sock_recvmsg(struct socket *sock, struct msghdr *msg,
 				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);
-				}
-			}
+							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 v2 0/3] net/iucv: fix the recvmsg window update

[Alexandra Winter]

On 21.08.26 13:17, Bryam Vargas via B4 Relay wrote:
> v2 answers Jakub's reviews of v1 with changes rather than replies.
> 
> 1/3 keeps its hunk and states the impact properly. SO_MSGLIMIT accepts 1,
> which makes msglimit / 2 zero, and msg_recv never leaves zero on a classic
> socket, so the NULL read is one recvmsg() away for an unprivileged process
> on a socket of its own. With relocate_lowcore it faults.
> 
> Its Fixes: tag was wrong as well. The unconditional send does date to
> 3881ac441f64, but iucv_send_ctrl() sized the skb with a constant ETH_HLEN
> until 238965b71b96, so before that it just returned -ENODEV. Backport
> window is v5.3, not v3.1.
> 
> 2/3 is new. v1 admitted it widened the msg_recv race and left it open,
> which isn't good enough under Cc: stable. afiucv_hs_send() samples the
> counter and settles it after dev_queue_xmit(); sendmsg reaches that under
> lock_sock() and recvmsg reaches it under no socket lock, so the two don't
> exclude each other and both can subtract the same value. Negative counter,
> WARN_ON, and the same credit advertised twice to a peer whose
> afiucv_hs_callback_win() subtracts the wire value from msg_sent unchecked.
> 
> Taking the socket lock around the deferred send would close it too, since
> recvmsg is the only unlocked side. I didn't: recvmsg has never held that
> lock, and making it do so changes the receive path for every caller. That
> belongs in the locking rework, not in a stable fix.
> 
> The claim sits after the last error exit rather than in the header build,
> so the counter reads zero only while the transmit is in flight, and a
> concurrent sender isn't talked out of its own update.
> 
> 3/3 is v1's patch, and it must not be applied without 2/3: hoisting the
> send out of message_q.lock drops the serialisation that lock gave two
> concurrent recvmsg(). Backported alone it recreates what 2/3 fixes. Both
> changelogs say so.
> 
> Litmus test under LKMM: the counter reaches -2 before 2/3 and cannot after.
> No hardware run; I have no IBM Z, and CONFIG_AFIUCV is s390-only.
> 
> The unlocked hs_dev and sk_shutdown window, the unbounded backlog_skb_q and
> the WARN_ON a flooding peer can reach are pre-existing and wider than this
> series. They belong with the locking rework Alexandra has open.
> 
> Thanks for the reviews.
> 
> ---
> Bryam Vargas (3):
>       net/iucv: only send the window update on HiperSockets sockets
>       net/iucv: claim the receive credit atomically
>       net/iucv: send the window update outside message_q.lock
> 
>  net/iucv/af_iucv.c | 42 +++++++++++++++++++++++++-----------------
>  1 file changed, 25 insertions(+), 17 deletions(-)
> ---
> base-commit: 746fc0787f616da418ffc04a110296fe95d53491
> change-id: 20260821-b4-disp-3a6e8695-c1f4a6069169
> 
> Best regards,
> --  
> Bryam Vargas <hexlabsecurity@proton.me>
> 
> 
> 

Thank you for the fixes Bryam.
I have reviewed them and would technically give an R-b to all three.

However I propose to send them again as one single patch. They are
really about the same subject: iucv->msg_recv handling and even have
a dependency, as you rightfully noted.
It will be much easier to proceed and to backport them, if it is only
one patch.

I know it's a tradeoff, but I think we will do ourselves and the
upstream maintainers a favor, if we send not too small fragments
of fixes.