netdev - Re: [PATCH] vsock: remove vsock from connected table when connect is interrupted by a signal

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20220216161122.eacdfgljg2c6yeby@sgarzare-redhat>
Date:   Wed, 16 Feb 2022 17:11:22 +0100
From:   Stefano Garzarella <sgarzare@...hat.com>
To:     Seth Forshee <sforshee@...italocean.com>
Cc:     "David S. Miller" <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>,
        virtualization@...ts.linux-foundation.org, netdev@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH] vsock: remove vsock from connected table when connect is
 interrupted by a signal

Hi Seth,

On Wed, Feb 16, 2022 at 08:32:22AM -0600, Seth Forshee wrote:
>vsock_connect() expects that the socket could already be in the
>TCP_ESTABLISHED state when the connecting task wakes up with a signal
>pending. If this happens the socket will be in the connected table, and
>it is not removed when the socket state is reset. In this situation it's
>common for the process to retry connect(), and if the connection is
>successful the socket will be added to the connected table a second
>time, corrupting the list.
>
>Prevent this by calling vsock_remove_connected() if a signal is received
>while waiting for a connection. This is harmless if the socket is not in
>the connected table, and if it is in the table then removing it will
>prevent list corruption from a double add.
>
>Signed-off-by: Seth Forshee <sforshee@...italocean.com>
>---
> net/vmw_vsock/af_vsock.c | 1 +
> 1 file changed, 1 insertion(+)
>
>diff --git a/net/vmw_vsock/af_vsock.c b/net/vmw_vsock/af_vsock.c
>index 3235261f138d..38baeb189d4e 100644
>--- a/net/vmw_vsock/af_vsock.c
>+++ b/net/vmw_vsock/af_vsock.c
>@@ -1401,6 +1401,7 @@ static int vsock_connect(struct socket *sock, struct sockaddr *addr,
> 			sk->sk_state = sk->sk_state == TCP_ESTABLISHED ? TCP_CLOSING : TCP_CLOSE;
> 			sock->state = SS_UNCONNECTED;
> 			vsock_transport_cancel_pkt(vsk);
>+			vsock_remove_connected(vsk);
> 			goto out_wait;
> 		} else if (timeout == 0) {
> 			err = -ETIMEDOUT;
>-- 
>2.32.0
>

Thanks for this fix! The patch LGTM:

Reviewed-by: Stefano Garzarella <sgarzare@...hat.com>


@Dave, @Jakub, since we need this also in stable branches, I was going 
to suggest adding a Fixes tag, but I'm a little confused: the issue 
seems to have always been there, so from commit d021c344051a ("VSOCK: 
Introduce VM Sockets"), but to use vsock_remove_connected() as we are 
using in this patch, we really need commit d5afa82c977e ("vsock: correct 
removal of socket from the list").

Commit d5afa82c977e was introduces in v5.3 and it was backported in 
v4.19 and v4.14, but not in v4.9.
So if we want to backport this patch also for v4.9, I think we need 
commit d5afa82c977e as well.

Thanks,
Stefano