search

DaemonEngine / Daemon

The Dæmon game engine. With some bits of ioq3 and XreaL.
https://unvanquished.net
BSD 3-Clause "New" or "Revised" License
295 stars 60 forks source link

Memory safety issue in SSE code #447

Open necessarily-equal opened 3 years ago

necessarily-equal commented 3 years ago

some functions, like TransAddTranslation, take a vec3_t as a parameter, and do SSE operations with it. From a quick read, it seems that SSE operations expect 16 bytes (octets) operand size, and vec3_t is a float[3], and is of size 12 on x86_64. This means that the last 4 octets of the buffer are garbage and could cause issues. I don't know enough of the subject matter to know if it can actually cause a real problem. You can find the Asan error below (-fsanitize=address).

Asan error ``` ================================================================= ==17472==ERROR: AddressSanitizer: stack-buffer-overflow on address 0x7fff6eb0bd40 at pc 0x00000099e66d bp 0x7fff6eb0b950 sp 0x7fff6eb0b948 READ of size 16 at 0x7fff6eb0bd40 thread T0 #0 0x99e66c in _mm_loadu_ps(float const*) /nix/store/fvf3qjqa5qpcjjkq37pb6ypnk1mzhf5h-gcc-9.3.0/lib/gcc/x86_64-unknown-linux-gnu/9.3.0/include/xmmintrin.h:934 #1 0x99e66c in TransAddTranslation(float const*, transform_t*) /home/afontain/unv/Unvanquished/daemon/src/engine/qcommon/q_shared.h:1255 #2 0x99e66c in R_LoadIQModel(model_t*, void*, int, char const*) /home/afontain/unv/Unvanquished/daemon/src/engine/renderer/tr_model_iqm.cpp:675 #3 0x9928f3 in RE_RegisterModel(char const*) /home/afontain/unv/Unvanquished/daemon/src/engine/renderer/tr_model.cpp:159 #4 0x619e2d in operator() /home/afontain/unv/Unvanquished/daemon/src/engine/client/cl_cgame.cpp:1266 #5 0x619e2d in apply_impl, std::tuple, std::allocator >&&, int&>, 0, 1> /home/afontain/unv/Unvanquished/daemon/src/common/Util.h:125 #6 0x619e2d in apply, std::tuple, std::allocator >&&, int&> > /home/afontain/unv/Unvanquished/daemon/src/common/Util.h:130 #7 0x619e2d in HandleMsg, IPC::Message, std::__cxx11::basic_string, std::allocator > >, IPC::Reply > /home/afontain/unv/Unvanquished/daemon/src/common/IPC/Channel.h:217 #8 0x619e2d in HandleMsg, std::__cxx11::basic_string, std::allocator > >, IPC::Reply >, CGameVM::QVMSyscall(int, Util::Reader&, IPC::Channel&):: > /home/afontain/unv/Unvanquished/daemon/src/common/IPC/Channel.h:241 #9 0x640f86 in CGameVM::QVMSyscall(int, Util::Reader&, IPC::Channel&) /home/afontain/unv/Unvanquished/daemon/src/engine/client/cl_cgame.cpp:1265 #10 0x646b8a in CGameVM::Syscall(unsigned int, Util::Reader, IPC::Channel&) /home/afontain/unv/Unvanquished/daemon/src/engine/client/cl_cgame.cpp:1097 #11 0x6614a4 in VM::VMBase::SendMsg, int, int, glconfig_t, std::array, std::allocator >, 1024ul> >, IPC::Reply<> >, int&, int&, glconfig_t&, std::array, std::allocator >, 1024ul>&>(int&, int&, glconfig_t&, std::array, std::allocator >, 1024ul>&)::{lambda(unsigned int, Util::Reader)#1}::operator()(unsigned int, Util::Reader) /home/afontain/unv/Unvanquished/daemon/src/engine/framework/VirtualMachine.h:142 #12 0x6614a4 in void IPC::detail::SendMsg, int, int, glconfig_t, std::array, std::allocator >, 1024ul> >, IPC::Reply<> >, int&, int&, glconfig_t&, std::array, std::allocator >, 1024ul>&>(int&, int&, glconfig_t&, std::array, std::allocator >, 1024ul>&)::{lambda(unsigned int, Util::Reader)#1}&, IPC::Message, int, int, glconfig_t, std::array, std::allocator >, 1024ul> >, IPC::Reply<>, int&, int&, glconfig_t&, std::array, std::allocator >, 1024ul>&>(IPC::Channel&, VM::VMBase::SendMsg, int, int, glconfig_t, std::array, std::allocator >, 1024ul> >, IPC::Reply<> >, int&, int&, glconfig_t&, std::array, std::allocator >, 1024ul>&>(int&, int&, glconfig_t&, std::array, std::allocator >, 1024ul>&)::{lambda(unsigned int, Util::Reader)#1}&, IPC::SyncMessage, int, int, glconfig_t, std::array, std::allocator >, 1024ul> >, IPC::Reply<> >, int&, int&, glconfig_t&, std::array, std::allocator >, 1024ul>&) /home/afontain/unv/Unvanquished/daemon/src/common/IPC/Channel.h:174 #13 0x62bcf9 in void IPC::SendMsg, int, int, glconfig_t, std::array, std::allocator >, 1024ul> >, IPC::Reply<> >, VM::VMBase::SendMsg, int, int, glconfig_t, std::array, std::allocator >, 1024ul> >, IPC::Reply<> >, int&, int&, glconfig_t&, std::array, std::allocator >, 1024ul>&>(int&, int&, glconfig_t&, std::array, std::allocator >, 1024ul>&)::{lambda(unsigned int, Util::Reader)#1}, int&, int&, glconfig_t&, std::array, std::allocator >, 1024ul>&>(IPC::Channel&, VM::VMBase::SendMsg, int, int, glconfig_t, std::array, std::allocator >, 1024ul> >, IPC::Reply<> >, int&, int&, glconfig_t&, std::array, std::allocator >, 1024ul>&>(int&, int&, glconfig_t&, std::array, std::allocator >, 1024ul>&)::{lambda(unsigned int, Util::Reader)#1}&&, int&, int&, glconfig_t&, std::array, std::allocator >, 1024ul>&) /home/afontain/unv/Unvanquished/daemon/src/common/IPC/Channel.h:234 #14 0x62bcf9 in void VM::VMBase::SendMsg, int, int, glconfig_t, std::array, std::allocator >, 1024ul> >, IPC::Reply<> >, int&, int&, glconfig_t&, std::array, std::allocator >, 1024ul>&>(int&, int&, glconfig_t&, std::array, std::allocator >, 1024ul>&) /home/afontain/unv/Unvanquished/daemon/src/engine/framework/VirtualMachine.h:140 #15 0x62bcf9 in CGameVM::CGameInit(int, int) /home/afontain/unv/Unvanquished/daemon/src/engine/client/cl_cgame.cpp:1021 #16 0x62c356 in CL_InitCGame() /home/afontain/unv/Unvanquished/daemon/src/engine/client/cl_cgame.cpp:661 #17 0x69bd5c in CL_DownloadsComplete /home/afontain/unv/Unvanquished/daemon/src/engine/client/cl_download.cpp:109 #18 0x69bd5c in CL_DownloadsComplete /home/afontain/unv/Unvanquished/daemon/src/engine/client/cl_download.cpp:64 #19 0x69c7e2 in CL_InitDownloads() /home/afontain/unv/Unvanquished/daemon/src/engine/client/cl_download.cpp:237 #20 0x6e4734 in CL_ParseGamestate(msg_t*) /home/afontain/unv/Unvanquished/daemon/src/engine/client/cl_parse.cpp:461 #21 0x6e6767 in CL_ParseServerMessage(msg_t*) /home/afontain/unv/Unvanquished/daemon/src/engine/client/cl_parse.cpp:571 #22 0x6cf86c in CL_PacketEvent(netadr_t const&, msg_t*) /home/afontain/unv/Unvanquished/daemon/src/engine/client/cl_main.cpp:2423 #23 0x4aa1da in Com_EventLoop() /home/afontain/unv/Unvanquished/daemon/src/engine/qcommon/common.cpp:447 #24 0x4ac473 in Com_Frame() /home/afontain/unv/Unvanquished/daemon/src/engine/qcommon/common.cpp:1005 #25 0x494054 in main /home/afontain/unv/Unvanquished/daemon/src/engine/framework/System.cpp:693 #26 0x7f41f8fcbcbc in __libc_start_main (/usr/lib/libc.so.6+0x23cbc) #27 0x496ca9 in _start (/home/antoine/unv/Unvanquished/build-nixos/daemon+0x496ca9) Address 0x7fff6eb0bd40 is located in stack of thread T0 at offset 736 in frame #0 0x99983f in R_LoadIQModel(model_t*, void*, int, char const*) /home/afontain/unv/Unvanquished/daemon/src/engine/renderer/tr_model_iqm.cpp:439 This frame has 23 object(s): [32, 36) '' [48, 56) 'mod_name' (line 438) [80, 88) 'mod_name' (line 439) [112, 128) '' [144, 160) '' [176, 192) '' [208, 224) '' [240, 256) '' [272, 288) '' [304, 320) '' [336, 352) '' [368, 384) '' [400, 416) '' [432, 448) '' [464, 480) '' [496, 512) '' [528, 544) '' [560, 576) '' [592, 608) '' [624, 696) 'vboData' (line 456) [736, 748) 'translate' (line 630) <== Memory access at offset 736 partially overflows this variable [768, 784) 'rotate' (line 631) [800, 864) 'name' (line 881) HINT: this may be a false positive if your program uses some custom stack unwind mechanism, swapcontext or vfork (longjmp and C++ exceptions *are* supported) SUMMARY: AddressSanitizer: stack-buffer-overflow /nix/store/fvf3qjqa5qpcjjkq37pb6ypnk1mzhf5h-gcc-9.3.0/lib/gcc/x86_64-unknown-linux-gnu/9.3.0/include/xmmintrin.h:934 in _mm_loadu_ps(float const*) Shadow bytes around the buggy address: 0x10006dd59750: 00 00 00 f2 00 00 00 f2 f2 f2 00 00 f2 f2 00 00 0x10006dd59760: f2 f2 00 00 f2 f2 00 00 f2 f2 00 00 f2 f2 00 00 0x10006dd59770: f2 f2 00 00 f2 f2 00 00 f2 f2 00 00 f2 f2 00 00 0x10006dd59780: f2 f2 00 00 f2 f2 00 00 f2 f2 00 00 f2 f2 00 00 0x10006dd59790: f2 f2 00 00 f2 f2 00 00 f2 f2 00 00 00 00 00 00 =>0x10006dd597a0: 00 00 00 f2 f2 f2 f2 f2[00]04 f2 f2 00 00 f2 f2 0x10006dd597b0: 00 00 00 00 00 00 00 00 f3 f3 f3 f3 00 00 00 00 0x10006dd597c0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 0x10006dd597d0: 00 00 00 00 f1 f1 f1 f1 00 00 00 f2 00 00 00 f2 0x10006dd597e0: f2 f2 00 00 f2 f2 00 00 f2 f2 00 00 f2 f2 00 00 0x10006dd597f0: f2 f2 00 00 00 00 00 00 00 00 00 00 f2 f2 f2 f2 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 Shadow gap: cc ==17472==ABORTING ```
slipher commented 3 years ago

I looked at these a while ago and I think the 4th element should never affect the result. I have a commit here adding directives to silence ASAN in some functions.