Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Export standard sFlow LAG, PORTNAME and OPENFLOWPORT structures #23

Closed
wants to merge 1 commit into from
Closed

Conversation

sflow
Copy link
Contributor

@sflow sflow commented Nov 7, 2014

Export standard sFlow LAG, PORTNAME and OPENFLOWPORT structures with
each counter-sample. Add unit-test for sFlow-LAG. Adjust other unit-tests to
accommodate these new annotations.

The sFlow-LAG structures are important for topology discovery, for
troubleshooting LAG instability, and for correctly combining
sFlow feeds from multiple sources.

The OPENFLOWPORT and PORTNAME structures are important for systems that
aim to combine sFlow monitoring with OpenFlow controls, as they
provide straightforward mapping (1) between sFlow agent IP and OpenFlow
datapath-id, and (2) between interface name,ifIndex and OpenFlow
port number.

Signed-off-by: Neil McKee [email protected]

…each

counter-sample. Add unit-test for sFlow-LAG. Adjust other unit-tests to
accommodate these new annotations.

The sFlow-LAG structures are important for topology discovery, for
troubleshooting LAG instability,  and for correctly combining
sFlow feeds from multiple sources.

The OPENFLOWPORT and PORTNAME structures are important for systems that
aim to combine sFlow monitoring with OpenFlow controls,  as they
provide straightforward mapping (1) between sFlow agent IP and OpenFlow
datapath-id,  and (2) between interface name,ifIndex and OpenFlow
port number.

Signed-off-by: Neil McKee <[email protected]>
@blp
Copy link
Contributor

blp commented Nov 11, 2014

Applied, thansk.

@blp blp closed this Nov 11, 2014
zhanghaooo pushed a commit to zhanghaooo/ovs that referenced this pull request Jan 25, 2019
ovsrobot pushed a commit to ovsrobot/ovs that referenced this pull request Oct 12, 2021
IP fragmentation engine may not only steal the packet but also add
more.  For example, after receiving the last fragment, it will
add all previous fragments to a batch.  Unfortunately, it will also
free the original last fragment and replace it with a copy.
This invalidates the 'packet_clone' pointer in the dpif_netdev_execute()
leading to the use-after-free:

==3525086==ERROR: AddressSanitizer: heap-use-after-free on
                  address 0x61600020439c at pc 0x000000688a6d
READ of size 1 at 0x61600020439c thread T0
    #0 0x688a6c in dp_packet_swap ./lib/dp-packet.h:265:5
    openvswitch#1 0x68781d in dpif_netdev_execute lib/dpif-netdev.c:4103:9
    openvswitch#2 0x6675db in dpif_netdev_operate lib/dpif-netdev.c:4129:25
    openvswitch#3 0x691e5e in dpif_operate lib/dpif.c:1367:13
    #4 0x692909 in dpif_execute lib/dpif.c:1321:9
    #5 0x5b19c6 in packet_execute ofproto/ofproto-dpif.c:4991:5
    openvswitch#6 0x5a2861 in ofproto_packet_out_finish ofproto/ofproto.c:3662:5
    #7 0x5a65c6 in do_bundle_commit ofproto/ofproto.c:8270:13
    #8 0x5a0cae in handle_bundle_control ofproto/ofproto.c:8309:17
    #9 0x59a476 in handle_single_part_openflow ofproto/ofproto.c:8593:16
    #10 0x5877ac in handle_openflow ofproto/ofproto.c:8674:21
    #11 0x6296f1 in ofconn_run ofproto/connmgr.c:1329:13
    #12 0x62925d in connmgr_run ofproto/connmgr.c:356:9
    #13 0x586904 in ofproto_run ofproto/ofproto.c:1879:5
    #14 0x55c830 in bridge_run__ vswitchd/bridge.c:3251:9
    #15 0x55c015 in bridge_run vswitchd/bridge.c:3310:5
    openvswitch#16 0x575f31 in main vswitchd/ovs-vswitchd.c:127:9
    #17 0x7f01099d3492 in __libc_start_main (/lib64/libc.so.6+0x23492)
    openvswitch#18 0x47d96d in _start (vswitchd/ovs-vswitchd+0x47d96d)

0x61600020439c is located 28 bytes inside of 560-byte region
freed by thread T0 here:
    #0 0x5177a8 in free (vswitchd/ovs-vswitchd+0x5177a8)
    openvswitch#1 0x6b17b6 in dp_packet_delete ./lib/dp-packet.h:256:9
    openvswitch#2 0x6afeee in ipf_extract_frags_from_batch lib/ipf.c:947:17
    openvswitch#3 0x6afd63 in ipf_preprocess_conntrack lib/ipf.c:1232:9
    #4 0x946b2c in conntrack_execute lib/conntrack.c:1446:5
    #5 0x67e3ed in dp_execute_cb lib/dpif-netdev.c:8277:9
    openvswitch#6 0x7097d7 in odp_execute_actions lib/odp-execute.c:865:17
    #7 0x66409e in dp_netdev_execute_actions lib/dpif-netdev.c:8322:5
    #8 0x6877ad in dpif_netdev_execute lib/dpif-netdev.c:4090:5
    #9 0x6675db in dpif_netdev_operate lib/dpif-netdev.c:4129:25
    #10 0x691e5e in dpif_operate lib/dpif.c:1367:13
    #11 0x692909 in dpif_execute lib/dpif.c:1321:9
    #12 0x5b19c6 in packet_execute ofproto/ofproto-dpif.c:4991:5
    #13 0x5a2861 in ofproto_packet_out_finish ofproto/ofproto.c:3662:5
    #14 0x5a65c6 in do_bundle_commit ofproto/ofproto.c:8270:13
    #15 0x5a0cae in handle_bundle_control ofproto/ofproto.c:8309:17
    openvswitch#16 0x59a476 in handle_single_part_openflow ofproto/ofproto.c:8593:16
    #17 0x5877ac in handle_openflow ofproto/ofproto.c:8674:21
    openvswitch#18 0x6296f1 in ofconn_run ofproto/connmgr.c:1329:13
    #19 0x62925d in connmgr_run ofproto/connmgr.c:356:9
    #20 0x586904 in ofproto_run ofproto/ofproto.c:1879:5
    #21 0x55c830 in bridge_run__ vswitchd/bridge.c:3251:9
    openvswitch#22 0x55c015 in bridge_run vswitchd/bridge.c:3310:5
    openvswitch#23 0x575f31 in main vswitchd/ovs-vswitchd.c:127:9
    #24 0x7f01099d3492 in __libc_start_main (/lib64/libc.so.6+0x23492)

The issue can be reproduced with system-userspace testsuite on the
'conntrack - IPv4 fragmentation with fragments specified' test.
Previously, there was a leak inside the IP fragmentation module that
kept the original segment, so 'packet_clone' remained a valid pointer.
But commit 803ed12 ("ipf: release unhandled packets from the batch")
fixed the leak leading to use-after-free.

Using the packet from a batch instead of 'packet_clone' to swap packet
content to avoid the issue.

While investigating this problem, more issues uncovered.  One of them
is that IP fragmentation engine can add more packets to the batch, but
there is no way to get them to a caller.  Adding an extra branch for
this case with a 'FIXME' comment in order to highlight the issue.

Another one is that IP fragmentation engine will keep only 32 fragments
dropping all other fragments while refilling a batch, but that should
be fixed separately.

Fixes: 7e6b41a ("dpif-netdev: Fix crash when PACKET_OUT is metered.")
Signed-off-by: Ilya Maximets <[email protected]>
Signed-off-by: 0-day Robot <[email protected]>
igsilya added a commit to igsilya/ovs that referenced this pull request Oct 13, 2021
IP fragmentation engine may not only steal the packet but also add
more.  For example, after receiving the last fragment, it will
add all previous fragments to a batch.  Unfortunately, it will also
free the original last fragment and replace it with a copy.
This invalidates the 'packet_clone' pointer in the dpif_netdev_execute()
leading to the use-after-free:

==3525086==ERROR: AddressSanitizer: heap-use-after-free on
                  address 0x61600020439c at pc 0x000000688a6d
READ of size 1 at 0x61600020439c thread T0
    #0 0x688a6c in dp_packet_swap ./lib/dp-packet.h:265:5
    openvswitch#1 0x68781d in dpif_netdev_execute lib/dpif-netdev.c:4103:9
    openvswitch#2 0x6675db in dpif_netdev_operate lib/dpif-netdev.c:4129:25
    openvswitch#3 0x691e5e in dpif_operate lib/dpif.c:1367:13
    #4 0x692909 in dpif_execute lib/dpif.c:1321:9
    #5 0x5b19c6 in packet_execute ofproto/ofproto-dpif.c:4991:5
    openvswitch#6 0x5a2861 in ofproto_packet_out_finish ofproto/ofproto.c:3662:5
    #7 0x5a65c6 in do_bundle_commit ofproto/ofproto.c:8270:13
    #8 0x5a0cae in handle_bundle_control ofproto/ofproto.c:8309:17
    #9 0x59a476 in handle_single_part_openflow ofproto/ofproto.c:8593:16
    #10 0x5877ac in handle_openflow ofproto/ofproto.c:8674:21
    #11 0x6296f1 in ofconn_run ofproto/connmgr.c:1329:13
    #12 0x62925d in connmgr_run ofproto/connmgr.c:356:9
    #13 0x586904 in ofproto_run ofproto/ofproto.c:1879:5
    #14 0x55c830 in bridge_run__ vswitchd/bridge.c:3251:9
    #15 0x55c015 in bridge_run vswitchd/bridge.c:3310:5
    openvswitch#16 0x575f31 in main vswitchd/ovs-vswitchd.c:127:9
    #17 0x7f01099d3492 in __libc_start_main (/lib64/libc.so.6+0x23492)
    openvswitch#18 0x47d96d in _start (vswitchd/ovs-vswitchd+0x47d96d)

0x61600020439c is located 28 bytes inside of 560-byte region
freed by thread T0 here:
    #0 0x5177a8 in free (vswitchd/ovs-vswitchd+0x5177a8)
    openvswitch#1 0x6b17b6 in dp_packet_delete ./lib/dp-packet.h:256:9
    openvswitch#2 0x6afeee in ipf_extract_frags_from_batch lib/ipf.c:947:17
    openvswitch#3 0x6afd63 in ipf_preprocess_conntrack lib/ipf.c:1232:9
    #4 0x946b2c in conntrack_execute lib/conntrack.c:1446:5
    #5 0x67e3ed in dp_execute_cb lib/dpif-netdev.c:8277:9
    openvswitch#6 0x7097d7 in odp_execute_actions lib/odp-execute.c:865:17
    #7 0x66409e in dp_netdev_execute_actions lib/dpif-netdev.c:8322:5
    #8 0x6877ad in dpif_netdev_execute lib/dpif-netdev.c:4090:5
    #9 0x6675db in dpif_netdev_operate lib/dpif-netdev.c:4129:25
    #10 0x691e5e in dpif_operate lib/dpif.c:1367:13
    #11 0x692909 in dpif_execute lib/dpif.c:1321:9
    #12 0x5b19c6 in packet_execute ofproto/ofproto-dpif.c:4991:5
    #13 0x5a2861 in ofproto_packet_out_finish ofproto/ofproto.c:3662:5
    #14 0x5a65c6 in do_bundle_commit ofproto/ofproto.c:8270:13
    #15 0x5a0cae in handle_bundle_control ofproto/ofproto.c:8309:17
    openvswitch#16 0x59a476 in handle_single_part_openflow ofproto/ofproto.c:8593:16
    #17 0x5877ac in handle_openflow ofproto/ofproto.c:8674:21
    openvswitch#18 0x6296f1 in ofconn_run ofproto/connmgr.c:1329:13
    #19 0x62925d in connmgr_run ofproto/connmgr.c:356:9
    #20 0x586904 in ofproto_run ofproto/ofproto.c:1879:5
    #21 0x55c830 in bridge_run__ vswitchd/bridge.c:3251:9
    openvswitch#22 0x55c015 in bridge_run vswitchd/bridge.c:3310:5
    openvswitch#23 0x575f31 in main vswitchd/ovs-vswitchd.c:127:9
    #24 0x7f01099d3492 in __libc_start_main (/lib64/libc.so.6+0x23492)

The issue can be reproduced with system-userspace testsuite on the
'conntrack - IPv4 fragmentation with fragments specified' test.
Previously, there was a leak inside the IP fragmentation module that
kept the original segment, so 'packet_clone' remained a valid pointer.
But commit 803ed12 ("ipf: release unhandled packets from the batch")
fixed the leak leading to use-after-free.

Using the packet from a batch instead of 'packet_clone' to swap packet
content to avoid the issue.

While investigating this problem, more issues uncovered.  One of them
is that IP fragmentation engine can add more packets to the batch, but
there is no way to get them to a caller.  Adding an extra branch for
this case with a 'FIXME' comment in order to highlight the issue.

Another one is that IP fragmentation engine will keep only 32 fragments
dropping all other fragments while refilling a batch, but that should
be fixed separately.

Fixes: 7e6b41a ("dpif-netdev: Fix crash when PACKET_OUT is metered.")
Signed-off-by: Ilya Maximets <[email protected]>
Acked-by: Aaron Conole <[email protected]>
igsilya added a commit to igsilya/ovs that referenced this pull request Oct 13, 2021
IP fragmentation engine may not only steal the packet but also add
more.  For example, after receiving the last fragment, it will
add all previous fragments to a batch.  Unfortunately, it will also
free the original last fragment and replace it with a copy.
This invalidates the 'packet_clone' pointer in the dpif_netdev_execute()
leading to the use-after-free:

==3525086==ERROR: AddressSanitizer: heap-use-after-free on
                  address 0x61600020439c at pc 0x000000688a6d
READ of size 1 at 0x61600020439c thread T0
    #0 0x688a6c in dp_packet_swap ./lib/dp-packet.h:265:5
    openvswitch#1 0x68781d in dpif_netdev_execute lib/dpif-netdev.c:4103:9
    openvswitch#2 0x6675db in dpif_netdev_operate lib/dpif-netdev.c:4129:25
    openvswitch#3 0x691e5e in dpif_operate lib/dpif.c:1367:13
    #4 0x692909 in dpif_execute lib/dpif.c:1321:9
    #5 0x5b19c6 in packet_execute ofproto/ofproto-dpif.c:4991:5
    openvswitch#6 0x5a2861 in ofproto_packet_out_finish ofproto/ofproto.c:3662:5
    #7 0x5a65c6 in do_bundle_commit ofproto/ofproto.c:8270:13
    #8 0x5a0cae in handle_bundle_control ofproto/ofproto.c:8309:17
    #9 0x59a476 in handle_single_part_openflow ofproto/ofproto.c:8593:16
    #10 0x5877ac in handle_openflow ofproto/ofproto.c:8674:21
    #11 0x6296f1 in ofconn_run ofproto/connmgr.c:1329:13
    #12 0x62925d in connmgr_run ofproto/connmgr.c:356:9
    #13 0x586904 in ofproto_run ofproto/ofproto.c:1879:5
    #14 0x55c830 in bridge_run__ vswitchd/bridge.c:3251:9
    #15 0x55c015 in bridge_run vswitchd/bridge.c:3310:5
    openvswitch#16 0x575f31 in main vswitchd/ovs-vswitchd.c:127:9
    #17 0x7f01099d3492 in __libc_start_main (/lib64/libc.so.6+0x23492)
    openvswitch#18 0x47d96d in _start (vswitchd/ovs-vswitchd+0x47d96d)

0x61600020439c is located 28 bytes inside of 560-byte region
freed by thread T0 here:
    #0 0x5177a8 in free (vswitchd/ovs-vswitchd+0x5177a8)
    openvswitch#1 0x6b17b6 in dp_packet_delete ./lib/dp-packet.h:256:9
    openvswitch#2 0x6afeee in ipf_extract_frags_from_batch lib/ipf.c:947:17
    openvswitch#3 0x6afd63 in ipf_preprocess_conntrack lib/ipf.c:1232:9
    #4 0x946b2c in conntrack_execute lib/conntrack.c:1446:5
    #5 0x67e3ed in dp_execute_cb lib/dpif-netdev.c:8277:9
    openvswitch#6 0x7097d7 in odp_execute_actions lib/odp-execute.c:865:17
    #7 0x66409e in dp_netdev_execute_actions lib/dpif-netdev.c:8322:5
    #8 0x6877ad in dpif_netdev_execute lib/dpif-netdev.c:4090:5
    #9 0x6675db in dpif_netdev_operate lib/dpif-netdev.c:4129:25
    #10 0x691e5e in dpif_operate lib/dpif.c:1367:13
    #11 0x692909 in dpif_execute lib/dpif.c:1321:9
    #12 0x5b19c6 in packet_execute ofproto/ofproto-dpif.c:4991:5
    #13 0x5a2861 in ofproto_packet_out_finish ofproto/ofproto.c:3662:5
    #14 0x5a65c6 in do_bundle_commit ofproto/ofproto.c:8270:13
    #15 0x5a0cae in handle_bundle_control ofproto/ofproto.c:8309:17
    openvswitch#16 0x59a476 in handle_single_part_openflow ofproto/ofproto.c:8593:16
    #17 0x5877ac in handle_openflow ofproto/ofproto.c:8674:21
    openvswitch#18 0x6296f1 in ofconn_run ofproto/connmgr.c:1329:13
    #19 0x62925d in connmgr_run ofproto/connmgr.c:356:9
    #20 0x586904 in ofproto_run ofproto/ofproto.c:1879:5
    #21 0x55c830 in bridge_run__ vswitchd/bridge.c:3251:9
    openvswitch#22 0x55c015 in bridge_run vswitchd/bridge.c:3310:5
    openvswitch#23 0x575f31 in main vswitchd/ovs-vswitchd.c:127:9
    #24 0x7f01099d3492 in __libc_start_main (/lib64/libc.so.6+0x23492)

The issue can be reproduced with system-userspace testsuite on the
'conntrack - IPv4 fragmentation with fragments specified' test.
Previously, there was a leak inside the IP fragmentation module that
kept the original segment, so 'packet_clone' remained a valid pointer.
But commit 803ed12 ("ipf: release unhandled packets from the batch")
fixed the leak leading to use-after-free.

Using the packet from a batch instead of 'packet_clone' to swap packet
content to avoid the issue.

While investigating this problem, more issues uncovered.  One of them
is that IP fragmentation engine can add more packets to the batch, but
there is no way to get them to a caller.  Adding an extra branch for
this case with a 'FIXME' comment in order to highlight the issue.

Another one is that IP fragmentation engine will keep only 32 fragments
dropping all other fragments while refilling a batch, but that should
be fixed separately.

Fixes: 7e6b41a ("dpif-netdev: Fix crash when PACKET_OUT is metered.")
Signed-off-by: Ilya Maximets <[email protected]>
Acked-by: Aaron Conole <[email protected]>
igsilya added a commit to igsilya/ovs that referenced this pull request Oct 13, 2021
IP fragmentation engine may not only steal the packet but also add
more.  For example, after receiving the last fragment, it will
add all previous fragments to a batch.  Unfortunately, it will also
free the original last fragment and replace it with a copy.
This invalidates the 'packet_clone' pointer in the dpif_netdev_execute()
leading to the use-after-free:

==3525086==ERROR: AddressSanitizer: heap-use-after-free on
                  address 0x61600020439c at pc 0x000000688a6d
READ of size 1 at 0x61600020439c thread T0
    #0 0x688a6c in dp_packet_swap ./lib/dp-packet.h:265:5
    openvswitch#1 0x68781d in dpif_netdev_execute lib/dpif-netdev.c:4103:9
    openvswitch#2 0x6675db in dpif_netdev_operate lib/dpif-netdev.c:4129:25
    openvswitch#3 0x691e5e in dpif_operate lib/dpif.c:1367:13
    #4 0x692909 in dpif_execute lib/dpif.c:1321:9
    #5 0x5b19c6 in packet_execute ofproto/ofproto-dpif.c:4991:5
    openvswitch#6 0x5a2861 in ofproto_packet_out_finish ofproto/ofproto.c:3662:5
    #7 0x5a65c6 in do_bundle_commit ofproto/ofproto.c:8270:13
    #8 0x5a0cae in handle_bundle_control ofproto/ofproto.c:8309:17
    #9 0x59a476 in handle_single_part_openflow ofproto/ofproto.c:8593:16
    #10 0x5877ac in handle_openflow ofproto/ofproto.c:8674:21
    #11 0x6296f1 in ofconn_run ofproto/connmgr.c:1329:13
    #12 0x62925d in connmgr_run ofproto/connmgr.c:356:9
    #13 0x586904 in ofproto_run ofproto/ofproto.c:1879:5
    #14 0x55c830 in bridge_run__ vswitchd/bridge.c:3251:9
    #15 0x55c015 in bridge_run vswitchd/bridge.c:3310:5
    openvswitch#16 0x575f31 in main vswitchd/ovs-vswitchd.c:127:9
    #17 0x7f01099d3492 in __libc_start_main (/lib64/libc.so.6+0x23492)
    openvswitch#18 0x47d96d in _start (vswitchd/ovs-vswitchd+0x47d96d)

0x61600020439c is located 28 bytes inside of 560-byte region
freed by thread T0 here:
    #0 0x5177a8 in free (vswitchd/ovs-vswitchd+0x5177a8)
    openvswitch#1 0x6b17b6 in dp_packet_delete ./lib/dp-packet.h:256:9
    openvswitch#2 0x6afeee in ipf_extract_frags_from_batch lib/ipf.c:947:17
    openvswitch#3 0x6afd63 in ipf_preprocess_conntrack lib/ipf.c:1232:9
    #4 0x946b2c in conntrack_execute lib/conntrack.c:1446:5
    #5 0x67e3ed in dp_execute_cb lib/dpif-netdev.c:8277:9
    openvswitch#6 0x7097d7 in odp_execute_actions lib/odp-execute.c:865:17
    #7 0x66409e in dp_netdev_execute_actions lib/dpif-netdev.c:8322:5
    #8 0x6877ad in dpif_netdev_execute lib/dpif-netdev.c:4090:5
    #9 0x6675db in dpif_netdev_operate lib/dpif-netdev.c:4129:25
    #10 0x691e5e in dpif_operate lib/dpif.c:1367:13
    #11 0x692909 in dpif_execute lib/dpif.c:1321:9
    #12 0x5b19c6 in packet_execute ofproto/ofproto-dpif.c:4991:5
    #13 0x5a2861 in ofproto_packet_out_finish ofproto/ofproto.c:3662:5
    #14 0x5a65c6 in do_bundle_commit ofproto/ofproto.c:8270:13
    #15 0x5a0cae in handle_bundle_control ofproto/ofproto.c:8309:17
    openvswitch#16 0x59a476 in handle_single_part_openflow ofproto/ofproto.c:8593:16
    #17 0x5877ac in handle_openflow ofproto/ofproto.c:8674:21
    openvswitch#18 0x6296f1 in ofconn_run ofproto/connmgr.c:1329:13
    #19 0x62925d in connmgr_run ofproto/connmgr.c:356:9
    #20 0x586904 in ofproto_run ofproto/ofproto.c:1879:5
    #21 0x55c830 in bridge_run__ vswitchd/bridge.c:3251:9
    openvswitch#22 0x55c015 in bridge_run vswitchd/bridge.c:3310:5
    openvswitch#23 0x575f31 in main vswitchd/ovs-vswitchd.c:127:9
    #24 0x7f01099d3492 in __libc_start_main (/lib64/libc.so.6+0x23492)

The issue can be reproduced with system-userspace testsuite on the
'conntrack - IPv4 fragmentation with fragments specified' test.
Previously, there was a leak inside the IP fragmentation module that
kept the original segment, so 'packet_clone' remained a valid pointer.
But commit 803ed12 ("ipf: release unhandled packets from the batch")
fixed the leak leading to use-after-free.

Using the packet from a batch instead of 'packet_clone' to swap packet
content to avoid the issue.

While investigating this problem, more issues uncovered.  One of them
is that IP fragmentation engine can add more packets to the batch, but
there is no way to get them to a caller.  Adding an extra branch for
this case with a 'FIXME' comment in order to highlight the issue.

Another one is that IP fragmentation engine will keep only 32 fragments
dropping all other fragments while refilling a batch, but that should
be fixed separately.

Fixes: 7e6b41a ("dpif-netdev: Fix crash when PACKET_OUT is metered.")
Signed-off-by: Ilya Maximets <[email protected]>
Acked-by: Aaron Conole <[email protected]>
igsilya added a commit to igsilya/ovs that referenced this pull request Oct 13, 2021
IP fragmentation engine may not only steal the packet but also add
more.  For example, after receiving the last fragment, it will
add all previous fragments to a batch.  Unfortunately, it will also
free the original last fragment and replace it with a copy.
This invalidates the 'packet_clone' pointer in the dpif_netdev_execute()
leading to the use-after-free:

==3525086==ERROR: AddressSanitizer: heap-use-after-free on
                  address 0x61600020439c at pc 0x000000688a6d
READ of size 1 at 0x61600020439c thread T0
    #0 0x688a6c in dp_packet_swap ./lib/dp-packet.h:265:5
    openvswitch#1 0x68781d in dpif_netdev_execute lib/dpif-netdev.c:4103:9
    openvswitch#2 0x6675db in dpif_netdev_operate lib/dpif-netdev.c:4129:25
    openvswitch#3 0x691e5e in dpif_operate lib/dpif.c:1367:13
    #4 0x692909 in dpif_execute lib/dpif.c:1321:9
    #5 0x5b19c6 in packet_execute ofproto/ofproto-dpif.c:4991:5
    openvswitch#6 0x5a2861 in ofproto_packet_out_finish ofproto/ofproto.c:3662:5
    #7 0x5a65c6 in do_bundle_commit ofproto/ofproto.c:8270:13
    #8 0x5a0cae in handle_bundle_control ofproto/ofproto.c:8309:17
    #9 0x59a476 in handle_single_part_openflow ofproto/ofproto.c:8593:16
    #10 0x5877ac in handle_openflow ofproto/ofproto.c:8674:21
    #11 0x6296f1 in ofconn_run ofproto/connmgr.c:1329:13
    #12 0x62925d in connmgr_run ofproto/connmgr.c:356:9
    #13 0x586904 in ofproto_run ofproto/ofproto.c:1879:5
    #14 0x55c830 in bridge_run__ vswitchd/bridge.c:3251:9
    #15 0x55c015 in bridge_run vswitchd/bridge.c:3310:5
    openvswitch#16 0x575f31 in main vswitchd/ovs-vswitchd.c:127:9
    #17 0x7f01099d3492 in __libc_start_main (/lib64/libc.so.6+0x23492)
    openvswitch#18 0x47d96d in _start (vswitchd/ovs-vswitchd+0x47d96d)

0x61600020439c is located 28 bytes inside of 560-byte region
freed by thread T0 here:
    #0 0x5177a8 in free (vswitchd/ovs-vswitchd+0x5177a8)
    openvswitch#1 0x6b17b6 in dp_packet_delete ./lib/dp-packet.h:256:9
    openvswitch#2 0x6afeee in ipf_extract_frags_from_batch lib/ipf.c:947:17
    openvswitch#3 0x6afd63 in ipf_preprocess_conntrack lib/ipf.c:1232:9
    #4 0x946b2c in conntrack_execute lib/conntrack.c:1446:5
    #5 0x67e3ed in dp_execute_cb lib/dpif-netdev.c:8277:9
    openvswitch#6 0x7097d7 in odp_execute_actions lib/odp-execute.c:865:17
    #7 0x66409e in dp_netdev_execute_actions lib/dpif-netdev.c:8322:5
    #8 0x6877ad in dpif_netdev_execute lib/dpif-netdev.c:4090:5
    #9 0x6675db in dpif_netdev_operate lib/dpif-netdev.c:4129:25
    #10 0x691e5e in dpif_operate lib/dpif.c:1367:13
    #11 0x692909 in dpif_execute lib/dpif.c:1321:9
    #12 0x5b19c6 in packet_execute ofproto/ofproto-dpif.c:4991:5
    #13 0x5a2861 in ofproto_packet_out_finish ofproto/ofproto.c:3662:5
    #14 0x5a65c6 in do_bundle_commit ofproto/ofproto.c:8270:13
    #15 0x5a0cae in handle_bundle_control ofproto/ofproto.c:8309:17
    openvswitch#16 0x59a476 in handle_single_part_openflow ofproto/ofproto.c:8593:16
    #17 0x5877ac in handle_openflow ofproto/ofproto.c:8674:21
    openvswitch#18 0x6296f1 in ofconn_run ofproto/connmgr.c:1329:13
    #19 0x62925d in connmgr_run ofproto/connmgr.c:356:9
    #20 0x586904 in ofproto_run ofproto/ofproto.c:1879:5
    #21 0x55c830 in bridge_run__ vswitchd/bridge.c:3251:9
    openvswitch#22 0x55c015 in bridge_run vswitchd/bridge.c:3310:5
    openvswitch#23 0x575f31 in main vswitchd/ovs-vswitchd.c:127:9
    #24 0x7f01099d3492 in __libc_start_main (/lib64/libc.so.6+0x23492)

The issue can be reproduced with system-userspace testsuite on the
'conntrack - IPv4 fragmentation with fragments specified' test.
Previously, there was a leak inside the IP fragmentation module that
kept the original segment, so 'packet_clone' remained a valid pointer.
But commit 803ed12 ("ipf: release unhandled packets from the batch")
fixed the leak leading to use-after-free.

Using the packet from a batch instead of 'packet_clone' to swap packet
content to avoid the issue.

While investigating this problem, more issues uncovered.  One of them
is that IP fragmentation engine can add more packets to the batch, but
there is no way to get them to a caller.  Adding an extra branch for
this case with a 'FIXME' comment in order to highlight the issue.

Another one is that IP fragmentation engine will keep only 32 fragments
dropping all other fragments while refilling a batch, but that should
be fixed separately.

Fixes: 7e6b41a ("dpif-netdev: Fix crash when PACKET_OUT is metered.")
Signed-off-by: Ilya Maximets <[email protected]>
Acked-by: Aaron Conole <[email protected]>
igsilya added a commit to igsilya/ovs that referenced this pull request Oct 13, 2021
IP fragmentation engine may not only steal the packet but also add
more.  For example, after receiving the last fragment, it will
add all previous fragments to a batch.  Unfortunately, it will also
free the original last fragment and replace it with a copy.
This invalidates the 'packet_clone' pointer in the dpif_netdev_execute()
leading to the use-after-free:

==3525086==ERROR: AddressSanitizer: heap-use-after-free on
                  address 0x61600020439c at pc 0x000000688a6d
READ of size 1 at 0x61600020439c thread T0
    #0 0x688a6c in dp_packet_swap ./lib/dp-packet.h:265:5
    openvswitch#1 0x68781d in dpif_netdev_execute lib/dpif-netdev.c:4103:9
    openvswitch#2 0x6675db in dpif_netdev_operate lib/dpif-netdev.c:4129:25
    openvswitch#3 0x691e5e in dpif_operate lib/dpif.c:1367:13
    #4 0x692909 in dpif_execute lib/dpif.c:1321:9
    #5 0x5b19c6 in packet_execute ofproto/ofproto-dpif.c:4991:5
    openvswitch#6 0x5a2861 in ofproto_packet_out_finish ofproto/ofproto.c:3662:5
    #7 0x5a65c6 in do_bundle_commit ofproto/ofproto.c:8270:13
    #8 0x5a0cae in handle_bundle_control ofproto/ofproto.c:8309:17
    #9 0x59a476 in handle_single_part_openflow ofproto/ofproto.c:8593:16
    #10 0x5877ac in handle_openflow ofproto/ofproto.c:8674:21
    #11 0x6296f1 in ofconn_run ofproto/connmgr.c:1329:13
    #12 0x62925d in connmgr_run ofproto/connmgr.c:356:9
    #13 0x586904 in ofproto_run ofproto/ofproto.c:1879:5
    #14 0x55c830 in bridge_run__ vswitchd/bridge.c:3251:9
    #15 0x55c015 in bridge_run vswitchd/bridge.c:3310:5
    openvswitch#16 0x575f31 in main vswitchd/ovs-vswitchd.c:127:9
    #17 0x7f01099d3492 in __libc_start_main (/lib64/libc.so.6+0x23492)
    openvswitch#18 0x47d96d in _start (vswitchd/ovs-vswitchd+0x47d96d)

0x61600020439c is located 28 bytes inside of 560-byte region
freed by thread T0 here:
    #0 0x5177a8 in free (vswitchd/ovs-vswitchd+0x5177a8)
    openvswitch#1 0x6b17b6 in dp_packet_delete ./lib/dp-packet.h:256:9
    openvswitch#2 0x6afeee in ipf_extract_frags_from_batch lib/ipf.c:947:17
    openvswitch#3 0x6afd63 in ipf_preprocess_conntrack lib/ipf.c:1232:9
    #4 0x946b2c in conntrack_execute lib/conntrack.c:1446:5
    #5 0x67e3ed in dp_execute_cb lib/dpif-netdev.c:8277:9
    openvswitch#6 0x7097d7 in odp_execute_actions lib/odp-execute.c:865:17
    #7 0x66409e in dp_netdev_execute_actions lib/dpif-netdev.c:8322:5
    #8 0x6877ad in dpif_netdev_execute lib/dpif-netdev.c:4090:5
    #9 0x6675db in dpif_netdev_operate lib/dpif-netdev.c:4129:25
    #10 0x691e5e in dpif_operate lib/dpif.c:1367:13
    #11 0x692909 in dpif_execute lib/dpif.c:1321:9
    #12 0x5b19c6 in packet_execute ofproto/ofproto-dpif.c:4991:5
    #13 0x5a2861 in ofproto_packet_out_finish ofproto/ofproto.c:3662:5
    #14 0x5a65c6 in do_bundle_commit ofproto/ofproto.c:8270:13
    #15 0x5a0cae in handle_bundle_control ofproto/ofproto.c:8309:17
    openvswitch#16 0x59a476 in handle_single_part_openflow ofproto/ofproto.c:8593:16
    #17 0x5877ac in handle_openflow ofproto/ofproto.c:8674:21
    openvswitch#18 0x6296f1 in ofconn_run ofproto/connmgr.c:1329:13
    #19 0x62925d in connmgr_run ofproto/connmgr.c:356:9
    #20 0x586904 in ofproto_run ofproto/ofproto.c:1879:5
    #21 0x55c830 in bridge_run__ vswitchd/bridge.c:3251:9
    openvswitch#22 0x55c015 in bridge_run vswitchd/bridge.c:3310:5
    openvswitch#23 0x575f31 in main vswitchd/ovs-vswitchd.c:127:9
    #24 0x7f01099d3492 in __libc_start_main (/lib64/libc.so.6+0x23492)

The issue can be reproduced with system-userspace testsuite on the
'conntrack - IPv4 fragmentation with fragments specified' test.
Previously, there was a leak inside the IP fragmentation module that
kept the original segment, so 'packet_clone' remained a valid pointer.
But commit 803ed12 ("ipf: release unhandled packets from the batch")
fixed the leak leading to use-after-free.

Using the packet from a batch instead of 'packet_clone' to swap packet
content to avoid the issue.

While investigating this problem, more issues uncovered.  One of them
is that IP fragmentation engine can add more packets to the batch, but
there is no way to get them to a caller.  Adding an extra branch for
this case with a 'FIXME' comment in order to highlight the issue.

Another one is that IP fragmentation engine will keep only 32 fragments
dropping all other fragments while refilling a batch, but that should
be fixed separately.

Fixes: 7e6b41a ("dpif-netdev: Fix crash when PACKET_OUT is metered.")
Signed-off-by: Ilya Maximets <[email protected]>
Acked-by: Aaron Conole <[email protected]>
igsilya added a commit to igsilya/ovs that referenced this pull request Oct 13, 2021
IP fragmentation engine may not only steal the packet but also add
more.  For example, after receiving the last fragment, it will
add all previous fragments to a batch.  Unfortunately, it will also
free the original last fragment and replace it with a copy.
This invalidates the 'packet_clone' pointer in the dpif_netdev_execute()
leading to the use-after-free:

==3525086==ERROR: AddressSanitizer: heap-use-after-free on
                  address 0x61600020439c at pc 0x000000688a6d
READ of size 1 at 0x61600020439c thread T0
    #0 0x688a6c in dp_packet_swap ./lib/dp-packet.h:265:5
    openvswitch#1 0x68781d in dpif_netdev_execute lib/dpif-netdev.c:4103:9
    openvswitch#2 0x6675db in dpif_netdev_operate lib/dpif-netdev.c:4129:25
    openvswitch#3 0x691e5e in dpif_operate lib/dpif.c:1367:13
    #4 0x692909 in dpif_execute lib/dpif.c:1321:9
    #5 0x5b19c6 in packet_execute ofproto/ofproto-dpif.c:4991:5
    openvswitch#6 0x5a2861 in ofproto_packet_out_finish ofproto/ofproto.c:3662:5
    #7 0x5a65c6 in do_bundle_commit ofproto/ofproto.c:8270:13
    #8 0x5a0cae in handle_bundle_control ofproto/ofproto.c:8309:17
    #9 0x59a476 in handle_single_part_openflow ofproto/ofproto.c:8593:16
    #10 0x5877ac in handle_openflow ofproto/ofproto.c:8674:21
    #11 0x6296f1 in ofconn_run ofproto/connmgr.c:1329:13
    #12 0x62925d in connmgr_run ofproto/connmgr.c:356:9
    #13 0x586904 in ofproto_run ofproto/ofproto.c:1879:5
    #14 0x55c830 in bridge_run__ vswitchd/bridge.c:3251:9
    #15 0x55c015 in bridge_run vswitchd/bridge.c:3310:5
    openvswitch#16 0x575f31 in main vswitchd/ovs-vswitchd.c:127:9
    #17 0x7f01099d3492 in __libc_start_main (/lib64/libc.so.6+0x23492)
    openvswitch#18 0x47d96d in _start (vswitchd/ovs-vswitchd+0x47d96d)

0x61600020439c is located 28 bytes inside of 560-byte region
freed by thread T0 here:
    #0 0x5177a8 in free (vswitchd/ovs-vswitchd+0x5177a8)
    openvswitch#1 0x6b17b6 in dp_packet_delete ./lib/dp-packet.h:256:9
    openvswitch#2 0x6afeee in ipf_extract_frags_from_batch lib/ipf.c:947:17
    openvswitch#3 0x6afd63 in ipf_preprocess_conntrack lib/ipf.c:1232:9
    #4 0x946b2c in conntrack_execute lib/conntrack.c:1446:5
    #5 0x67e3ed in dp_execute_cb lib/dpif-netdev.c:8277:9
    openvswitch#6 0x7097d7 in odp_execute_actions lib/odp-execute.c:865:17
    #7 0x66409e in dp_netdev_execute_actions lib/dpif-netdev.c:8322:5
    #8 0x6877ad in dpif_netdev_execute lib/dpif-netdev.c:4090:5
    #9 0x6675db in dpif_netdev_operate lib/dpif-netdev.c:4129:25
    #10 0x691e5e in dpif_operate lib/dpif.c:1367:13
    #11 0x692909 in dpif_execute lib/dpif.c:1321:9
    #12 0x5b19c6 in packet_execute ofproto/ofproto-dpif.c:4991:5
    #13 0x5a2861 in ofproto_packet_out_finish ofproto/ofproto.c:3662:5
    #14 0x5a65c6 in do_bundle_commit ofproto/ofproto.c:8270:13
    #15 0x5a0cae in handle_bundle_control ofproto/ofproto.c:8309:17
    openvswitch#16 0x59a476 in handle_single_part_openflow ofproto/ofproto.c:8593:16
    #17 0x5877ac in handle_openflow ofproto/ofproto.c:8674:21
    openvswitch#18 0x6296f1 in ofconn_run ofproto/connmgr.c:1329:13
    #19 0x62925d in connmgr_run ofproto/connmgr.c:356:9
    #20 0x586904 in ofproto_run ofproto/ofproto.c:1879:5
    #21 0x55c830 in bridge_run__ vswitchd/bridge.c:3251:9
    openvswitch#22 0x55c015 in bridge_run vswitchd/bridge.c:3310:5
    openvswitch#23 0x575f31 in main vswitchd/ovs-vswitchd.c:127:9
    #24 0x7f01099d3492 in __libc_start_main (/lib64/libc.so.6+0x23492)

The issue can be reproduced with system-userspace testsuite on the
'conntrack - IPv4 fragmentation with fragments specified' test.
Previously, there was a leak inside the IP fragmentation module that
kept the original segment, so 'packet_clone' remained a valid pointer.
But commit 803ed12 ("ipf: release unhandled packets from the batch")
fixed the leak leading to use-after-free.

Using the packet from a batch instead of 'packet_clone' to swap packet
content to avoid the issue.

While investigating this problem, more issues uncovered.  One of them
is that IP fragmentation engine can add more packets to the batch, but
there is no way to get them to a caller.  Adding an extra branch for
this case with a 'FIXME' comment in order to highlight the issue.

Another one is that IP fragmentation engine will keep only 32 fragments
dropping all other fragments while refilling a batch, but that should
be fixed separately.

Fixes: 7e6b41a ("dpif-netdev: Fix crash when PACKET_OUT is metered.")
Signed-off-by: Ilya Maximets <[email protected]>
Acked-by: Aaron Conole <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants