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

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 <[email protected]>

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: [email protected]

> 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/[email protected]?part=1