Re: [PATCH net] vsock: do not reset a socket in connect() once it has connected

[Michal Luczaj <mhal@rbox.co>]

On 8/19/26 10:54, Stefano Garzarella wrote:
>>>> diff --git a/net/vmw_vsock/af_vsock.c b/net/vmw_vsock/af_vsock.c
>>>> index 622dbd046799..39c42ef016c3 100644
>>>> --- a/net/vmw_vsock/af_vsock.c
>>>> +++ b/net/vmw_vsock/af_vsock.c
>>>> @@ -1807,15 +1807,18 @@ static int vsock_connect(struct socket *sock, struct sockaddr_unsized *addr,
>>>>  		timeout = schedule_timeout(timeout);
>>>>  		lock_sock(sk);
>>>>
>>>> -		/* Connection established. Whatever happens to socket once we
>>>> +		/* Connection (has been) established. Whatever happens to socket once we
>>>>  		 * release it, that's not connect()'s concern. No need to go
>>>>  		 * into signal and timeout handling. Call it a day.
>>>>  		 *
>>>>  		 * Note that allowing to "reset" an already established socket
>>>>  		 * here is racy and insecure.
>>>>  		 */
>>>> -		if (sk->sk_state == TCP_ESTABLISHED)
>>>> -			break;
>>>> +		if (sk->sk_state == TCP_ESTABLISHED ||
>>>> +		    sk->sk_state == TCP_CLOSING) {
>>>> +			err = -sk->sk_err;
>>>> +			goto out_wait;
>>>> +		}
>>>>
>>>>  		/* If connection was _not_ established and a signal/timeout came
>>>>  		 * to be, we want the socket's state reset. User space may want
>>>>
>>>> ?
>>>
>>> Yes, I like it better than mine. I confirmed it fixes the issue.
>>
>> Great, thanks.
>>
>>> If you don't mind, would you take the patch from here?
>>
>> Sure, no problem.
>>
>> Stefano, does this look good to you?
> 
> Yep, thanks for helping here!
> 
> My only doubt is if it makes sense to leave the `break` there, and add a 
> similar check before resetting the socket, I mean something like this:
> 
> 	err = sock_error(sk);
> 	if (err && sk->sk_state != TCP_ESTABLISHED &&
> 		   sk->sk_state != TCP_CLOSING)) {
> 		sk->sk_state = TCP_CLOSE;
> 		sock->state = SS_UNCONNECTED;
> 	}
> 
> Just to be a bit more defensive, but I don't have a strong opinion, your 
> version is also fine.
> 
>> And should any sk_err be consumed
>> here, too? (`err = sock_error(sk)` instead of `err = -sk->sk_err`)
> 
> I'd stay with sock_error() to consume the error if it makes sense also 
> for you.

Sure, one note: perhaps it's better to always return 0 on
TCP_ESTABLISHED/TCP_CLOSING? From connect()'s PoV these two states mean "we
did good, we connected to something for some time". Currently, even if the
connection was established, virtio_transport_recv_connected()'s
VIRTIO_VSOCK_OP_RW error handling may set sk_err = ENOBUFS, and connect()
would return that. Even though connect() itself was a success.

Anyway, here's a fix for the uaf:
https://lore.kernel.org/netdev/20260909-vsock-connect-reset-closing-v1-1-50298b9ccfbf@rbox.co/
It's incomplete, but I'm not sure how to follow up. Please see the
below---comment.

thanks,
Michal