[PATCH net 0/2] net/iucv: two fixes in the HiperSockets receive path

[PATCH net 0/2] net/iucv: two fixes in the HiperSockets receive path

[Bryam Vargas via B4 Relay]

Two bugs in the HiperSockets receive path that predate the socket-locking
rework [1]; review and testing of that rework turned them up. Neither
depends on it.

1/2 clears hs_dev before the socket drops its reference on the device and
marks the accesses that race with it: afiucv_hs_rcv() compares hs_dev
under iucv_sk_list.lock while close() clears it under lock_sock().
Hidayath Khan caught the racing pair with KCSAN under z/VM [2] and
suggested the fix. The listener's child now takes its device from the
frame instead of reading the listener's field in softirq. This is an
ordering fix with no known crash.

2/2 stops counting the SHT's empty frame against the receive window (the
peer does not count it) and drops the WARN_ON on the overrun path, which
a peer reaches from the network by sending past its window. Only frames
with data count now; a zero-length data frame, which already ends the
stream for its reader, is no longer counted either. 2/2 applies on its
own and goes to stable.

Based on net f4377bf85887. The same two patches apply on net-next
8df0638138d3, where the test set ran, and on mainline (7.3-rc7); the
SHT case below ran on this base as well. Tested on an s390x guest
under QEMU TCG with KASAN and lockdep on. QEMU has no HiperSockets
adapter and its s390x CPU emulation lacked things this needs: the
adapter is one I wrote, under the real qeth and qdio drivers, the CPU
carries my patches, and there is no IBM Z to compare against. With 2/2
an SHT read while frames wait in backlog_skb_q gives EOF instead of
WARN plus -EFAULT. 1/2 does not change any result in the rework's test
set; its race shows under KCSAN, not here. Logs on request.

[1] https://lore.kernel.org/netdev/20261011-b4-disp-410518f4-v1-1-753bf3258292@proton.me/
[2] https://lore.kernel.org/netdev/b3fea2ca-25ff-42c0-812c-f8cc6bb6667e@linux.ibm.com/

---
Bryam Vargas (2):
      net/iucv: clear hs_dev before dropping its reference
      net/iucv: do not count the SHT frame against the receive window

 net/iucv/af_iucv.c | 39 +++++++++++++++++++++++++++++----------
 1 file changed, 29 insertions(+), 10 deletions(-)
---
base-commit: f4377bf85887f57b59abb5479f7660a81ffe4449
change-id: 20261022-b4-disp-b35a9153-ad04dc76713a

Best regards,
--  
bryamzxz <hexlabsecurity@proton.me>

[PATCH net 1/2] net/iucv: clear hs_dev before dropping its reference

[Bryam Vargas via B4 Relay]

From: Bryam Vargas <hexlabsecurity@proton.me>

iucv_sock_close() drops the socket's reference on its HiperSockets
device and only then clears iucv->hs_dev, while afiucv_hs_rcv() compares
that field against the ingress device for every socket in the list,
under iucv_sk_list.lock and not the socket lock. Between the two it
compares a pointer the socket no longer holds; KCSAN reports the plain
read against the plain write. The order dates from 800c5eb7b5eb, the
compare from 80230a18c164. Nothing user-visible is known from it: the
receive path holds the ingress device, so the stale pointer is compared,
never dereferenced.

Clear the pointer first, through a local, then drop the reference, and
mark the lookup's read and the two stores that race with it: the clear
in close and the listener's child's, which is in iucv_sk_list from
iucv_sock_alloc() on. The child now takes its device and
sk_bound_dev_if from the frame the lookup matched, held before it is
published, instead of reading the listener's fields: close() clears
them under lock_sock(), which the bh_lock_sock() in
afiucv_hs_callback_syn() does not exclude, so the callback can pass its
IUCV_LISTEN check and then read a field close() has cleared. That a SYN
can be accepted while the listener closes is left to the socket-locking
rework; this is its KCSAN-visible part. bind()'s hs_dev store runs
under the list's write lock and stays plain. sk_bound_dev_if, which
getsockopt() reads without the lock, is marked where close(), bind()
and the child store it; annotation only.

Fixes: 800c5eb7b5eb ("af_iucv: change net_device handling for HS transport")
Fixes: 80230a18c164 ("net/iucv: filter frames in afiucv_hs_rcv() by ingress device")
Reported-by: Hidayath Khan <hidayath@linux.ibm.com>
Closes: https://lore.kernel.org/netdev/b3fea2ca-25ff-42c0-812c-f8cc6bb6667e@linux.ibm.com/
Suggested-by: Hidayath Khan <hidayath@linux.ibm.com>
Signed-off-by: Bryam Vargas <hexlabsecurity@proton.me>
---
Nagamani PV's "net/iucv: fix races in afiucv_netdev_event()" [1] marks the
same store in close(); Alexandra Winter asked him to hold it for the
locking series and he agreed [2]. This one goes now on a new, independent
KCSAN report, and its close() hunk subsumes his, which marks the store
but keeps dev_put() first; once this is applied, the
afiucv_netdev_event() read is what remains for his patch.

The loop's memcmp()s on the name fields are plain as well, against
connect()'s stores under lock_sock(); they were not in Hidayath's report
and are not touched here. The child that afiucv_hs_callback_syn() leaks
on its error paths (iucv_sock_kill() on a socket that was never closed
is a no-op), with the device reference it holds when dev_queue_xmit()
fails, is pre-existing; Hidayath flagged it in August [3] and his fix
for it is pending. A child the callback enqueues after
iucv_sock_cleanup_listen() has drained the accept queue is never
accepted or killed either; it now holds the device the frame arrived on,
where before it held the listener's device or nothing, depending on
which side of the clear it read.

Tested on s390x under QEMU TCG (net-next 8df0638138d3, KASAN +
PROVE_LOCKING) with an emulated HiperSockets adapter under the real qeth
and qdio drivers. The rework's test set, 53 tests, gives the same result
with and without this patch: the 8 that fail on net-next are the bugs
the rework fixes, and they fail the same way. That setup has no KCSAN
and nothing that drives afiucv_hs_rcv()'s lookup against a close() at
the right instant. Hidayath's KCSAN report covers the lookup's read and
proposed the clear-before-put order through a local; the child's device
source comes from reading the code.

[1] https://lore.kernel.org/netdev/20260917071706.23831-1-nagamani@linux.ibm.com/
[2] https://lore.kernel.org/netdev/1d9d3cc1-24fb-40e0-a4c7-6fbfbcf4dc37@linux.ibm.com/
[3] https://lore.kernel.org/netdev/5f88daf4-cd8e-4464-b132-ac45be205121@linux.ibm.com/
---
 net/iucv/af_iucv.c | 32 ++++++++++++++++++++++++--------
 1 file changed, 24 insertions(+), 8 deletions(-)

diff --git a/net/iucv/af_iucv.c b/net/iucv/af_iucv.c
index db261ecd19af..501e758c5e68 100644
--- a/net/iucv/af_iucv.c
+++ b/net/iucv/af_iucv.c
@@ -447,9 +447,16 @@ static void iucv_sock_close(struct sock *sk)
 	}
 
 	if (iucv->hs_dev) {
-		dev_put(iucv->hs_dev);
-		iucv->hs_dev = NULL;
-		sk->sk_bound_dev_if = 0;
+		struct net_device *dev = iucv->hs_dev;
+
+		/* afiucv_hs_rcv() matches frames on hs_dev under
+		 * iucv_sk_list.lock, not the socket lock: clear it before the
+		 * reference goes, so the match can no longer find a device
+		 * this socket does not hold.
+		 */
+		WRITE_ONCE(iucv->hs_dev, NULL);
+		WRITE_ONCE(sk->sk_bound_dev_if, 0);
+		dev_put(dev);
 	}
 
 	/* mark socket for deletion by iucv_sock_kill() */
@@ -630,7 +637,7 @@ static int iucv_sock_bind(struct socket *sock, struct sockaddr_unsized *addr,
 				__iucv_auto_name(iucv);
 			else
 				memcpy(iucv->src_name, sa->siucv_name, 8);
-			sk->sk_bound_dev_if = dev->ifindex;
+			WRITE_ONCE(sk->sk_bound_dev_if, dev->ifindex);
 			iucv->hs_dev = dev;
 			dev_hold(dev);
 			sk->sk_state = IUCV_BOUND;
@@ -1912,9 +1919,14 @@ static int afiucv_hs_callback_syn(struct sock *sk, struct sk_buff *skb)
 	memcpy(niucv->dst_user_id, trans_hdr->srcUserID, 8);
 	memcpy(niucv->src_name, iucv->src_name, 8);
 	memcpy(niucv->src_user_id, iucv->src_user_id, 8);
-	nsk->sk_bound_dev_if = sk->sk_bound_dev_if;
-	niucv->hs_dev = iucv->hs_dev;
-	dev_hold(niucv->hs_dev);
+	/* skb->dev is the device the lookup matched against the listener's
+	 * hs_dev; the listener's own fields are cleared by close() under
+	 * lock_sock(), which this softirq does not hold. nsk is in
+	 * iucv_sk_list already: hold, then publish.
+	 */
+	WRITE_ONCE(nsk->sk_bound_dev_if, skb->dev->ifindex);
+	dev_hold(skb->dev);
+	WRITE_ONCE(niucv->hs_dev, skb->dev);
 	afiucv_swap_src_dest(skb);
 	trans_hdr->flags = AF_IUCV_FLAG_SYN | AF_IUCV_FLAG_ACK;
 	trans_hdr->window = niucv->msglimit;
@@ -2087,7 +2099,11 @@ static int afiucv_hs_rcv(struct sk_buff *skb, struct net_device *dev,
 	sk = NULL;
 	read_lock(&iucv_sk_list.lock);
 	sk_for_each(sk, &iucv_sk_list.head) {
-		if (iucv_sk(sk)->hs_dev != dev)
+		/* Cleared before iucv_sock_close() drops the reference:
+		 * whichever value is read is a device the socket still
+		 * holds, or NULL.
+		 */
+		if (READ_ONCE(iucv_sk(sk)->hs_dev) != dev)
 			continue;
 		if (trans_hdr->flags == AF_IUCV_FLAG_SYN) {
 			if ((!memcmp(&iucv_sk(sk)->src_name,

-- 
2.56.0

[PATCH net 2/2] net/iucv: do not count the SHT frame against the receive window

[Bryam Vargas via B4 Relay]

From: Bryam Vargas <hexlabsecurity@proton.me>

iucv_sock_recvmsg() counts every HiperSockets frame it consumes in
msg_recv, the SHT's empty one included, while the peer counts only what
sendmsg() sends against the window it was granted; the SHT goes out
through iucv_send_ctrl() uncounted, one per shutdown(SHUT_WR) call. The
next window update zeroes msg_recv and takes the surplus with it, but
recvmsg() sends none while frames wait in backlog_skb_q, and an SHT read
in that state pushes msg_recv past msglimit: WARN_ON(1) fires, the socket
is closed and the reader gets -EFAULT instead of EOF. A peer that sends
past its window reaches the same WARN from the network; with
panic_on_warn that is a remote panic. The SHT frame dates from
82492a355fac, the WARN and the close from 800c5eb7b5eb.

The SHT reaches recvmsg() as a zero-length frame, like a zero-length
data frame, so test the length: count only frames that still carry
data when the read consumes them (rlen; a partial SOCK_STREAM read
requeues above this point, so each data frame counts once), and close a
socket over the window without the WARN; -EFAULT is kept and nothing is
logged. A zero-length data frame sets RCV_SHUTDOWN, after which
afiucv_hs_callback_rx() drops every later data frame uncounted; the one
slot this patch no longer credits for it is lost like the slots of the
frames dropped after it.

Fixes: 82492a355fac ("af_iucv: add shutdown for HS transport")
Fixes: 800c5eb7b5eb ("af_iucv: change net_device handling for HS transport")
Reported-by: Sashiko <netdev-bot+sashiko@kernel.org>
Closes: https://lore.kernel.org/netdev/179183431327.434549.446348148837850426@kernel.org/
Cc: stable@vger.kernel.org
Signed-off-by: Bryam Vargas <hexlabsecurity@proton.me>
---
Reproduced on s390x under QEMU TCG (net-next 8df0638138d3 and net
f4377bf85887, same result) with an emulated HiperSockets adapter under
the real qeth and qdio drivers: the reader has SO_MSGLIMIT 2 and
SO_RCVBUF 2304 (the minimum); the peer sends 1 byte twice, which fills
the window, then calls shutdown(SHUT_WR) 32 times, one SHT each. The
reader starts once everything has arrived; the receive queue took two
frames, the rest wait in backlog_skb_q, and with that queue non-empty
recvmsg() sends no window update, so the first SHT read counts 3
against a limit of 2: WARN and -EFAULT without the patch, EOF with it.
With one SHT instead of 32 the queue drains, the update goes out after
the two reads, and both kernels read EOF.

Without the patch, the surplus a window update carries for a counted
SHT is credit the peer never spent, and it takes the peer's msg_sent
below zero; harmless, since a peer that sent an SHT has SEND_SHUTDOWN
and its sendmsg() returns -EPIPE. With the patch no SHT is counted, so
no update carries that surplus.

A HiperSockets socket stays in IUCV_BOUND from its SYN until the SYN|ACK
arrives, and SO_MSGLIMIT is accepted in that state, so msglimit can drop
below the window the SYN advertised. Hidayath's test for that [1] runs
against the rework's backlog budget and passes on net-next, where the
update after every read keeps msg_recv low; measured here with
SO_RCVBUF at its minimum, so that frames wait in backlog_skb_q, the
same conforming peer reaches this check (net-next: WARN and -EFAULT
after two of eight messages; with this patch -EFAULT without the
WARN). The rework keeps the advertised window in a field of its own
for that, and this patch only takes the WARN out of its way. That
iucv_sock_shutdown() sends an SHT on every call is a sender-side oddity
left alone here; the receiver has to cope with any peer.

[1] https://lore.kernel.org/netdev/51a4a40a-f34b-4d41-b049-69aa53f4dcf5@linux.ibm.com/
---
 net/iucv/af_iucv.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/net/iucv/af_iucv.c b/net/iucv/af_iucv.c
index 501e758c5e68..a5142081bb6a 100644
--- a/net/iucv/af_iucv.c
+++ b/net/iucv/af_iucv.c
@@ -1317,10 +1317,13 @@ static int iucv_sock_recvmsg(struct socket *sock, struct msghdr *msg,
 		}
 
 		consume_skb(skb);
-		if (iucv->transport == AF_IUCV_TRANS_HIPER) {
+		/* The peer counts only the data it sends against its window;
+		 * the SHT's empty frame is not among them. Over the window,
+		 * close without a WARN: a peer reaches this from the network.
+		 */
+		if (iucv->transport == AF_IUCV_TRANS_HIPER && rlen) {
 			atomic_inc(&iucv->msg_recv);
 			if (atomic_read(&iucv->msg_recv) > iucv->msglimit) {
-				WARN_ON(1);
 				iucv_sock_close(sk);
 				return -EFAULT;
 			}

-- 
2.56.0

Re: [PATCH net 1/2] net/iucv: clear hs_dev before dropping its reference

[sashiko-bot]

> From: Bryam Vargas <hexlabsecurity@proton.me>
> 
> iucv_sock_close() drops the socket's reference on its HiperSockets
> device and only then clears iucv->hs_dev, while afiucv_hs_rcv() compares
> that field against the ingress device for every socket in the list,
> under iucv_sk_list.lock and not the socket lock. Between the two it
> compares a pointer the socket no longer holds; KCSAN reports the plain
> read against the plain write. The order dates from 800c5eb7b5eb, the
> compare from 80230a18c164. Nothing user-visible is known from it: the
> receive path holds the ingress device, so the stale pointer is compared,
> never dereferenced.
> 
> Clear the pointer first, through a local, then drop the reference, and
> mark the lookup's read and the two stores that race with it: the clear
> in close and the listener's child's, which is in iucv_sk_list from
> [ ... ]
> Fixes: 80230a18c164 ("net/iucv: filter frames in afiucv_hs_rcv() by ingress device")
> Reported-by: Hidayath Khan <hidayath@linux.ibm.com>
> Closes: https://lore.kernel.org/netdev/b3fea2ca-25ff-42c0-812c-f8cc6bb6667e@linux.ibm.com/
> Suggested-by: Hidayath Khan <hidayath@linux.ibm.com>
> Signed-off-by: Bryam Vargas <hexlabsecurity@proton.me>

Sashiko has reviewed this patch and found no issues.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261022-b4-disp-b35a9153-v1-0-d74645876195@proton.me?part=1

Re: [PATCH net 2/2] net/iucv: do not count the SHT frame against the receive window

[sashiko-bot]

> From: Bryam Vargas <hexlabsecurity@proton.me>
> 
> iucv_sock_recvmsg() counts every HiperSockets frame it consumes in
> msg_recv, the SHT's empty one included, while the peer counts only what
> sendmsg() sends against the window it was granted; the SHT goes out
> through iucv_send_ctrl() uncounted, one per shutdown(SHUT_WR) call. The
> next window update zeroes msg_recv and takes the surplus with it, but
> recvmsg() sends none while frames wait in backlog_skb_q, and an SHT read
> in that state pushes msg_recv past msglimit: WARN_ON(1) fires, the socket
> is closed and the reader gets -EFAULT instead of EOF. A peer that sends
> past its window reaches the same WARN from the network; with
> panic_on_warn that is a remote panic. The SHT frame dates from
> 82492a355fac, the WARN and the close from 800c5eb7b5eb.
> 
> The SHT reaches recvmsg() as a zero-length frame, like a zero-length
> data frame, so test the length: count only frames that still carry
> data when the read consumes them (rlen; a partial SOCK_STREAM read
> requeues above this point, so each data frame counts once), and close a
> socket over the window without the WARN; -EFAULT is kept and nothing is
> logged. A zero-length data frame sets RCV_SHUTDOWN, after which
> afiucv_hs_callback_rx() drops every later data frame uncounted; the one
> slot this patch no longer credits for it is lost like the slots of the
> frames dropped after it.
> 
> Fixes: 82492a355fac ("af_iucv: add shutdown for HS transport")
> Fixes: 800c5eb7b5eb ("af_iucv: change net_device handling for HS transport")
> Reported-by: Sashiko <netdev-bot+sashiko@kernel.org>
> Closes: https://lore.kernel.org/netdev/179183431327.434549.446348148837850426@kernel.org/
> Cc: stable@vger.kernel.org
> Signed-off-by: Bryam Vargas <hexlabsecurity@proton.me>

Sashiko has reviewed this patch and found no issues.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261022-b4-disp-b35a9153-v1-0-d74645876195@proton.me?part=2