Skip to content

Memory safety issue in SSE code #447

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Closed
necessarily-equal opened this issue Apr 13, 2021 · 2 comments
Closed

Memory safety issue in SSE code #447

necessarily-equal opened this issue Apr 13, 2021 · 2 comments

Comments

@necessarily-equal
Copy link
Contributor

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<CGameVM::QVMSyscall(int, Util::Reader&, IPC::Channel&)::<lambda(const string&, int&)>, std::tuple<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >&&, int&>, 0, 1> /home/afontain/unv/Unvanquished/daemon/src/common/Util.h:125
    #6 0x619e2d in apply<CGameVM::QVMSyscall(int, Util::Reader&, IPC::Channel&)::<lambda(const string&, int&)>, std::tuple<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >&&, int&> > /home/afontain/unv/Unvanquished/daemon/src/common/Util.h:130
    #7 0x619e2d in HandleMsg<CGameVM::QVMSyscall(int, Util::Reader&, IPC::Channel&)::<lambda(const string&, int&)>, IPC::Message<IPC::Id<0, 36>, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > >, IPC::Reply<int> > /home/afontain/unv/Unvanquished/daemon/src/common/IPC/Channel.h:217
    #8 0x619e2d in HandleMsg<IPC::SyncMessage<IPC::Message<IPC::Id<0, 36>, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > >, IPC::Reply<int> >, CGameVM::QVMSyscall(int, Util::Reader&, IPC::Channel&)::<lambda(const string&, int&)> > /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<IPC::SyncMessage<IPC::Message<IPC::Id<(unsigned short)0, (unsigned short)1>, int, int, glconfig_t, std::array<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, 1024ul> >, IPC::Reply<> >, int&, int&, glconfig_t&, std::array<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, 1024ul>&>(int&, int&, glconfig_t&, std::array<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, 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<VM::VMBase::SendMsg<IPC::SyncMessage<IPC::Message<IPC::Id<(unsigned short)0, (unsigned short)1>, int, int, glconfig_t, std::array<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, 1024ul> >, IPC::Reply<> >, int&, int&, glconfig_t&, std::array<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, 1024ul>&>(int&, int&, glconfig_t&, std::array<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, 1024ul>&)::{lambda(unsigned int, Util::Reader)#1}&, IPC::Message<IPC::Id<(unsigned short)0, (unsigned short)1>, int, int, glconfig_t, std::array<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, 1024ul> >, IPC::Reply<>, int&, int&, glconfig_t&, std::array<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, 1024ul>&>(IPC::Channel&, VM::VMBase::SendMsg<IPC::SyncMessage<IPC::Message<IPC::Id<(unsigned short)0, (unsigned short)1>, int, int, glconfig_t, std::array<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, 1024ul> >, IPC::Reply<> >, int&, int&, glconfig_t&, std::array<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, 1024ul>&>(int&, int&, glconfig_t&, std::array<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, 1024ul>&)::{lambda(unsigned int, Util::Reader)#1}&, IPC::SyncMessage<IPC::Message<IPC::Id<(unsigned short)0, (unsigned short)1>, int, int, glconfig_t, std::array<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, 1024ul> >, IPC::Reply<> >, int&, int&, glconfig_t&, std::array<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, 1024ul>&) /home/afontain/unv/Unvanquished/daemon/src/common/IPC/Channel.h:174
    #13 0x62bcf9 in void IPC::SendMsg<IPC::SyncMessage<IPC::Message<IPC::Id<(unsigned short)0, (unsigned short)1>, int, int, glconfig_t, std::array<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, 1024ul> >, IPC::Reply<> >, VM::VMBase::SendMsg<IPC::SyncMessage<IPC::Message<IPC::Id<(unsigned short)0, (unsigned short)1>, int, int, glconfig_t, std::array<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, 1024ul> >, IPC::Reply<> >, int&, int&, glconfig_t&, std::array<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, 1024ul>&>(int&, int&, glconfig_t&, std::array<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, 1024ul>&)::{lambda(unsigned int, Util::Reader)#1}, int&, int&, glconfig_t&, std::array<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, 1024ul>&>(IPC::Channel&, VM::VMBase::SendMsg<IPC::SyncMessage<IPC::Message<IPC::Id<(unsigned short)0, (unsigned short)1>, int, int, glconfig_t, std::array<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, 1024ul> >, IPC::Reply<> >, int&, int&, glconfig_t&, std::array<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, 1024ul>&>(int&, int&, glconfig_t&, std::array<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, 1024ul>&)::{lambda(unsigned int, Util::Reader)#1}&&, int&, int&, glconfig_t&, std::array<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, 1024ul>&) /home/afontain/unv/Unvanquished/daemon/src/common/IPC/Channel.h:234
    #14 0x62bcf9 in void VM::VMBase::SendMsg<IPC::SyncMessage<IPC::Message<IPC::Id<(unsigned short)0, (unsigned short)1>, int, int, glconfig_t, std::array<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, 1024ul> >, IPC::Reply<> >, int&, int&, glconfig_t&, std::array<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, 1024ul>&>(int&, int&, glconfig_t&, std::array<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, 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) '<unknown>'
    [48, 56) 'mod_name' (line 438)
    [80, 88) 'mod_name' (line 439)
    [112, 128) '<unknown>'
    [144, 160) '<unknown>'
    [176, 192) '<unknown>'
    [208, 224) '<unknown>'
    [240, 256) '<unknown>'
    [272, 288) '<unknown>'
    [304, 320) '<unknown>'
    [336, 352) '<unknown>'
    [368, 384) '<unknown>'
    [400, 416) '<unknown>'
    [432, 448) '<unknown>'
    [464, 480) '<unknown>'
    [496, 512) '<unknown>'
    [528, 544) '<unknown>'
    [560, 576) '<unknown>'
    [592, 608) '<unknown>'
    [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
Copy link
Member

slipher commented Apr 13, 2021

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.

@slipher
Copy link
Member

slipher commented May 1, 2025

I went over those SSE math functions add added unit tests in #1452 so I think we can be confident enough that they work.

@slipher slipher closed this as completed May 1, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
2 participants
@slipher @necessarily-equal