virtio-devices: refactor VSOCK "connect" parsing #7310

mguentner · 2025-09-02T13:22:22Z

The function read_local_stream_port had no proper handling for unexpected or incomplete input.
When the control socket of the VSOCK device was closed without sending the expected CONNECT <PORT>\n statement completely, the thread got stuck in an infinite loop as it attempted to read from a closed socket over and over again which never returned any data.

This resulted in the thread responsible for epoll being completely blocked. New VSOCK connections could not be established and existing ones became defunct, effectively leading to a Denial of Service of the entire VSOCK device.

The issue can be reproduced by opening a socket and immediately closing it.

socat - UNIX-CONNECT:/socket.vsock
<Ctrl-C>

Instead of applying a quick fix by handling the EPOLLHUP event before reading, the function is refactored to remove the error-prone while loop and multiple reads.
Notably, we now check if the number of bytes read is zero, which occurs when event_set == EPOLLHUP | EPOLLIN, indicating that the socket has been closed by the client.

Additionally, the actual parsing code is now extracted into a dedicated function that is tested.

Fixes: #6798

rbradford

@alyssais Would you like to take a look at this as our vsock expert :-)

alyssais · 2025-09-05T11:05:57Z

Happy to but won't get a chance until next week at the earliest as I'm at KVM Forum

virtio-devices/src/vsock/unix/muxer.rs

alyssais · 2025-09-03T18:03:07Z

virtio-devices/src/vsock/unix/muxer.rs

+        // to read past the `\n` character which might swallow actual application data
+        // alternative: the bytes that might have been read beyond `\n` would need
+        // to be sent somehow via `MuxerConnection` prior to reading from `stream` again
+        let read_bytes = stream


A more practical eventual alternative would be to use the UnixStream::peak API, assuming it stabilizes. We could link to rust-lang/rust#76923 and then it would be obvious when this can be improved.

(Just to be clear: the reason I mention this is because I think the comment should mention it.)

Added the link :)

virtio-devices/src/vsock/unix/muxer.rs

rbradford · 2025-09-10T14:04:05Z

Maybe simplify it all by making read_local_stream_port() use BufReader::new() and use .read_until(). It looks like this code is trying to reimplement a good chunk of BufReader...

mguentner · 2025-09-10T14:33:01Z

@alyssais I adjusted the code with your suggestions. Regarding the constants, I hope to have found the middle ground between your comment and what @RuoqingHe said. Having the strings referenced as consts can at least prevent some bugs when dealing repeatedly with the same string (typos).

@rbradford Can you please sketch how that would work? BufReader::read_until is blocking, since we are using epoll here, we need to do non-blocking reads

virtio-devices/src/vsock/unix/muxer.rs

rbradford · 2025-09-11T15:17:55Z

@alyssais I adjusted the code with your suggestions. Regarding the constants, I hope to have found the middle ground between your comment and what @RuoqingHe said. Having the strings referenced as consts can at least prevent some bugs when dealing repeatedly with the same string (typos).

@rbradford Can you please sketch how that would work? BufReader::read_until is blocking, since we are using epoll here, we need to do non-blocking reads

Right, but don't we know that for the first read we can block until we get the connect message?

mguentner · 2025-09-11T15:30:30Z

@alyssais I adjusted the code with your suggestions. Regarding the constants, I hope to have found the middle ground between your comment and what @RuoqingHe said. Having the strings referenced as consts can at least prevent some bugs when dealing repeatedly with the same string (typos).
@rbradford Can you please sketch how that would work? BufReader::read_until is blocking, since we are using epoll here, we need to do non-blocking reads

Right, but don't we know that for the first read we can block until we get the connect message?

Only when we can trust a client to send the connect statement at once. Any client that doesn't, be it malicious or just due to a bug, will stall the vsock device/thread, therefore leading again to a DOS.

virtio-devices/src/vsock/unix/muxer.rs

alyssais · 2025-09-13T12:31:20Z

virtio-devices/src/vsock/unix/muxer.rs

+        // to read past the `\n` character which might swallow actual application data
+        // alternative: the bytes that might have been read beyond `\n` would need
+        // to be sent somehow via `MuxerConnection` prior to reading from `stream` again
+        let read_bytes = stream


(Just to be clear: the reason I mention this is because I think the comment should mention it.)

alyssais · 2025-09-13T12:32:51Z

virtio-devices/src/vsock/unix/muxer.rs

    LocalStream(UnixStream),
 }

+const PARTIALLY_READ_COMMAND_BUF_SIZE: usize = 32;


I know this predates this change, but I wonder why the buffer is so big. The maximum possible command is 19 bytes long if I've counted correctly, so we shouldn't ever need to store more than that.

Correct. My math is:

> len("CONNECT 4294967296\n") > 19

However: With 19 byte we would require the client to send exactly this string, with some head space we would allow for some stray whitespaces, i.e. "CONNECT 4294967296\n".

I've got to wonder how stray whitespaces would end up in the string from a client, and whether we want to allow that…

We can only assume some strange client code implementation.
However the current code does support e.g. CONNECT 1337\n.
I did not want to break client implementations with this PR.

Is the firecracker protocol defined somewhere, ideally as BNF?

I doubt it.

virtio-devices/src/vsock/unix/muxer.rs

The function `read_local_stream_port` had no proper handling for unexpected or incomplete input. When the control socket of the VSOCK device was closed without sending the expected `CONNECT <PORT>\n` statement completely, the thread got stuck in an infinite loop as it attempted to read from a closed socket over and over again which never returned any data. This resulted in the thread responsible for `epoll` being completely blocked. New VSOCK connections could not be established and existing ones became defunct, effectively leading to a Denial of Service of the entire VSOCK device. The issue can be reproduced by opening a socket and immediately closing it. ``` socat - UNIX-CONNECT:/socket.vsock <Ctrl-C> ``` Instead of applying a quick fix by handling the `EPOLLHUP` event before reading, the function is refactored to remove the error-prone `while` loop and multiple `read`s. Notably, we now check if the number of bytes read is zero, which occurs when `event_set == EPOLLHUP | EPOLLIN`, indicating that the socket has been closed by the client. Additionally, the actual parsing code is now extracted into a dedicated function that is tested. Fixes: cloud-hypervisor#6798 Signed-off-by: Maximilian Güntner <[email protected]>

mguentner · 2025-09-16T13:33:44Z

Thanks for the review, @alyssais :)

RuoqingHe

Thanks @mguentner for your patience, the changes look good to me

mguentner · 2025-09-17T13:21:52Z

Can someone with the necessary rights please retry the failing CI step. It failed already 2 times due to a timeout, I do not see vsock being involved in the last tests. Random failure?

mguentner requested a review from a team as a code owner September 2, 2025 13:22

mguentner force-pushed the mguentner/fix_vsock_connect_parsing branch 4 times, most recently from ed2ddcf to d4ccb3e Compare September 2, 2025 13:45

mguentner marked this pull request as draft September 2, 2025 14:02

mguentner force-pushed the mguentner/fix_vsock_connect_parsing branch from d4ccb3e to 4fda2ab Compare September 2, 2025 15:56

mguentner marked this pull request as ready for review September 2, 2025 16:47

mguentner force-pushed the mguentner/fix_vsock_connect_parsing branch 3 times, most recently from 1ee095d to 00db0c4 Compare September 2, 2025 16:53

rbradford reviewed Sep 4, 2025

View reviewed changes

RuoqingHe reviewed Sep 8, 2025

View reviewed changes

virtio-devices/src/vsock/unix/muxer.rs Outdated Show resolved Hide resolved

mguentner force-pushed the mguentner/fix_vsock_connect_parsing branch from 00db0c4 to 0849796 Compare September 10, 2025 13:02

alyssais reviewed Sep 10, 2025

View reviewed changes

mguentner force-pushed the mguentner/fix_vsock_connect_parsing branch from 0849796 to 9c94e62 Compare September 10, 2025 14:24

alyssais reviewed Sep 10, 2025

View reviewed changes

mguentner force-pushed the mguentner/fix_vsock_connect_parsing branch 2 times, most recently from a8af51c to 0acd943 Compare September 11, 2025 08:56

alyssais reviewed Sep 13, 2025

View reviewed changes

mguentner force-pushed the mguentner/fix_vsock_connect_parsing branch 2 times, most recently from 34ac0e6 to a124595 Compare September 13, 2025 14:37

alyssais reviewed Sep 13, 2025

View reviewed changes

virtio-devices/src/vsock/unix/muxer.rs Show resolved Hide resolved

virtio-devices/src/vsock/unix/muxer.rs Outdated Show resolved Hide resolved

mguentner force-pushed the mguentner/fix_vsock_connect_parsing branch from a124595 to e428f89 Compare September 13, 2025 22:29

alyssais approved these changes Sep 14, 2025

View reviewed changes

RuoqingHe approved these changes Sep 16, 2025

View reviewed changes

likebreath added this pull request to the merge queue Sep 17, 2025

Merged via the queue into cloud-hypervisor:main with commit d28d9eb Sep 17, 2025
40 of 42 checks passed

mguentner deleted the mguentner/fix_vsock_connect_parsing branch September 18, 2025 08:07

likebreath added the bug-fix Bug fix to include in release notes label Nov 6, 2025

likebreath moved this to ✅ Done in Cloud Hypervisor Roadmap Nov 6, 2025

likebreath added this to Cloud Hypervisor Roadmap Nov 6, 2025

virtio-devices: refactor VSOCK "connect" parsing #7310

virtio-devices: refactor VSOCK "connect" parsing #7310

Uh oh!

Conversation

mguentner commented Sep 2, 2025

Uh oh!

rbradford left a comment

Choose a reason for hiding this comment

Uh oh!

alyssais commented Sep 5, 2025

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

rbradford commented Sep 10, 2025

Uh oh!

mguentner commented Sep 10, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

rbradford commented Sep 11, 2025

Uh oh!

mguentner commented Sep 11, 2025

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

mguentner commented Sep 16, 2025

Uh oh!

RuoqingHe left a comment

Choose a reason for hiding this comment

Uh oh!

mguentner commented Sep 17, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants