[ovs-dev] [PATCH] dpif-netdev: Fix use-after-free on PACKET_OUT of IP fragments.

[Aaron Conole]

Ilya Maximets <i.maximets at 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)

> Fixes: 7e6b41ac8d9d ("dpif-netdev: Fix crash when PACKET_OUT is metered.")
> Signed-off-by: Ilya Maximets <i.maximets at ovn.org>
> ---

Thanks for the quick fix.

Acked-by: Aaron Conole <aconole at redhat.com>