Re: [PATCH v7 1/2] virtio-net: define UDP tunnel segmentation offload feature

[Paolo Abeni <pabeni@redhat.com>]

On 8/21/24 21:48, Willem de Bruijn wrote:
>>> +Geneve UDP tunnel (again IPv4) with no tunnel options. Note that the
>>> +only relevant variable related to the tunnel type is the tunnel header length.
>>> +The packet will have a 14 byte outer ethernet header, 20 byte outer IP header
>>> +followed by the 8 byte UDP header (with a 0 checksum value), 8 byte Geneve header,
>>> +14 byte inner ethernet header, 20 byte inner IP header
>>> +and the TCP header (with the TCP checksum field 16 bytes
>>> +into that header). \field{csum_start} will be 14+20+8+8+14+20 = 84 (the TCP
>>> +checksum includes the header), \field{csum_offset} will be 16.
>>
>> The csum_offset field is not even needed in virtio_net_hdr if gso is
>> enabled, as then the gso type already defines the protocol.
> 
> We recently started testing this invariant in the Linux code. We have
> learned the hard way to not trust userspace processes to set up any
> fields correctly.
> 
> For the new fields, we should from the start think of such invariants
> that can detect bad input. For instance,
> 
>      outer_th_offset < inner_nh_offset
>      inner_nh_offset < csum_start
>      csum_start < pkt_len
> 
> Maybe even more stringent invariant checks are possible. The only
> ones that truly matter are the ones that can affect device side
> correctness (such as host kernel crashes).

FTR, I intend to implement something alike:

	outer_th_offset >= 14 + base outer network hdr size
	outer_th_offset + 8 <= inner_nh_offset
	inner_nh_offset + base inner network hdr size  <= csum_start
	csum_start <= pkt_len - base inner transport hdr size

where base inner/outer network hdr size is 20 or 40 depending on ipv4 vs 
ipv6 and base inner transport hdr size is 8 or 20 debending on udp vs tcp.

> Not sure that we have to call them out in the spec, but perhaps
> helpful too. Either way, good to think about possible attacks on
> the new interface even before coding, before the spec is complete.

AFIACS the current spec currently does not describe validation for the 
existing fields subject to such issues (e.g. csum_start/csum_offset). I 
think it could be added in a second moment, if needed.

Thanks,

Paolo