Hi, On 7/9/2022 15:09, Martin Storsjö wrote: > On Sat, 3 Sep 2022, Alvin Wong via Mingw-w64-public wrote: > >> >> (Note: the patches to add the Clang option hasn't landed yet, therefore >> this is still subject to change.) > > Thanks a lot for your work on this matter! > >> >> This adds support to enable building mingw-w64-crt and user code with >> Control Flow Gurad using Clang, > > Nit: Typo in Guard above.
Noted, will be fixed next time. >> with the option `--enable-cfguard`. In >> addition to adding the Clang 16 compiler option `-mguard=cf` to CFLAGS, >> it also add two new objects to `libmingwex.a`: >> >> - `mingw_cfguard_support.c` contains the guard check and dispatch >> function pointers and their backward compatible no-op. >> - `mingw_cfguard_loadcfg.S` contains the definition of a load config >> directory structure with the symbol name `_load_config_used`. > > I'm a bit undecided about whether always should add these files, even if the > crt files aren't compiled with CFG enabled (for simplicity and robustness > maybe). > > On the other hand, I thought that leaving it out might be good - that we'd > get a linker error if we link with -mguard=cf but the symbol is missing - but > apparently lld doesn't error out in that configuration. But with your patch > in https://reviews.llvm.org/D133099, we'd at least get a warning in that > setting, so I guess this setup, with leaving out the files unless CFG is > enabled, makes sense. > A third thought on the matter: IIRC the _load_config_used structure is used > for a bunch of other things too, not only CFG. If other aspects of it would > be used in mingw-w64 contexts, I guess we should include it unconditionally? I don't think the other fields in the load config are useful to mingw-w64 but I could be wrong. I can think of a reason to always include the guard check function pointers in mingw_cfguard_support.c -- to support linking to object files compiled with -mguard=cf. Actually I think it may be better to always include them. >> This change alone is not enough to fully enable CFGuard in user-built >> binaries. All other libraries shipped with the toolchain will also need >> to be built with `-mguard=cf` to include CFGuard checks. Same goes for >> building user code. > > What happens if you'd have all the necessary structures (_load_config_used > etc), but e.g. the crt startup files wouldn't be compiled with -mguard=cf? > (What happens both in the simple case if those files don't do any indirect > calls - and what happens if they do?) From within these object files nothing changes. There are no guard checks inserted at indirect call sites. If the final executable is linked with CFGuard disabled, then the GuardFlags field is not set and the executable should run normally without CFGuard. If the executable is linked with CFGuard enabled: LLD does have a fallback case for object files not compiled with -mguard=cf, which takes all relocations and treat them as potential call targets to be added to the guard function table. This means if any of these function addresses are being passed to other parts of the process (e.g. to Windows API) which do perform CFGuard checks, they will not fail the check. But this also means there may be more valid call targets that malicious parties can potentially abuse in an attack (though it is slightly better than without CFGuard at all). It may also give a false sense of security since the executable claims to be "CF instrumented", but in fact there are no CFGuard checks in certain parts of it. >> --- >> >> CFGuard works by having the compiler insert a check before performing >> any indirect calls (which terminates the process if the call target is >> invalid). The compiler also has to keep track of all the address-taken >> functions, which the linker will then use to assemble a >> `GuardCFFunctionTable` to list all valid indirect call target addresses. >> >> To maintain backward compatibile, the guard check function is referenced >> via a function pointer which points to a no-op by default. On systems >> supporting CFGuard, the image loader replaces the function pointer with >> the address of the actual guard check function. >> >> For this to work, the metadata is stored in the load config directory >> struct. MSVC supplies this struct in vcruntime as a symbol with the name >> `_load_config_used`, which the linker takes as the load config >> directory. In addition, the linker generates synthetic symbols to be >> included in the load config struct. This also works with LLD since it >> has implemented this feature. >> >> The load config directory can be inspected using `llvm-readobj >> --coff-load-config file.exe` or `dumpbin.exe /loadconfig file.exe` >> (dumpbin.exe is included in MSVC build tools.) >> >> Further reading: >> - Control Flow Guard: >> https://docs.microsoft.com/en-us/windows/win32/secbp/control-flow-guard >> - Load config structure: >> https://docs.microsoft.com/en-us/windows/win32/debug/pe-format#the-load-configuration-structure-image-only >> - Control Flow Guard metadata: >> https://docs.microsoft.com/en-us/windows/win32/secbp/pe-metadata >> - My personal notes on CFGuard: >> https://gist.github.com/alvinhochun/a65e4177e2b34d551d7ecb02b55a4b0a >> >> Signed-off-by: Alvin Wong <al...@alvinhc.com> >> --- >> mingw-w64-crt/Makefile.am | 8 +- >> mingw-w64-crt/cfguard/mingw_cfguard_loadcfg.S | 118 ++++++++++++++++++ >> mingw-w64-crt/cfguard/mingw_cfguard_support.c | 47 +++++++ >> mingw-w64-crt/configure.ac | 16 +++ >> 4 files changed, 188 insertions(+), 1 deletion(-) >> create mode 100644 mingw-w64-crt/cfguard/mingw_cfguard_loadcfg.S >> create mode 100644 mingw-w64-crt/cfguard/mingw_cfguard_support.c > >> --- /dev/null >> +++ b/mingw-w64-crt/cfguard/mingw_cfguard_loadcfg.S >> @@ -0,0 +1,118 @@ >> +/* >> +This assembly source file defines the `_load_config_used` structure, which >> is >> +purely data. The main purpose is to include the metadata necessary for >> enabling >> +Control Flow Guard (CFGuard). >> + >> +Many of the symbols referenced here are supplied by the linker. This file >> has >> +been constructed with LLVM/LLD in mind. >> +*/ > > Please add some sort of license header to the new source files. I guess this > one is the simplest one in use in the project: > > /** > * This file has no copyright assigned and is placed in the Public Domain. > * This file is part of the mingw-w64 runtime package. > * No warranty is given; refer to the file DISCLAIMER.PD within this package. > */ Will add them next time. >> + >> +#if defined(__x86_64__) || defined(__aarch64__) >> +# define PTR .8byte >> +# define ALIGN 8 >> +#elif defined(__i386__) || defined(__arm__) >> +# define PTR .4byte >> +# define ALIGN 4 >> +#else >> +# error "Load config structure is unimplemented for the current >> architecture." >> +#endif >> + >> +#if defined(__x86_64__) >> +/* >> +GuardCFCheckDispatch is only used on x86_64. For other platforms it should >> be 0. >> +Ref: https://docs.microsoft.com/en-us/windows/win32/secbp/pe-metadata >> +*/ >> +# define CFGUARD_USE_DISPATCH >> +#endif >> + >> +#if defined(__i386__) >> +# define SYM(x) _##x >> +#else >> +# define SYM(x) x >> +#endif >> + >> +#if defined(INCLUDE_MSVC_FEATURES) >> +/* >> +These features are specific to MSVC as far as I am aware. >> +*/ >> +# define HAS_GS_SECURITY_COOKIE >> +# if defined(__i386__) >> +# define HAS_SAFE_SE_HANDLER >> +# endif >> +# define HAS_ENCLAVE_CONFIG >> +#endif >> + >> + .section .rdata,"dr" >> + .globl SYM(_load_config_used) >> + .balign ALIGN >> +SYM(_load_config_used): >> + .4byte SYM(_load_config_used__end) - SYM(_load_config_used) /* Size >> */ >> + .4byte 0 /* TimeDateStamp */ >> + .2byte 0 /* MajorVersion */ >> + .2byte 0 /* MinorVersion */ >> + .4byte 0 /* GLobalFlagsClear */ >> + .4byte 0 /* GlobalFlagsSet */ >> + .4byte 0 /* CriticalSectionDefaultTimeout */ >> + PTR 0 /* DeCommitFreeBlockThreshold */ >> + PTR 0 /* DeCommitTotalFreeThreshold */ >> + PTR 0 /* LockPrefixTable */ >> + PTR 0 /* MaximumAllocationSize */ >> + PTR 0 /* VirtualMemoryThreshold */ >> + PTR 0 /* ProcessAffinityMask */ >> + .4byte 0 /* ProcessHeapFlags */ >> + .2byte 0 /* CSDVersion */ >> + .2byte 0 /* DependentLoadFlags */ >> + PTR 0 /* EditList */ >> +#if defined(HAS_GS_SECURITY_COOKIE) >> + PTR __security_cookie /* SecurityCookie */ >> +#else >> + PTR 0 /* SecurityCookie */ >> +#endif >> +#if defined(HAS_SAFE_SE_HANDLER) >> + PTR SYM(__safe_se_handler_table) /* SEHandlerTable */ >> + PTR SYM(__safe_se_handler_count) /* SEHandlerCount */ >> +#else >> + PTR 0 /* SEHandlerTable */ >> + PTR 0 /* SEHandlerCount */ >> +#endif >> + PTR SYM(__guard_check_icall_fptr) /* GuardCFCheckFunction */ >> +#if defined(CFGUARD_USE_DISPATCH) >> + PTR SYM(__guard_dispatch_icall_fptr) /* GuardCFCheckDispatch */ >> +#else >> + PTR 0 /* GuardCFCheckDispatch */ >> +#endif >> + PTR SYM(__guard_fids_table) /* GuardCFFunctionTable */ >> + PTR SYM(__guard_fids_count) /* GuardCFFunctionCount */ >> + .4byte SYM(__guard_flags) /* GuardFlags */ >> + .2byte 0 /* CodeIntegrity_Flags */ >> + .2byte 0 /* CodeIntegrity_Catalog */ >> + .4byte 0 /* CodeIntegrity_CatalogOffset */ >> + .4byte 0 /* CodeIntegrity_Reserved */ >> + PTR SYM(__guard_iat_table) /* GuardAddressTakenIatEntryTable */ >> + PTR SYM(__guard_iat_count) /* GuardAddressTakenIatEntryCount */ >> + PTR SYM(__guard_longjmp_table) /* GuardLongJumpTargetTable */ >> + PTR SYM(__guard_longjmp_count) /* GuardLongJumpTargetCount */ >> + PTR 0 /* DynamicValueRelocTable */ >> + PTR 0 /* CHPEMetadataPointer */ >> + PTR 0 /* GuardRFFailureRoutine */ >> + PTR 0 /* GuardRFFailureRoutineFunctionPointer */ >> + .4byte 0 /* DynamicValueRelocTableOffset */ >> + .2byte 0 /* DynamicValueRelocTableSection */ >> + .2byte 0 /* Reserved2 */ >> + PTR 0 /* GuardRFVerifyStackPointerFunctionPointer */ >> + .4byte 0 /* HotPatchTableOffset */ >> + .4byte 0 /* Reserved3 */ >> +#if defined(HAS_ENCLAVE_CONFIG) >> + PTR SYM(__enclave_config) /* EnclaveConfigurationPointer */ >> +#else >> + PTR 0 /* EnclaveConfigurationPointer */ >> +#endif >> + PTR 0 /* VolatileMetadataPointer */ >> + PTR SYM(__guard_eh_cont_table) /* GuardEHContinuationTable */ >> + PTR SYM(__guard_eh_cont_count) /* GuardEHContinuationCount */ >> + PTR 0 /* GuardXFGCheckFunctionPointer */ >> + PTR 0 /* GuardXFGDispatchFunctionPointer */ >> + PTR 0 /* GuardXFGTableDispatchFunctionPointer */ >> + PTR 0 /* CastGuardOsDeterminedFailureMode */ >> + >> +SYM(_load_config_used__end): >> diff --git a/mingw-w64-crt/cfguard/mingw_cfguard_support.c >> b/mingw-w64-crt/cfguard/mingw_cfguard_support.c >> new file mode 100644 >> index 00000000..859d1a7c >> --- /dev/null >> +++ b/mingw-w64-crt/cfguard/mingw_cfguard_support.c >> @@ -0,0 +1,47 @@ > > Ditto about license header > >> +#if defined(__x86_64__) >> + >> +// The target address is passed as a parameter in an arch-specific manner, >> +// however it is not specified how on x86_64 because >> __guard_dispatch_icall_fptr >> +// is used instead. My guess would be that it's passed on %rcx, but it >> doesn't >> +// really matter here because this is a no-op anyway. >> +static void __guard_check_icall_dummy(void) {} >> + >> +// When CFGuard is not supported, directly tail-call the target address, >> which >> +// is passed via %rax. >> +__asm__( >> + "__guard_dispatch_icall_dummy:\n" >> + " jmp *%rax\n" >> +); >> + >> +// This is intentionally declared as _not_ a function pointer, so that the >> +// jmp instruction is not included as a valid call target for CFGuard. >> +extern void *__guard_dispatch_icall_dummy; >> + >> +#elif defined(__i386__) || defined(__aarch64__) || defined(__arm__) >> + >> +// The target address is passed via %ecx (x86), X15 (aarch64) or R0 (arm), >> +// but it doesn't really matter here because this is a no-op anyway. >> +static void __guard_check_icall_dummy(void) {} >> + >> +#else >> +# error "CFGuard support is unimplemented for the current architecture." >> +#endif >> + >> +// I am not sure about the `.00cfg` section. This is just an attempt to >> follow >> +// what VC runtime defines -- it places all the guard check function >> pointers >> +// inside this section. The MSVC linker appears to merge this section into >> +// `.rdata`, but LLD does not do this at the time of writing. >> +// This section should be readonly data. The only thing that modifies these >> +// pointers is the PE image loader. >> + >> +__asm__(".section .00cfg,\"dr\""); >> + >> +__attribute__(( section (".00cfg") )) >> +void *__guard_check_icall_fptr = &__guard_check_icall_dummy; >> + >> +#if defined(__x86_64__) >> + >> +__attribute__(( section (".00cfg") )) >> +void *__guard_dispatch_icall_fptr = &__guard_dispatch_icall_dummy; >> + >> +#endif >> diff --git a/mingw-w64-crt/configure.ac b/mingw-w64-crt/configure.ac >> index ad509531..5c68ff96 100644 >> --- a/mingw-w64-crt/configure.ac >> +++ b/mingw-w64-crt/configure.ac >> @@ -277,6 +277,21 @@ AC_MSG_RESULT([$with_default_msvcrt]) >> AS_VAR_SET([MSVCRT_LIB],[lib${with_default_msvcrt}.a]) >> AC_SUBST([MSVCRT_LIB]) >> >> +AC_MSG_CHECKING([whether to include support for Control Flow Guard]) >> +AC_ARG_ENABLE([cfguard], >> + [AS_HELP_STRING([--enable-cfguard], >> + [Enable building with Control Flow Guard support (Clang)])], >> + [], >> + [AS_VAR_SET([enable_cfguard],[no])]) >> +AC_MSG_RESULT([$enable_cfguard]) >> +AS_CASE([$enable_cfguard], >> + [no],[], >> + [yes],[AS_VAR_SET([CFGUARD]) >> + AS_VAR_SET([CFGUARD_CFLAGS],[-mguard=cf])], >> + [AC_MSG_ERROR([invalid argument. Must be either yes or no.])]) >> +AM_CONDITIONAL([CFGUARD], [AS_VAR_TEST_SET([CFGUARD])]) >> +AC_SUBST([CFGUARD_CFLAGS]) > > I didn't throughly check what this does, but I presume you've tested both > --enable-cfguard, --disable-cfguard (is it even possible to hit the invalid > argument case?). Yes, both seems to work. The invalid argument case can be hit by passing --enable-cfguard=arg. > > // Martin > Regards, Alvin Wong _______________________________________________ Mingw-w64-public mailing list Mingw-w64-public@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/mingw-w64-public
