net: prevent infinite loop caused by incorrect proto from virtio_net_hdr_set_proto
Subsystems (4)
v2 cover net: prevent infinite loop caused by incorrect proto from virtio_net_hdr_set_proto
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
PATCH v2 1/2 net: check if protocol extracted by virtio_net_hdr_set_proto is correct
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.
--- include/linux/virtio_net.h | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-)
include/linux/virtio_net.h +7 -1
@@ -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; + } retry: if (!skb_flow_dissect_flow_keys_basic(NULL, skb, &keys, NULL, 0, 0, 0,
Signature · 1 line
2.29.2
PATCH v2 2/2 net: avoid infinite loop in mpls_gso_segment when mpls_hlen == 0
In reply to Balazs NemethA packet with skb_inner_network_header(skb) == skb_network_header(skb) and ETH_P_MPLS_UC will prevent mpls_gso_segment from pulling any headers from the packet. Subsequently, the call to skb_mac_gso_segment will again call mpls_gso_segment with the same packet leading to an infinite loop.
--- net/mpls/mpls_gso.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
net/mpls/mpls_gso.c +1 -1
@@ -27,7 +27,7 @@ static struct sk_buff *mpls_gso_segment(struct sk_buff *skb,skb_reset_network_header (skb); mpls_hlen =skb_inner_network_header (skb) -skb_network_header (skb); -if (unlikely (!pskb_may_pull (skb, mpls_hlen))) +if (unlikely (!mpls_hlen || !pskb_may_pull (skb, mpls_hlen))) goto out;/* Setup inner SKB. */
Signature · 1 line
2.29.2
Re: [PATCH v2 1/2] net: check if protocol extracted by virtio_net_hdr_set_proto is correct
In reply to Balazs NemethQuoted 20 lines · Balazs Nemeth
For gso packets,
virtio_net_hdr_set_protosets 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 frompacket_sndthrough a raw socket. If there is a mismatch between whatvirtio_net_hdr_set_protosets 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_basicwhich relies onskb->protocolbeing set correctly. A specially crafted packet could foolskb_flow_dissect_flow_keys_basicpreventing 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 ifskb->devimplementsheader_ops->parse_protocol, packets from devices without the implementation are not checked at this stage.
Quoted 2 lines
Fixes 9274124f023b ("net: stricter validation of untrusted gso packets")Signed-off-by Balazs Nemeth <[email protected]>
Going forward, please mark your the patch as targeting the net tree using [PATCH net]
Quoted 9 lines
--- include/linux/virtio_net.h | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-)include/linux/virtio_net.h +0
-0
include/linux/virtio_net.h static inline int virtio_net_hdr_to_skb(struct sk_buff *skb, · line 83
Quoted 6 lines
include/linux/virtio_net.h +2
-1@@ -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);
eth is no longer used.
include/linux/virtio_net.h static inline int virtio_net_hdr_to_skb(struct sk_buff *skb, · line 84
Quoted 1 line
include/linux/virtio_net.h +1
-0+__be16 etype =dev_parse_header_protocol (skb);
nit: customary to call this protocol. etype, I guess short for EtherType, makes sense, but is not commonly used in the kernel.
Quoted 11 lines
include/linux/virtio_net.h +4
-1+virtio_net_hdr_set_proto (skb, hdr); +if (etype && etype != skb->protocol) +return -EINVAL ; + } retry: if (!skb_flow_dissect_flow_keys_basic(NULL, skb, &keys, NULL, 0, 0, 0, --2.29.2
_______________________________________________
Virtualization mailing list
[email protected]
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v2 2/2] net: avoid infinite loop in mpls_gso_segment when mpls_hlen == 0
In reply to Balazs NemethQuoted 17 lines · Balazs Nemeth
A packet with
skb_inner_network_header(skb)==skb_network_header(skb)andETH_P_MPLS_UCwill preventmpls_gso_segmentfrom pulling any headers from the packet. Subsequently, the call toskb_mac_gso_segmentwill again callmpls_gso_segmentwith the same packet leading to an infinite loop.Signed-off-by Balazs Nemeth <[email protected]>--- net/mpls/mpls_gso.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)net/mpls/mpls_gso.c +0
-0
net/mpls/mpls_gso.c static struct sk_buff *mpls_gso_segment(struct sk_buff *skb, · line 31
Quoted 6 lines
net/mpls/mpls_gso.c +1
-1@@ -27,7 +27,7 @@ static struct sk_buff *mpls_gso_segment(struct sk_buff *skb,skb_reset_network_header (skb); mpls_hlen =skb_inner_network_header (skb) -skb_network_header (skb); -if (unlikely (!pskb_may_pull (skb, mpls_hlen))) +if (unlikely (!mpls_hlen || !pskb_may_pull (skb, mpls_hlen))) goto out;
Good cathc. Besides length zero, this can be more strict: a label is 4B, so mpls_hlen needs to be >= 4B.
Perhaps even aligned to 4B, too, but not if there may be other encap on top.
Unfortunately there is no struct or type definition that we can use a sizeof instead of open coding the raw constant.
_______________________________________________
Virtualization mailing list [email protected]
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v2 2/2] net: avoid infinite loop in mpls_gso_segment when mpls_hlen == 0
Quoted 15 lines · Willem de Bruijn
net/mpls/mpls_gso.c static struct sk_buff *mpls_gso_segment(struct sk_buff *skb, · line 31
Quoted 11 lines
net/mpls/mpls_gso.c +1
-1@@ -27,7 +27,7 @@ static struct sk_buff *mpls_gso_segment(struct sk_buff *skb,skb_reset_network_header (skb); mpls_hlen =skb_inner_network_header (skb) -skb_network_header (skb); -if (unlikely (!pskb_may_pull (skb, mpls_hlen))) +if (unlikely (!mpls_hlen || !pskb_may_pull (skb, mpls_hlen))) goto out;Good cathc. Besides length zero, this can be more strict: a label is 4B, so
mpls_hlenneeds to be >= 4B.
Quoted 5 lines
Perhaps even aligned to 4B, too, but not if there may be other encap on top.
Unfortunately there is no struct or type definition that we can use a sizeof instead of open coding the raw constant.
MPLS_HLEN can be used here.
_______________________________________________
Virtualization mailing list
[email protected]
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v2 2/2] net: avoid infinite loop in mpls_gso_segment when mpls_hlen == 0
Quoted 23 lines · David Ahern
Quoted 22 lines · Willem de Bruijn
Quoted 8 lines
net/mpls/mpls_gso.c +0
-0@@ -27,7 +27,7 @@ static struct sk_buff *mpls_gso_segment(structsk_buff *skb, skb_reset_network_header(skb);Quoted 6 lines
mpls_hlen=skb_inner_network_header(skb)-skb_network_header(skb);
- if (unlikely(!pskb_may_pull(skb, mpls_hlen))) + if (unlikely(!mpls_hlen || !pskb_may_pull(skb, mpls_hlen)))
goto out;Good cathc. Besides length zero, this can be more strict: a label is
4B, sompls_hlenneeds to be >= 4B.Perhaps even aligned to 4B, too, but not if there may be other encap on top.
Quoted 7 lines
Quoted 4 lines
Unfortunately there is no struct or type definition that we can use a
sizeof instead of open coding the raw constant.
MPLS_HLENcan be used here.
What about sizeof(struct mpls_label), like in net/ipv4/tunnel4.c?
Re: [PATCH v2 2/2] net: avoid infinite loop in mpls_gso_segment when mpls_hlen == 0
Quoted 29 lines · Balazs Nemeth
Quoted 28 lines · David Ahern
Quoted 26 lines · Willem de Bruijn
Quoted 8 lines
net/mpls/mpls_gso.c +0
-0@@ -27,7 +27,7 @@ static struct sk_buff *mpls_gso_segment(structsk_buff *skb, skb_reset_network_header(skb);Quoted 6 lines
mpls_hlen=skb_inner_network_header(skb)-skb_network_header(skb);
- if (unlikely(!pskb_may_pull(skb, mpls_hlen))) + if (unlikely(!mpls_hlen || !pskb_may_pull(skb, mpls_hlen)))
goto out;Good cathc. Besides length zero, this can be more strict: a label is
4B, sompls_hlenneeds to be >= 4B.Perhaps even aligned to 4B, too, but not if there may be other encap on top.
Unfortunately there is no struct or type definition that we can use a
sizeof instead of open coding the raw constant.
Quoted 5 lines
Quoted 2 lines
MPLS_HLENcan be used here.What about
sizeof(struct mpls_label), like innet/ipv4/tunnel4.c?
I was thinking MPLS_HLEN because of its consistent use with skb manipulations. net/mpls code uses mpls_shim_hdr over mpls_label.
Looks like the MPLS code could use some cleanups to make this consistent.
_______________________________________________
Virtualization mailing list [email protected]
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v2 2/2] net: avoid infinite loop in mpls_gso_segment when mpls_hlen == 0
Quoted 23 lines · David Ahern
Quoted 21 lines · Balazs Nemeth
Quoted 20 lines · David Ahern
Quoted 19 lines · Willem de Bruijn
Quoted 13 lines
net/mpls/mpls_gso.c +1
-1@@ -27,7 +27,7 @@ static struct sk_buff *mpls_gso_segment(structsk_buff *skb,skb_reset_network_header (skb ); mpls_hlen =skb_inner_network_header (skb) -skb_network_header (skb); -if (unlikely (!pskb_may_pull (skb, mpls_hlen))) +if (unlikely (!mpls_hlen || !pskb_may_pull (skb, mpls_hlen)))Quoted 1 line
goto out;Good cathc. Besides length zero, this can be more strict: a label is
4B, sompls_hlenneeds to be >= 4B.
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Perhaps even aligned to 4B, too, but not if there may be other encap on top.
On second thought, since mpls_gso_segment pulls all these headers, it is correct to require it to be a multiple of MPLS_HLEN.
_______________________________________________
Virtualization mailing list [email protected]
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Michael S. Tsirkin
maintainerRe: [PATCH v2 1/2] net: check if protocol extracted by virtio_net_hdr_set_proto is correct
In reply to Balazs NemethQuoted 32 lines · Balazs Nemeth
For gso packets,
virtio_net_hdr_set_protosets 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 frompacket_sndthrough a raw socket. If there is a mismatch between whatvirtio_net_hdr_set_protosets 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_basicwhich relies onskb->protocolbeing set correctly. A specially crafted packet could foolskb_flow_dissect_flow_keys_basicpreventing 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 ifskb->devimplementsheader_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 <[email protected]>--- include/linux/virtio_net.h | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-)include/linux/virtio_net.h +0
-0@@ -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;
include/linux/virtio_net.h static inline int virtio_net_hdr_to_skb(struct sk_buff *skb, · line 89
Quoted 10 lines
include/linux/virtio_net.h +7
-1-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?
Quoted 5 lines
include/linux/virtio_net.h +0
-0retry: if (!skb_flow_dissect_flow_keys_basic(NULL, skb, &keys, NULL, 0, 0, 0,Signature · 1 line
2.29.2
_______________________________________________
Virtualization mailing list
[email protected]
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v2 1/2] net: check if protocol extracted by virtio_net_hdr_set_proto is correct
Quoted 35 lines · Michael S. Tsirkin
Quoted 33 lines · Balazs Nemeth
For gso packets,
virtio_net_hdr_set_protosets 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 frompacket_sndthrough a raw socket. If there is a mismatch between whatvirtio_net_hdr_set_protosets 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_basicwhich relies onskb->protocolbeing set correctly. A specially crafted packet could foolskb_flow_dissect_flow_keys_basicpreventing 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 ifskb->devimplementsheader_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 <[email protected]>--- include/linux/virtio_net.h | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-)include/linux/virtio_net.h +0
-0@@ -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;
Quoted 14 lines
include/linux/virtio_net.h static inline int virtio_net_hdr_to_skb(struct sk_buff *skb, · line 89
Quoted 9 lines
include/linux/virtio_net.h +7
-1-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).
_______________________________________________
Virtualization mailing list [email protected]
https://lists.linuxfoundation.org/mailman/listinfo/virtualization