Re: [patch net-next v2] net: virtio_net: implement exact header length guest feature

[Willem de Bruijn <willemdebruijn.kernel@gmail.com>]

Michael S. Tsirkin wrote:
> On Wed, Feb 22, 2023 at 10:14:21AM -0500, Willem de Bruijn wrote:
> > Either including the link that Michael shared or quoting the relevant
> > part verbatim in the commit message would help, thanks.
> > 
> > Thinking it over, my main concern is that the prescriptive section in
> > the spec does not state what to do when the value is clearly garbage,
> > as we have seen with syzkaller.
> > 
> > Having to sanitize input, by dropping if < ETH_HLEN or > length, to
> > me means that the device cannot trust the field, as the spec says it
> > should. 
> 
> Right. I think the implication is that if device detects and illegal
> value it's OK for it to just drop the packet or reset or enter
> a broken mode until reset.
> 
> By contrast without the feature bit the header size can be
> used as a hint e.g. to size allocations but you must
> recover if it's incorrect.
> 
> And yes tap seems to break if you make it too small or if you make
> it huge so it does not really follow the spec in this regard.
> 
> Setting the flag will not fix tap because we can't really
> affort breaking all drivers who don't set it. But it will
> prepare the ground for when tens of years from now we
> actually look back and say all drivers set it, no problem.
> 
> So that's a good reason to ack this patch.

I also have no concerns with the commit itself. It would become an
issue only if tap would support it and trust hdr_len unconditionally.

Acked-by: Willem de Bruijn <willemb@google.com>
 
> However if someone is worried about this then fixing tap
> so it recovers from incorrect header length without
> packet loss is a good idea.
> 
> > Sanitization is harder in the kernel, because it has to support all
> > kinds of link layers, including variable length.
> > 
> > Perhaps that's a discussion for the spec rather than this commit. But
> > it's a point to clarify as we add support to the code.
> 
> -- 
> MST
>