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

[Stefano Brivio <sbrivio@redhat.com>]

On Fri, 28 Jun 2024 17:19:37 +1000
David Gibson <david@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@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@redhat.com>
> > > > Suggested-by: Matej Hrica <mhrica@redhat.com>
> > > > Signed-off-by: Stefano Brivio <sbrivio@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