On Wed, Oct 13, 2021 at 5:12 AM Aaron Conole <acon...@redhat.com> wrote:
>
> Ilya Maximets <i.maxim...@ovn.org> 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)
>
Speaking of the purge timer issue, I think this is because the IPF module
is packet-driven, in your test there must be no new frag packets received
here. So ipf_send_frags_in_list can not be called.
And eventually frags are purged by the ipf_clean thread. In ipf_clean thread,
the purge timer is 1min exactly.
> > Fixes: 7e6b41ac8d9d ("dpif-netdev: Fix crash when PACKET_OUT is metered.")
> > Signed-off-by: Ilya Maximets <i.maxim...@ovn.org>
> > ---
>
> Thanks for the quick fix.
>
> Acked-by: Aaron Conole <acon...@redhat.com>
>
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
