Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
Pre-existing issues: - [Critical] Calling release_sock() after sock_put() in hvs_open_connection() leads to a use-after-free if the last reference is dropped. - [High] Failed connection handshakes leak the listening socket's sk_ack_backlog count in vmci_transport_recv_listen(), leading to a permanent DoS. - [High] Leaked sk_ack_backlog in virtio_transport_recv_listen() on transport assignment failure, leading to a permanent DoS. -- commit b2cf725bf1bbce146242b6299dd0f179a06ce499 Author: Raf Dickson <[email protected]> vsock: use sk_acceptq_is_full() helper in all transports Replace the open-coded backlog check with sk_acceptq_is_full(). The helper uses > instead of >=, which is the correct comparison per commit 64a146513f8f ("[NET]: Revert incorrect accept queue backlog changes."), and adds READ_ONCE() for proper memory ordering. Link: https://patch.msgid.link/[email protected] > diff --git a/net/vmw_vsock/hyperv_transport.c > b/net/vmw_vsock/hyperv_transport.c > index 432fcbbd14d4f..2c38c8ec2c829 100644 > --- a/net/vmw_vsock/hyperv_transport.c > +++ b/net/vmw_vsock/hyperv_transport.c > @@ -323,7 +323,7 @@ static void hvs_open_connection(struct vmbus_channel > *chan) > goto out; > > if (conn_from_host) { > - if (sk->sk_ack_backlog >= sk->sk_max_ack_backlog) > + if (sk_acceptq_is_full(sk)) > goto out; > > new = vsock_create_connected(sk); [Severity: Critical] This is a pre-existing issue, but does calling release_sock() after sock_put() in hvs_open_connection() lead to a use-after-free? In hvs_open_connection(), if the reference dropped by sock_put(sk) is the final one, the socket memory will be freed, but release_sock(sk) is called immediately after: out: /* Release refcnt obtained when we called vsock_find_bound_socket() */ sock_put(sk); release_sock(sk); Should release_sock(sk) be called before sock_put(sk) to prevent dereferencing freed memory? > diff --git a/net/vmw_vsock/vmci_transport.c b/net/vmw_vsock/vmci_transport.c > index 7eccd6708d664..0198d06ce3d3e 100644 > --- a/net/vmw_vsock/vmci_transport.c > +++ b/net/vmw_vsock/vmci_transport.c > @@ -1000,7 +1000,7 @@ static int vmci_transport_recv_listen(struct sock *sk, > * reset. Otherwise we create and initialize a child socket and reply > * with a connection negotiation. > */ > - if (sk->sk_ack_backlog >= sk->sk_max_ack_backlog) { > + if (sk_acceptq_is_full(sk)) { > vmci_transport_reply_reset(pkt); > return -ECONNREFUSED; > } [Severity: High] This isn't a bug introduced by this patch, but could the error path in vmci_transport_recv_listen() leak the listening socket's sk_ack_backlog count on a failed connection handshake? Earlier in vmci_transport_recv_listen(), if the handshake fails, the pending socket is removed: if (err < 0) vsock_remove_pending(sk, pending); release_sock(pending); vmci_transport_release_pending(pending); return err; Since sk_acceptq_added() was previously called, shouldn't this path also call sk_acceptq_removed(sk) to decrement the listener's backlog? Without it, legitimate connection attempts might be permanently rejected once the counter reaches the maximum limit. [Severity: High] This is a pre-existing issue, but is there a leak of sk_ack_backlog in virtio_transport_recv_listen() on transport assignment failure? In net/vmw_vsock/virtio_transport_common.c, in the error path where the socket is destroyed: ret = vsock_assign_transport(vchild, vsk); /* Transport assigned (looking at remote_addr) must be the same * where we received the request. */ if (ret || vchild->transport != &t->transport) { release_sock(child); virtio_transport_reset_no_sock(t, skb); sock_put(child); return ret; } Because sk_acceptq_added() was already called for this connection, does this path need to call sk_acceptq_removed(sk) to prevent a permanent denial of service where no further connections are accepted? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=1
