On 10/12/21 23:12, Aaron Conole wrote: > Ilya Maximets <[email protected]> writes: > >> 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. > > This is useful in case some future mechanism does the same. > >> 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. > > Thanks. > >> 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. > > :( > > There's a lot of work still needed for this feature. Separately, I've > noticed that the purge timeout isn't settable, and is much longer than > the 15s or so that is noted in the commit (on my system, it tested at > almost two minutes) > >> Fixes: 7e6b41ac8d9d ("dpif-netdev: Fix crash when PACKET_OUT is metered.") >> Signed-off-by: Ilya Maximets <[email protected]> >> --- > > Thanks for the quick fix. > > Acked-by: Aaron Conole <[email protected]> >
Thanks! I applied this patch and backported down to 2.12. At least, with this change I don't see any leaks or use-after-free issues in our tests while running under AddressSanitizer. Best regards, Ilya Maximets. _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
