Willem de Bruijn
2021-Feb-18 15:50 UTC
[PATCH] net: check if protocol extracted by virtio_net_hdr_set_proto is correct
On Thu, Feb 18, 2021 at 10:01 AM Balazs Nemeth <bnemeth at redhat.com> 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. > > For that reason, address the root cause of the issue: don't blindly > trust the information provided by the virtio net header. Instead, > check if the protocol in the packet actually matches the protocol set by > virtio_net_hdr_set_proto. > > Fixes: 9274124f023b ("net: stricter validation of untrusted gso packets") > Signed-off-by: Balazs Nemeth <bnemeth at redhat.com> > --- > include/linux/virtio_net.h | 7 ++++++- > 1 file changed, 6 insertions(+), 1 deletion(-) > > diff --git a/include/linux/virtio_net.h b/include/linux/virtio_net.h > index e8a924eeea3d..cf2c53563f22 100644 > --- a/include/linux/virtio_net.h > +++ b/include/linux/virtio_net.h > @@ -79,8 +79,13 @@ static inline int virtio_net_hdr_to_skb(struct sk_buff *skb, > if (gso_type && skb->network_header) { > struct flow_keys_basic keys; > > - if (!skb->protocol) > + if (!skb->protocol) { > + const struct ethhdr *eth = skb_eth_hdr(skb); > +Unfortunately, cannot assume that the device type is ARPHRD_ETHER. The underlying approach is sound: packets that have a gso type set in the virtio_net_hdr have to be IP packets.> virtio_net_hdr_set_proto(skb, hdr); > + if (skb->protocol != eth->h_proto) > + return -EINVAL; > + } > retry: > if (!skb_flow_dissect_flow_keys_basic(NULL, skb, &keys, > NULL, 0, 0, 0, > -- > 2.29.2 >
Jason Wang
2021-Feb-19 08:51 UTC
[PATCH] net: check if protocol extracted by virtio_net_hdr_set_proto is correct
On 2021/2/18 11:50 ??, Willem de Bruijn wrote:> On Thu, Feb 18, 2021 at 10:01 AM Balazs Nemeth <bnemeth at redhat.com> 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. Thanks>> >> For that reason, address the root cause of the issue: don't blindly >> trust the information provided by the virtio net header. Instead, >> check if the protocol in the packet actually matches the protocol set by >> virtio_net_hdr_set_proto. >> >> Fixes: 9274124f023b ("net: stricter validation of untrusted gso packets") >> Signed-off-by: Balazs Nemeth <bnemeth at redhat.com> >> --- >> include/linux/virtio_net.h | 7 ++++++- >> 1 file changed, 6 insertions(+), 1 deletion(-) >> >> diff --git a/include/linux/virtio_net.h b/include/linux/virtio_net.h >> index e8a924eeea3d..cf2c53563f22 100644 >> --- a/include/linux/virtio_net.h >> +++ b/include/linux/virtio_net.h >> @@ -79,8 +79,13 @@ static inline int virtio_net_hdr_to_skb(struct sk_buff *skb, >> if (gso_type && skb->network_header) { >> struct flow_keys_basic keys; >> >> - if (!skb->protocol) >> + if (!skb->protocol) { >> + const struct ethhdr *eth = skb_eth_hdr(skb); >> + > Unfortunately, cannot assume that the device type is ARPHRD_ETHER. > > The underlying approach is sound: packets that have a gso type set in > the virtio_net_hdr have to be IP packets. > >> virtio_net_hdr_set_proto(skb, hdr); >> + if (skb->protocol != eth->h_proto) >> + return -EINVAL; >> + } >> retry: >> if (!skb_flow_dissect_flow_keys_basic(NULL, skb, &keys, >> NULL, 0, 0, 0, >> -- >> 2.29.2 >>