This is an archive of the discontinued LLVM Phabricator instance.

msan: account for AVX state when unpoison ucontext_t
ClosedPublic

Authored by dvyukov on Dec 23 2021, 12:15 AM.

Details

Summary

ucontext_t can be larger than its static size if it contains
AVX state and YMM/ZMM registers.
Currently a signal handler that tries to access that state
can produce false positives with random origins on stack.
Account for the additional ucontext_t state.

Diff Detail

Event Timeline

dvyukov created this revision.Dec 23 2021, 12:15 AM
dvyukov requested review of this revision.Dec 23 2021, 12:15 AM
Herald added a project: Restricted Project. · View Herald TranscriptDec 23 2021, 12:15 AM
Herald added a subscriber: Restricted Project. · View Herald Transcript
vitalybuka added inline comments.Dec 23 2021, 12:23 PM
compiler-rt/lib/sanitizer_common/sanitizer_platform_limits_posix.cpp
176

undef RELEASE?

215

Weird and unrelated: entire file is #if SANITIZER_LINUX || SANITIZER_MAC, but we have a lot of SANITIZER_FREEBSD and SANITIZER_NETBSD

226

what is about SANITIZER_MAC?

dvyukov updated this revision to Diff 396765.Dec 31 2021, 12:53 AM

fixed issues pointed by comments and rebased

PTAL

compiler-rt/lib/sanitizer_common/sanitizer_platform_limits_posix.cpp
176

Added explanatory comment.

226

Added if defined(SANITIZER_LINUX)

vitalybuka accepted this revision.Jan 5 2022, 12:39 AM
vitalybuka added inline comments.
compiler-rt/lib/sanitizer_common/sanitizer_platform_limits_posix.cpp
176

Oh, we should rename our sanitizer_thread_safety, those names are extremely common.
e.g. SANITIZER_ prefix

224

SANITIZER_X64

compiler-rt/lib/sanitizer_common/sanitizer_platform_limits_posix.h
65

probably relevant only for msvc which should not use this file
so maybe SANITIZER_X64?

compiler-rt/test/msan/Linux/signal_mcontext.cpp
12

SANITIZER_X64

This revision is now accepted and ready to land.Jan 5 2022, 12:39 AM
This revision was landed with ongoing or failed builds.Jan 5 2022, 4:20 AM
This revision was automatically updated to reflect the committed changes.
dvyukov marked 2 inline comments as done.
dvyukov added inline comments.Jan 5 2022, 4:21 AM
compiler-rt/lib/sanitizer_common/sanitizer_platform_limits_posix.cpp
176

I will send a follow up fix.

224

Done, but somehow that's not what we use in the codebase:

$ git grep "if.*SANITIZER_X64" | wc -l
1
$ git grep "if.*defined(x86_64)" | wc -l
123

compiler-rt/test/msan/Linux/signal_mcontext.cpp
12

It does not seem that we define these macros for tests anywhere. At least none of the SANITIZER_* macros are used in tests so far.

dvyukov added inline comments.Jan 5 2022, 4:37 AM
compiler-rt/lib/sanitizer_common/sanitizer_platform_limits_posix.cpp
176

Hi, we're seeing breakages building LLVM for Fuchsia, that appear to be caused by https://reviews.llvm.org/D116208 when building compiler-rt.

The error message is as follows:

/b/s/w/ir/x/w/llvm-project/compiler-rt/lib/sanitizer_common/sanitizer_platform_limits_posix.cpp:227:17: error: no member named '__glibc_reserved1' in '_libc_fpstate'
    if (fpregs->__glibc_reserved1[12] == FP_XSTATE_MAGIC1)
        ~~~~~~  ^
/b/s/w/ir/x/w/llvm-project/compiler-rt/lib/sanitizer_common/sanitizer_platform_limits_posix.cpp:229:22: error: no member named '__glibc_reserved1' in '_libc_fpstate'
             fpregs->__glibc_reserved1[13] - static_cast<const char *>(ctx);
             ~~~~~~  ^

I've filled and issue on github. https://github.com/llvm/llvm-project/issues/53014

It appears this code assumes glibc >= 2.26.

@mcgrathr suggested this as a potential fix:

--- a/compiler-rt/lib/sanitizer_common/sanitizer_platform_limits_posix.cpp      
+++ b/compiler-rt/lib/sanitizer_common/sanitizer_platform_limits_posix.cpp      
@@ -224,9 +224,13 @@ namespace __sanitizer {                                    
 #    if SANITIZER_LINUX && SANITIZER_X64                                       
     // See kernel arch/x86/kernel/fpu/signal.c for details.                    
     const auto *fpregs = static_cast<ucontext_t *>(ctx)->uc_mcontext.fpregs;   
-    if (fpregs->__glibc_reserved1[12] == FP_XSTATE_MAGIC1)                     
+    // The member names differ across header versions, but the actual layout   
+    // is always the same.  So avoid using members, just use arithmetic.       
+    const uint32_t *after_xmm =                                                
+        reinterpret_cast<const uint32_t*>(fpregs + 1) - 24;                    
+    if (after_xmm[12] == FP_XSTATE_MAGIC1)                                     
       return reinterpret_cast<const char *>(fpregs) +                          
-             fpregs->__glibc_reserved1[13] - static_cast<const char *>(ctx);   
+             after_xmm[13] - static_cast<const char *>(ctx);                   
 #    endif                                                                     
     return sizeof(ucontext_t);                                                 
   }

We've applied that patch to work around this in https://reviews.llvm.org/D116695

Hi Paul,

Just got back from holidays. I see you already addressed the problem in D116695. Thanks for that. And sorry for the breakage.

It was no problem. Roland had the patch immediately when I asked him about
it, and it seemed easier than reverting three commits.