[PATCH net] net/iucv: reconcile the socket state when connect() severs the path

[PATCH net] net/iucv: reconcile the socket state when connect() severs the path

[Bryam Vargas via B4 Relay]

From: Bryam Vargas <hexlabsecurity@proton.me>

A connack can land between iucv_sock_wait() returning an error and the
sever at the end of iucv_sock_connect(): iucv_callback_connack() writes
IUCV_CONNECTED from the iucv tasklet with no socket lock, and lock_sock()
does not exclude it. The sever clears iucv->path and leaves the state, so
connect() fails on a socket still claiming a connection it no longer has,
and a later sendmsg() reaches iucv->path->msglim through the NULL pointer
inside iucv_below_msglim(). The sever runs on every failed classic
connect, O_NONBLOCK included.

Reconcile only that case, so an ordinary failed connect still returns the
socket to its pre-connect state. The sever is a barrier against a later
connack: iucv_path_sever() clears the path from iucv_path_table[] under
the lock the tasklet holds across dispatch.

Fixes: 18becbc5479f ("af_iucv: avoid left over IUCV connections from failing connects")
Cc: stable@vger.kernel.org
Signed-off-by: Bryam Vargas <hexlabsecurity@proton.me>
---
 net/iucv/af_iucv.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/net/iucv/af_iucv.c b/net/iucv/af_iucv.c
index ea047bab65e7..6fb0041c4c98 100644
--- a/net/iucv/af_iucv.c
+++ b/net/iucv/af_iucv.c
@@ -778,8 +778,16 @@ static int iucv_sock_connect(struct socket *sock, struct sockaddr_unsized *addr,
 	if (sk->sk_state == IUCV_DISCONN || sk->sk_state == IUCV_CLOSED)
 		err = -ECONNREFUSED;
 
-	if (err && iucv->transport == AF_IUCV_TRANS_IUCV)
+	if (err && iucv->transport == AF_IUCV_TRANS_IUCV) {
 		iucv_sever_path(sk, 0);
+		/* A connack may have landed while the wait was unwinding; the
+		 * path is gone, so the socket must not still claim it.
+		 */
+		if (sk->sk_state == IUCV_CONNECTED) {
+			sk->sk_state = IUCV_DISCONN;
+			sk->sk_state_change(sk);
+		}
+	}
 
 done:
 	release_sock(sk);

---
base-commit: a59f57e2aa127c5354168d2ec4bac920df1be4f4
change-id: 20260815-b4-disp-e1faf4b7-feddd1575cc2

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

Re: [PATCH net] net/iucv: reconcile the socket state when connect() severs the path

[sashiko-bot]

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

Pre-existing issues:
- [Critical] The IUCV tasklet callback `iucv_callback_connrej` ignores socket ownership, leading to asynchronous nullification and freeing of `iucv->path` while process-context socket functions (like `iucv_sock_sendmsg`) are concurrently dereferencing it, causing a Use-After-Free or NULL pointer dereference.
--

--- Patch [1]: [PATCH net] net/iucv: reconcile the socket state when connect() severs the path ---
commit 8d8ee46f28106e54c267f0a51094885740ee5c07
Author: Bryam Vargas <hexlabsecurity@proton.me>

net/iucv: reconcile the socket state when connect() severs the path

This commit ensures a failed connect properly returns the socket state to
disconnected. This prevents a race where a connack lands between the wait
and sever, causing a later sendmsg() to dereference a NULL path pointer.

Cc: stable@vger.kernel.org

> diff --git a/net/iucv/af_iucv.c b/net/iucv/af_iucv.c
> index ea047bab65e7c..6fb0041c4c987 100644
> --- a/net/iucv/af_iucv.c
> +++ b/net/iucv/af_iucv.c
> @@ -778,8 +778,16 @@ static int iucv_sock_connect(struct socket *sock, struct sockaddr_unsized *addr,
>  	if (sk->sk_state == IUCV_DISCONN || sk->sk_state == IUCV_CLOSED)
>  		err = -ECONNREFUSED;
>  
> -	if (err && iucv->transport == AF_IUCV_TRANS_IUCV)
> +	if (err && iucv->transport == AF_IUCV_TRANS_IUCV) {
>  		iucv_sever_path(sk, 0);
> +		/* A connack may have landed while the wait was unwinding; the
> +		 * path is gone, so the socket must not still claim it.
> +		 */
> +		if (sk->sk_state == IUCV_CONNECTED) {
> +			sk->sk_state = IUCV_DISCONN;
> +			sk->sk_state_change(sk);
> +		}
> +	}

[Severity: Critical]
This is a pre-existing issue, but looking at how the iucv tasklets interact
with the socket state, can a remote sever cause a NULL pointer dereference
while process context holds the socket lock?

The tasklet callback iucv_callback_connrej() acquires the spinlock via
bh_lock_sock(sk) and calls iucv_sever_path(sk, 1) unconditionally. It does
not check sock_owned_by_user(sk) to defer execution if the process context
currently holds lock_sock():

net/iucv/af_iucv.c:iucv_callback_connrej() {
        ...
        bh_lock_sock(sk);
        iucv_sever_path(sk, 1);
        sk->sk_state = IUCV_DISCONN;
        sk->sk_state_change(sk);
        bh_unlock_sock(sk);
        ...
}

Since lock_sock() enables bottom-halves (setting owned=1 but releasing
slock), a tasklet can interrupt it. If iucv_sock_sendmsg() is running in
process context, it checks the socket state and then dereferences iucv->path:

net/iucv/af_iucv.c:iucv_sock_sendmsg() {
        ...
        if (((iucv->path->flags & IUCV_IPRMDATA) & iucv->flags) &&
            skb->len <= 7) {
        ...
}

Similarly, if the tasklet interrupts iucv_below_msglim() right after the
state check but before the dereference:

net/iucv/af_iucv.c:iucv_below_msglim() {
        ...
        if (sk->sk_state != IUCV_CONNECTED)
                return 1;
        if (iucv->transport == AF_IUCV_TRANS_IUCV)
                return (atomic_read(&iucv->skbs_in_xmit) <
                        iucv->path->msglim);
        ...
}

Could the tasklet free and nullify iucv->path concurrently, resulting in a
use-after-free or NULL pointer dereference in these cases?

>  
>  done:
>  	release_sock(sk);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260815-b4-disp-e1faf4b7-v1-1-2a8a143a0055@proton.me?part=1

Re: [PATCH net] net/iucv: reconcile the socket state when connect() severs the path

[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: reconcile the socket state when connect() severs the path

When a classic AF_IUCV connect() fails and severs the path, the socket
could be left in IUCV_CONNECTED with iucv->path already NULL. The patch
adds a state reconciliation to IUCV_DISCONN right after iucv_sever_path()
so that a later sendmsg() does not walk the NULL path pointer.

> diff --git a/net/iucv/af_iucv.c b/net/iucv/af_iucv.c
> index ea047bab65e7c..6fb0041c4c987 100644
> --- a/net/iucv/af_iucv.c
> +++ b/net/iucv/af_iucv.c
> @@ -778,8 +778,16 @@ static int iucv_sock_connect(struct socket *sock, struct sockaddr_unsized *addr,
>  	if (sk->sk_state == IUCV_DISCONN || sk->sk_state == IUCV_CLOSED)
>  		err = -ECONNREFUSED;
>  
> -	if (err && iucv->transport == AF_IUCV_TRANS_IUCV)
> +	if (err && iucv->transport == AF_IUCV_TRANS_IUCV) {
>  		iucv_sever_path(sk, 0);
> +		/* A connack may have landed while the wait was unwinding; the
> +		 * path is gone, so the socket must not still claim it.
> +		 */
> +		if (sk->sk_state == IUCV_CONNECTED) {
> +			sk->sk_state = IUCV_DISCONN;
> +			sk->sk_state_change(sk);
> +		}
> +	}

[Severity: Critical]
This isn't a bug introduced by this patch, but it should probably be
fixed at the same time. Does the same CONNECTED-with-NULL-path state
remain reachable from the HiperSockets receive path, so that the NULL
dereference described in the changelog survives?

afiucv_hs_rcv() matches sockets on the global iucv_sk_list by name only:

net/iucv/af_iucv.c:afiucv_hs_rcv() {
	...
	sk_for_each(sk, &iucv_sk_list.head) {
		if (trans_hdr->flags == AF_IUCV_FLAG_SYN) {
			...
		} else {
			if ((!memcmp(&iucv_sk(sk)->src_name,
				     trans_hdr->destAppName, 8)) &&
			    ...
			    (!memcmp(&iucv_sk(sk)->dst_user_id,
				     trans_hdr->srcUserID, 8))) {
				iucv = iucv_sk(sk);
				break;
	...
}

There is no check of iucv->transport, no check of iucv->hs_dev and no
network namespace check, and iucv_packet_type is registered globally in
afiucv_init() with dev_add_pack(&iucv_packet_type) and no .dev filter.

afiucv_hs_callback_synack() then only requires IUCV_BOUND before writing
the connected state:

net/iucv/af_iucv.c:afiucv_hs_callback_synack() {
	if (!iucv || sk->sk_state != IUCV_BOUND) {
		kfree_skb(skb);
		return NET_RX_SUCCESS;
	}

	bh_lock_sock(sk);
	iucv->msglimit_peer = iucv_trans_hdr(skb)->window;
	sk->sk_state = IUCV_CONNECTED;
	sk->sk_state_change(sk);
	...
}

So a classic-transport socket that has only been bound, or one whose
connect() failed with the wait timing out or being interrupted (path
severed to NULL, state left at IUCV_BOUND, which the new code does not
reconcile), can end up at IUCV_CONNECTED with iucv->path == NULL.

Once that happens, iucv_below_msglim() dereferences the NULL path:

net/iucv/af_iucv.c:iucv_below_msglim() {
	if (sk->sk_state != IUCV_CONNECTED)
		return 1;
	if (iucv->transport == AF_IUCV_TRANS_IUCV)
		return (atomic_read(&iucv->skbs_in_xmit) < iucv->path->msglim);
	...
}

and it is called from iucv_sock_poll():

	if (sock_writeable(sk) && iucv_below_msglim(sk))

which the sk_state_change() above has just woken, as well as from
iucv_sock_sendmsg() via iucv_sock_wait(sk, iucv_below_msglim(sk), timeo).
iucv_sock_sendmsg() also reads iucv->path->flags:

	if (((iucv->path->flags & IUCV_IPRMDATA) & iucv->flags) &&

and passes iucv->path to pr_iucv->message_send().

The changelog says:

    The sever is a barrier against a later connack: iucv_path_sever()
    clears the path from iucv_path_table[] under the lock the tasklet
    holds across dispatch.

Is that statement complete? It holds for the classic iucv tasklet, where
iucv_tasklet_fn() holds iucv_table_lock across dispatch, but the
HiperSockets callbacks run from the netdev RX softirq under
bh_lock_sock() only, and are not excluded by iucv_table_lock.

Would it make sense to also check iucv->transport (and iucv->hs_dev)
in the afiucv_hs_rcv() socket lookup or in afiucv_hs_callback_synack(),
and/or to have iucv_below_msglim() and iucv_sock_sendmsg() test
iucv->path before dereferencing it?
-- 
pw-bot: cr