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 #1 0x68781d in dpif_netdev_execute lib/dpif-netdev.c:4103:9 #2 0x6675db in dpif_netdev_operate lib/dpif-netdev.c:4129:25 #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 #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 #16 0x575f31 in main vswitchd/ovs-vswitchd.c:127:9 #17 0x7f01099d3492 in __libc_start_main (/lib64/libc.so.6+0x23492) #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) #1 0x6b17b6 in dp_packet_delete ./lib/dp-packet.h:256:9 #2 0x6afeee in ipf_extract_frags_from_batch lib/ipf.c:947:17 #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 #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 #16 0x59a476 in handle_single_part_openflow ofproto/ofproto.c:8593:16 #17 0x5877ac in handle_openflow ofproto/ofproto.c:8674:21 #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 #22 0x55c015 in bridge_run vswitchd/bridge.c:3310:5 #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 803ed12e31b0 ("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: 7e6b41ac8d9d ("dpif-netdev: Fix crash when PACKET_OUT is metered.") Signed-off-by: Ilya Maximets <i.maxim...@ovn.org> --- lib/dpif-netdev.c | 16 +++++++++++++--- 1 file changed, 13 insertions(+), 3 deletions(-) diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c index 75f381ec1..b078c2da5 100644 --- a/lib/dpif-netdev.c +++ b/lib/dpif-netdev.c @@ -4096,11 +4096,21 @@ dpif_netdev_execute(struct dpif *dpif, struct dpif_execute *execute) dp_netdev_pmd_unref(pmd); } - if (dp_packet_batch_size(&pp)) { + if (dp_packet_batch_size(&pp) == 1) { /* Packet wasn't dropped during the execution. Swapping content with * the original packet, because the caller might expect actions to - * modify it. */ - dp_packet_swap(execute->packet, packet_clone); + * modify it. Uisng the packet from a batch instead of 'packet_clone' + * because it maybe stolen and replaced by other packet, e.g. by + * the fragmentation engine. */ + dp_packet_swap(execute->packet, pp.packets[0]); + dp_packet_delete_batch(&pp, true); + } else if (dp_packet_batch_size(&pp)) { + /* FIXME: We have more packets than expected. Likely, we got IP + * fragments of the reassembled packet. Dropping them here as we have + * no way to get them to the caller. It might be that all the required + * actions with them are already executed, but it also might not be a + * case, e.g. if dpif_netdev_execute() called to execute a single + * tunnel push. */ dp_packet_delete_batch(&pp, true); } -- 2.31.1 _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev