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 <redacted>
---
include/linux/virtio_net.h | 7 ++++++-
1 file changed, 6 insertions(+), 1 deletion(-)
From: Willem de Bruijn <willemdebruijn.kernel@gmail.com> Date: 2021-02-18 18:19:00
On Thu, Feb 18, 2021 at 10:01 AM Balazs Nemeth [off-list ref] wrote:
quoted hunk
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 <redacted>
---
include/linux/virtio_net.h | 7 ++++++-
1 file changed, 6 insertions(+), 1 deletion(-)
@@ -79,8 +79,13 @@ static inline int virtio_net_hdr_to_skb(struct sk_buff *skb,if(gso_type&&skb->network_header){structflow_keys_basickeys;-if(!skb->protocol)+if(!skb->protocol){+conststructethhdr*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.
From: Jason Wang <hidden> Date: 2021-02-19 08:53:26
On 2021/2/18 11:50 下午, Willem de Bruijn wrote:
On Thu, Feb 18, 2021 at 10:01 AM Balazs Nemeth [off-list ref] wrote:
quoted
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
quoted
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 <redacted>
---
include/linux/virtio_net.h | 7 ++++++-
1 file changed, 6 insertions(+), 1 deletion(-)
@@ -79,8 +79,13 @@ static inline int virtio_net_hdr_to_skb(struct sk_buff *skb,if(gso_type&&skb->network_header){structflow_keys_basickeys;-if(!skb->protocol)+if(!skb->protocol){+conststructethhdr*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.
From: Willem de Bruijn <willemdebruijn.kernel@gmail.com> Date: 2021-02-19 14:57:10
On Fri, Feb 19, 2021 at 3:53 AM Jason Wang [off-list ref] wrote:
On 2021/2/18 11:50 下午, Willem de Bruijn wrote:
quoted
On Thu, Feb 18, 2021 at 10:01 AM Balazs Nemeth [off-list ref] wrote:
quoted
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.
It is? All other GSO packets are generated by the stack itself, either
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?
From: Jason Wang <hidden> Date: 2021-02-22 03:41:08
On 2021/2/19 10:55 下午, Willem de Bruijn wrote:
On Fri, Feb 19, 2021 at 3:53 AM Jason Wang [off-list ref] wrote:
quoted
On 2021/2/18 11:50 下午, Willem de Bruijn wrote:
quoted
On Thu, Feb 18, 2021 at 10:01 AM Balazs Nemeth [off-list ref] wrote:
quoted
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.
It is? All other GSO packets are generated by the stack itself, either
locally or through GRO.
Something like what has been done in tcp_tso_segment()?
if (skb_gso_ok(skb, features | NETIF_F_GSO_ROBUST)) {
/* Packet is from an untrusted source, reset gso_segs. */
skb_shinfo(skb)->gso_segs = DIV_ROUND_UP(skb->len, mss);
segs = NULL;
goto out;
}
My understanding of the header check logic is that it tries to dealy the
check as much as possible, so for device that has GRO_ROBUST, there's
even no need to do that.
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.
I agree.
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?
It's a balance between performance and security. Ideally, it looks to me
the GSO codes should not assume the header of dodgy packet is correct
which means it must validate them before using them. I'm not sure if it
needs a lot of changes or not.
For security reason, it's better to do a strict check during first
ingest. But it bascially suppress the meaning of NETIF_F_GSO_ROBUST
somehow. And it needs some benchmark to see if it can cause obvious
performance regression.
Thanks
On Mon, 2021-02-22 at 11:39 +0800, Jason Wang wrote:
On 2021/2/19 10:55 下午, Willem de Bruijn wrote:
quoted
On Fri, Feb 19, 2021 at 3:53 AM Jason Wang [off-list ref]
wrote:
quoted
On 2021/2/18 11:50 下午, Willem de Bruijn wrote:
quoted
On Thu, Feb 18, 2021 at 10:01 AM Balazs Nemeth <
bnemeth@redhat.com> wrote:
quoted
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.
It is? All other GSO packets are generated by the stack itself,
either
locally or through GRO.
Something like what has been done in tcp_tso_segment()?
if (skb_gso_ok(skb, features | NETIF_F_GSO_ROBUST)) {
/* Packet is from an untrusted source, reset
gso_segs. */
skb_shinfo(skb)->gso_segs = DIV_ROUND_UP(skb->len, mss);
segs = NULL;
goto out;
}
My understanding of the header check logic is that it tries to dealy
the
check as much as possible, so for device that has GRO_ROBUST, there's
even no need to do that.
quoted
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.
I agree.
I'll add a check in the GSO layer as well.
quoted
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?
Given that this is related to dodgy packets and that we can't trust
eth.h_proto, wouldn't it make sense to always drop packets (with
potential false positives), erring on the side of caution, if
header_ops->parse_protocol isn't implemented for the dev in question?
It's a balance between performance and security. Ideally, it looks to
me
the GSO codes should not assume the header of dodgy packet is correct
which means it must validate them before using them. I'm not sure if
it
needs a lot of changes or not.
For security reason, it's better to do a strict check during first
ingest. But it bascially suppress the meaning of NETIF_F_GSO_ROBUST
somehow. And it needs some benchmark to see if it can cause obvious
performance regression.
Thanks
From: Willem de Bruijn <willemdebruijn.kernel@gmail.com> Date: 2021-02-23 14:24:40
On Tue, Feb 23, 2021 at 8:48 AM Balazs Nemeth [off-list ref] wrote:
On Mon, 2021-02-22 at 11:39 +0800, Jason Wang wrote:
quoted
On 2021/2/19 10:55 下午, Willem de Bruijn wrote:
quoted
On Fri, Feb 19, 2021 at 3:53 AM Jason Wang [off-list ref]
wrote:
quoted
On 2021/2/18 11:50 下午, Willem de Bruijn wrote:
quoted
On Thu, Feb 18, 2021 at 10:01 AM Balazs Nemeth <
bnemeth@redhat.com> wrote:
quoted
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.
It is? All other GSO packets are generated by the stack itself,
either
locally or through GRO.
Something like what has been done in tcp_tso_segment()?
if (skb_gso_ok(skb, features | NETIF_F_GSO_ROBUST)) {
/* Packet is from an untrusted source, reset
gso_segs. */
skb_shinfo(skb)->gso_segs = DIV_ROUND_UP(skb->len, mss);
segs = NULL;
goto out;
}
My understanding of the header check logic is that it tries to dealy
the
check as much as possible, so for device that has GRO_ROBUST, there's
even no need to do that.
quoted
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.
I agree.
I'll add a check in the GSO layer as well.
quoted
quoted
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?
Given that this is related to dodgy packets and that we can't trust
eth.h_proto, wouldn't it make sense to always drop packets (with
potential false positives), erring on the side of caution, if
header_ops->parse_protocol isn't implemented for the dev in question?
Unfortunately, that might break applications somewhere out there.