Re: [PATCH 4/4] tap: Drop frames from guest whose length is more than remaining buffer

On Fri, 28 Jun 2024 17:19:37 +1000 David Gibson wrote:

On Thu, Jun 27, 2024 at 10:21:04AM +0200, Stefano Brivio wrote:

...

On Thu, 27 Jun 2024 11:30:02 +1000 David Gibson wrote:

...

On Thu, Jun 27, 2024 at 01:45:36AM +0200, Stefano Brivio wrote:

...

This was reported by Matej a while ago, but we forgot to fix it. Even if the hypervisor is necessarily trusted by passt, as it can in any case terminate the guest or disrupt guest connectivity, it's a good idea to be robust against possible issues.

Instead of resetting the connection to the hypervisor, just skip the offending frame, as we had a few cases where QEMU would get the length descriptor wrong, in the past.

Reported-by: Matej Hrica Suggested-by: Matej Hrica Signed-off-by: Stefano Brivio --- tap.c | 3 +++ 1 file changed, 3 insertions(+)

diff --git a/tap.c b/tap.c index 7f8c26d..bb993e0 100644 --- a/tap.c +++ b/tap.c @@ -1026,6 +1026,9 @@ redo:

So.. I just realised there's a different pre-existing problem here, above what's quoted in the patch we have:

ssize_t l2len = ntohl(*(uint32_t *)p);

On a platform where ssize_t is only 32-bits we could get a negative value here, which would be very bad.

We can't, because the length is anyway limited to the maximum IPv4 path MTU in any hypervisor we might be talking to.

Only if we trust the hypervisor. And that the user didn't misconfigure us to point the socket at something that's not actually a hypervisor. And that it's not some future version of the hypervisor configured to use a different framing format. And that our code is robust enough to never get out of sync on the stream.

I really think it's better to read this into a u32, and range sanity check it before we do anything else.

Right... see v2? -- Stefano