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 <david(a)gibson.dropbear.id.au> 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 <david(a)gibson.dropbear.id.au> 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 <mhrica(a)redhat.com>
 Suggested-by: Matej Hrica <mhrica(a)redhat.com>
 Signed-off-by: Stefano Brivio <sbrivio(a)redhat.com>
 ---
  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