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 for Android: free password hash cracker in your pocket

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

Message-ID: <20230222110534-mutt-send-email-mst@kernel.org>
Date:   Wed, 22 Feb 2023 11:11:06 -0500
From:   "Michael S. Tsirkin" <mst@...hat.com>
To:     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

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.

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