[ovs-dev] [PATCH] ofproto: fix stack-buffer-overflow

[Ben Pfaff]

On Fri, Nov 29, 2019 at 04:15:59PM +0530, Numan Siddique wrote:
> On Fri, Nov 29, 2019 at 11:44 AM Linhaifeng <haifeng.lin at huawei.com> wrote:
> >
> > Should use flow->actions not &flow->actions.
> >
> > here is ASAN report:
> > =================================================================
> > ==57189==ERROR: AddressSanitizer: stack-buffer-overflow on address 0xffff428fa0e8 at pc 0xffff7f61a520 bp 0xffff428f9420 sp 0xffff428f9498 READ of size 196 at 0xffff428fa0e8 thread T150 (revalidator22)
> >     #0 0xffff7f61a51f in __interceptor_memcpy (/lib64/libasan.so.4+0xa251f)
> >     #1 0xaaaad26a3b2b in ofpbuf_put lib/ofpbuf.c:426
> >     #2 0xaaaad26a30cb in ofpbuf_clone_data_with_headroom lib/ofpbuf.c:248
> >     #3 0xaaaad26a2e77 in ofpbuf_clone_with_headroom lib/ofpbuf.c:218
> >     #4 0xaaaad26a2dc3 in ofpbuf_clone lib/ofpbuf.c:208
> >     #5 0xaaaad23e3993 in ukey_set_actions ofproto/ofproto-dpif-upcall.c:1640
> >     #6 0xaaaad23e3f03 in ukey_create__ ofproto/ofproto-dpif-upcall.c:1696
> >     #7 0xaaaad23e553f in ukey_create_from_dpif_flow ofproto/ofproto-dpif-upcall.c:1806
> >     #8 0xaaaad23e65fb in ukey_acquire ofproto/ofproto-dpif-upcall.c:1984
> >     #9 0xaaaad23eb583 in revalidate ofproto/ofproto-dpif-upcall.c:2625
> >     #10 0xaaaad23dee5f in udpif_revalidator ofproto/ofproto-dpif-upcall.c:1076
> >     #11 0xaaaad26b84ef in ovsthread_wrapper lib/ovs-thread.c:708
> >     #12 0xffff7e74a8bb in start_thread (/lib64/libpthread.so.0+0x78bb)
> >     #13 0xffff7e0665cb in thread_start (/lib64/libc.so.6+0xd55cb)
> >
> > Address 0xffff428fa0e8 is located in stack of thread T150 (revalidator22) at offset 328 in frame
> >     #0 0xaaaad23e4cab in ukey_create_from_dpif_flow ofproto/ofproto-dpif-upcall.c:1762
> >
> >   This frame has 4 object(s):
> >     [32, 96) 'actions'
> >     [128, 192) 'buf'
> >     [224, 328) 'full_flow'
> >     [384, 2432) 'stub' <== Memory access at offset 328 partially underflows this variable
> > HINT: this may be a false positive if your program uses some custom stack unwind mechanism or swapcontext
> >       (longjmp and C++ exceptions *are* supported) Thread T150 (revalidator22) created by T0 here:
> >     #0 0xffff7f5b0f7f in __interceptor_pthread_create (/lib64/libasan.so.4+0x38f7f)
> >     #1 0xaaaad26b891f in ovs_thread_create lib/ovs-thread.c:792
> >     #2 0xaaaad23dc62f in udpif_start_threads ofproto/ofproto-dpif-upcall.c:639
> >     #3 0xaaaad23daf87 in ofproto_set_flow_table ofproto/ofproto-dpif-upcall.c:446
> >     #4 0xaaaad230ff7f in dpdk_evs_cfg_set vswitchd/bridge.c:1134
> >     #5 0xaaaad2310097 in bridge_reconfigure vswitchd/bridge.c:1148
> >     #6 0xaaaad23279d7 in bridge_run vswitchd/bridge.c:3944
> >     #7 0xaaaad23365a3 in main vswitchd/ovs-vswitchd.c:240
> >     #8 0xffff7dfb1adf in __libc_start_main (/lib64/libc.so.6+0x20adf)
> >     #9 0xaaaad230a3d3  (/usr/sbin/ovs-vswitchd-2.7.0-1.1.RC5.001.asan+0x26f3d3)
> >
> > SUMMARY: AddressSanitizer: stack-buffer-overflow (/lib64/libasan.so.4+0xa251f) in __interceptor_memcpy Shadow bytes around the buggy address:
> >   0x200fe851f3c0: 00 00 00 00 f1 f1 f1 f1 f8 f2 f2 f2 00 00 00 00
> >   0x200fe851f3d0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> >   0x200fe851f3e0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> >   0x200fe851f3f0: 00 00 00 00 f1 f1 f1 f1 00 00 00 00 00 00 00 00
> >   0x200fe851f400: f2 f2 f2 f2 f8 f8 f8 f8 f8 f8 f8 f8 f2 f2 f2 f2
> > =>0x200fe851f410: 00 00 00 00 00 00 00 00 00 00 00 00 00[f2]f2 f2
> >   0x200fe851f420: f2 f2 f2 f2 00 00 00 00 00 00 00 00 00 00 00 00
> >   0x200fe851f430: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> >   0x200fe851f440: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> >   0x200fe851f450: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> >   0x200fe851f460: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 Shadow byte legend (one shadow byte represents 8 application bytes):
> >   Addressable:           00
> >   Partially addressable: 01 02 03 04 05 06 07
> >   Heap left redzone:       fa
> >   Freed heap region:       fd
> >   Stack left redzone:      f1
> >   Stack mid redzone:       f2
> >   Stack right redzone:     f3
> >   Stack after return:      f5
> >   Stack use after scope:   f8
> >   Global redzone:          f9
> >   Global init order:       f6
> >   Poisoned by user:        f7
> >   Container overflow:      fc
> >   Array cookie:            ac
> >   Intra object redzone:    bb
> >   ASan internal:           fe
> >   Left alloca redzone:     ca
> >   Right alloca redzone:    cb
> > ==57189==ABORTING
> >
> > Signed-off-by: Linhaifeng <haifeng.lin at huawei.com>
> 
> Acked-by: Numan Siddique <numans at ovn.org>

Linhaifeng, this is a great catch.  Thank you.  I applied this to master
and backported it.

I also checked through the source tree for other, similar issues.  I did
not find any.