On Fri, Feb 19, 2021 at 3:53 AM Jason Wang <jasowang@xxxxxxxxxx> wrote:
It is? All other GSO packets are generated by the stack itself, either
On 2021/2/18 11:50 下午, Willem de Bruijn wrote:
On Thu, Feb 18, 2021 at 10:01 AM Balazs Nemeth <bnemeth@xxxxxxxxxx> wrote:
For gso packets, virtio_net_hdr_set_proto sets the protocol (if it isn't
set) based on the type in the virtio net hdr, but the skb could contain
anything since it could come from packet_snd through a raw socket. If
there is a mismatch between what virtio_net_hdr_set_proto sets and
the actual protocol, then the skb could be handled incorrectly later
on by gso.
The network header of gso packets starts at 14 bytes, but a specially
crafted packet could fool the call to skb_flow_dissect_flow_keys_basic
as the network header offset in the skb could be incorrect.
Consequently, EINVAL is not returned.
There are even packets that can cause an infinite loop. For example, a
packet with ethernet type ETH_P_MPLS_UC (which is unnoticed by
virtio_net_hdr_to_skb) that is sent to a geneve interface will be
handled by geneve_build_skb. In turn, it calls
udp_tunnel_handle_offloads which then calls skb_reset_inner_headers.
After that, the packet gets passed to mpls_gso_segment. That function
calculates the mpls header length by taking the difference between
network_header and inner_network_header. Since the two are equal
(due to the earlier call to skb_reset_inner_headers), it will calculate
a header of length 0, and it will not pull any headers. Then, it will
call skb_mac_gso_segment which will again call mpls_gso_segment, etc...
This leads to the infinite loop.
I remember kernel will validate dodgy gso packets in gso ops. I wonder
why not do the check there? The reason is that virtio/TUN is not the
only source for those packets.
locally or through GRO.
But indeed some checks are better performed in the GSO layer. Such as
likely the 0-byte mpls header length.
If we cannot trust virtio_net_hdr.gso_type passed from userspace, then
we can also not trust the eth.h_proto coming from the same source.
But
it makes sense to require them to be consistent. There is a
dev_parse_header_protocol that may return the link layer type in a
more generic fashion than casting to skb_eth_hdr.
Question remains what to do for the link layer types that do not implement
header_ops->parse_protocol, and so we cannot validate the packet's
network protocol. Drop will cause false positives, accepts will leave a
potential path, just closes it for Ethernet.
This might call for multiple fixes, both on first ingest and inside the stack?