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

Wan Junjie wanjunjie at bytedance.com
Wed Oct 13 03:53:11 UTC 2021


On Wed, Oct 13, 2021 at 5:12 AM Aaron Conole <aconole at redhat.com> wrote:
>
> 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)
>

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.maximets at ovn.org>
> > ---
>
> Thanks for the quick fix.
>
> Acked-by: Aaron Conole <aconole at redhat.com>
>


More information about the dev mailing list