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

[Ilya Maximets]

On 10/12/21 23:12, Aaron Conole 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)
> 
>> 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>
> 

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.