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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <63f64f6053a19_189d62088a@willemb.c.googlers.com.notmuch>
Date:   Wed, 22 Feb 2023 12:22:40 -0500
From:   Willem de Bruijn <willemdebruijn.kernel@...il.com>
To:     "Michael S. Tsirkin" <mst@...hat.com>,
        Willem de Bruijn <willemdebruijn.kernel@...il.com>
Cc:     Jiri Pirko <jiri@...nulli.us>, netdev@...r.kernel.org,
        davem@...emloft.net, kuba@...nel.org, pabeni@...hat.com,
        edumazet@...gle.com, jasowang@...hat.com,
        virtualization@...ts.linux-foundation.org,
        alvaro.karsz@...id-run.com, vmireyno@...vell.com, parav@...dia.com
Subject: Re: [patch net-next v2] net: virtio_net: implement exact header
 length guest feature

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@...gle.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
>