Re: [PATCH net] net/af_iucv: fix use-after-free of listen sock in iucv_callback_connreq()

[Alexandra Winter <wintera@linux.ibm.com>]

On 06.07.26 10:48, Hidayath Khan wrote:
> iucv_callback_connreq() looks up the listening socket in iucv_sk_list
> under read_lock(&iucv_sk_list.lock), drops the lock, and only then
> uses the socket (bh_lock_sock() and the following connection setup).
> No reference is taken on the socket before the lock is released.
> 
> The callback runs from the iucv tasklet. A concurrent close of the
> listening socket does not synchronize with it.
> Between read_unlock() and bh_lock_sock() a concurrent close on another
> CPU can free the socket.
> 
> Fixes: eac3731bd04c ("[S390]: Add AF_IUCV socket support")
> Cc: stable@vger.kernel.org
> Signed-off-by: Hidayath Khan <hidayath@linux.ibm.com>
> ---
>  net/iucv/af_iucv.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/net/iucv/af_iucv.c b/net/iucv/af_iucv.c
> index fed240b453bd..890d9df5ae36 100644
> --- a/net/iucv/af_iucv.c
> +++ b/net/iucv/af_iucv.c
> @@ -1616,6 +1616,8 @@ static int iucv_callback_connreq(struct iucv_path *path,
>  			iucv = iucv_sk(sk);
>  			break;
>  		}
> +	if (iucv)
> +		sock_hold(sk);
>  	read_unlock(&iucv_sk_list.lock);
>  	if (!iucv)
>  		/* No socket found, not one of our paths. */
> @@ -1684,6 +1686,7 @@ static int iucv_callback_connreq(struct iucv_path *path,
>  	err = 0;
>  fail:
>  	bh_unlock_sock(sk);
> +	sock_put(sk);
>  	return 0;
>  }
>  

Instead of adding another refcount couldn't you simply move bh_lock_sock(sk) under
iucv_sk_list.lock ? Like this:
diff --git a/net/iucv/af_iucv.c b/net/iucv/af_iucv.c
index fed240b453bd..84a3f65b7c13 100644
--- a/net/iucv/af_iucv.c
+++ b/net/iucv/af_iucv.c
@@ -1614,6 +1614,7 @@ static int iucv_callback_connreq(struct iucv_path *path,
                         * src_name == ipuser[0-7].
                         */
                        iucv = iucv_sk(sk);
+                       bh_lock_sock(sk);
                        break;
                }
        read_unlock(&iucv_sk_list.lock);
@@ -1621,7 +1622,6 @@ static int iucv_callback_connreq(struct iucv_path *path,
                /* No socket found, not one of our paths. */
                return -EINVAL;

-       bh_lock_sock(sk);

        /* Check if parent socket is listening */
        low_nmcpy(user_data, iucv->src_name);



But actually that doesn't completely close this gap, see my commit message for
f558120cd709 ("net/iucv: fix use after free in iucv_sock_close()")
Iiuc the socket locking in AF_IUCV is incomplete in general.


Some links I stored for my reference (there may be better ones):
https://lore.kernel.org/netdev/1280155406.2899.407.camel@edumazet-laptop/
https://wiki.linuxfoundation.org/networking/socket_locks
https://medium.com/@c0ngwang/the-design-of-lock-sock-in-linux-kernel-69c3406e504b


If you fix this correctly it should also fix the HS over IUCV recieve path.