| Message ID | cover.1615199056.git.bnemeth@redhat.com |
|---|---|
| Headers | show |
| Series | net: prevent infinite loop caused by incorrect proto from virtio_net_hdr_set_proto | expand |
On Mon, Mar 08, 2021 at 11:31:25AM +0100, Balazs Nemeth 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. > > An example where this poses an issue is with the subsequent call to > skb_flow_dissect_flow_keys_basic which relies on skb->protocol being set > correctly. A specially crafted packet could fool > skb_flow_dissect_flow_keys_basic preventing EINVAL to be returned. > > Avoid blindly trusting the information provided by the virtio net header > by checking that the protocol in the packet actually matches the > protocol set by virtio_net_hdr_set_proto. Note that since the protocol > is only checked if skb->dev implements header_ops->parse_protocol, > packets from devices without the implementation are not checked at this > stage. > > Fixes: 9274124f023b ("net: stricter validation of untrusted gso packets") > Signed-off-by: Balazs Nemeth <bnemeth@redhat.com> > --- > include/linux/virtio_net.h | 8 +++++++- > 1 file changed, 7 insertions(+), 1 deletion(-) > > diff --git a/include/linux/virtio_net.h b/include/linux/virtio_net.h > index e8a924eeea3d..6c478eee0452 100644 > --- a/include/linux/virtio_net.h > +++ b/include/linux/virtio_net.h > @@ -79,8 +79,14 @@ 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); > + __be16 etype = dev_parse_header_protocol(skb); > + > virtio_net_hdr_set_proto(skb, hdr); > + if (etype && etype != skb->protocol) > + return -EINVAL; > + } Well the protocol in the header is an attempt at an optimization to remove need to parse the packet ... any data on whether this affecs performance? > retry: > if (!skb_flow_dissect_flow_keys_basic(NULL, skb, &keys, > NULL, 0, 0, 0, > -- > 2.29.2
On Tue, Mar 9, 2021 at 6:26 AM Michael S. Tsirkin <mst@redhat.com> wrote: > > On Mon, Mar 08, 2021 at 11:31:25AM +0100, Balazs Nemeth 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. > > > > An example where this poses an issue is with the subsequent call to > > skb_flow_dissect_flow_keys_basic which relies on skb->protocol being set > > correctly. A specially crafted packet could fool > > skb_flow_dissect_flow_keys_basic preventing EINVAL to be returned. > > > > Avoid blindly trusting the information provided by the virtio net header > > by checking that the protocol in the packet actually matches the > > protocol set by virtio_net_hdr_set_proto. Note that since the protocol > > is only checked if skb->dev implements header_ops->parse_protocol, > > packets from devices without the implementation are not checked at this > > stage. > > > > Fixes: 9274124f023b ("net: stricter validation of untrusted gso packets") > > Signed-off-by: Balazs Nemeth <bnemeth@redhat.com> > > --- > > include/linux/virtio_net.h | 8 +++++++- > > 1 file changed, 7 insertions(+), 1 deletion(-) > > > > diff --git a/include/linux/virtio_net.h b/include/linux/virtio_net.h > > index e8a924eeea3d..6c478eee0452 100644 > > --- a/include/linux/virtio_net.h > > +++ b/include/linux/virtio_net.h > > @@ -79,8 +79,14 @@ 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); > > + __be16 etype = dev_parse_header_protocol(skb); > > + > > virtio_net_hdr_set_proto(skb, hdr); > > + if (etype && etype != skb->protocol) > > + return -EINVAL; > > + } > > > Well the protocol in the header is an attempt at an optimization to > remove need to parse the packet ... any data on whether this > affecs performance? This adds a branch and reading a cacheline that is inevitably read not much later. It shouldn't be significant. And this branch is only taken if skb->protocol is not set. So the cost can easily be avoided by passing the information. But you raise a good point, because TUNTAP does set it, but only after the call to virtio_net_hdr_to_skb. That should perhaps be inverted (in a separate net-next patch).
Here is v2 of the patches that prevent an infinite loop for gso packets with a protocol from virtio net hdr that doesn't match the protocol in the packet. Note that packets coming from a device without header_ops->parse_protocol being implemented will not be caught by the check in virtio_net_hdr_to_skb, but the infinite loop will still be prevented by the check in the gso layer. Balazs Nemeth (2): net: check if protocol extracted by virtio_net_hdr_set_proto is correct net: avoid infinite loop in mpls_gso_segment when mpls_hlen == 0 include/linux/virtio_net.h | 8 +++++++- net/mpls/mpls_gso.c | 2 +- 2 files changed, 8 insertions(+), 2 deletions(-) -- 2.29.2