Message ID | 53C9079F.3060907@samsung.com |
---|---|
State | New |
Headers | show |
On Fri, Jul 18, 2014 at 3:40 PM, Yury Gribov <y.gribov@samsung.com> wrote: > Hi all, > > This tiny patch adds support for KernelASan. KASan brings Asan error > detection capabilities to Linux kernel > (https://code.google.com/p/address-sanitizer/wiki/AddressSanitizerForKernel). > > KASan works similar to normal userspace ASan but disables some options which > are not yet supported by kernel (notably inline instrumentation, > stack/global protection and UAR). We would prefer to hide all necessary > tweaks under a user-friendly flag (-fsanitize=kernel-address) instead of > forcing them directly in kernel's CFLAGS. > > Kernel patches are currently under review in LKML > (https://lkml.org/lkml/2014/7/9/990). > > Bootstrapped and regtested on x64. > > Ok to commit? Thanks for doing this, Yury. The patch looks good to me FWIW, but please wait for Jakub or somebody else with stronger gcc-fu.
On Fri, Jul 18, 2014 at 03:40:15PM +0400, Yury Gribov wrote: > This tiny patch adds support for KernelASan. KASan brings Asan error > detection capabilities to Linux kernel > (https://code.google.com/p/address-sanitizer/wiki/AddressSanitizerForKernel). > > KASan works similar to normal userspace ASan but disables some options which > are not yet supported by kernel (notably inline instrumentation, > stack/global protection and UAR). We would prefer to hide all necessary > tweaks under a user-friendly flag (-fsanitize=kernel-address) instead of > forcing them directly in kernel's CFLAGS. > > Kernel patches are currently under review in LKML > (https://lkml.org/lkml/2014/7/9/990). I thought KAsan used different entry points (__kasan_* etc.), has that changed? Also, oring in SANITIZER_ADDRESS means you add -lasan to link flags, I'd guess that for -fsanitize=kernel-address you don't want to add any libraries at link time? Do you error out on -fsanitize=thread -fsanitize=kernel-address ? Perhaps -fsanitize=kernel-address -fsanitize=address should be invalid too? Jakub
On Fri, Jul 18, 2014 at 4:26 PM, Jakub Jelinek <jakub@redhat.com> wrote: > On Fri, Jul 18, 2014 at 03:40:15PM +0400, Yury Gribov wrote: >> This tiny patch adds support for KernelASan. KASan brings Asan error >> detection capabilities to Linux kernel >> (https://code.google.com/p/address-sanitizer/wiki/AddressSanitizerForKernel). >> >> KASan works similar to normal userspace ASan but disables some options which >> are not yet supported by kernel (notably inline instrumentation, >> stack/global protection and UAR). We would prefer to hide all necessary >> tweaks under a user-friendly flag (-fsanitize=kernel-address) instead of >> forcing them directly in kernel's CFLAGS. >> >> Kernel patches are currently under review in LKML >> (https://lkml.org/lkml/2014/7/9/990). > > I thought KAsan used different entry points (__kasan_* etc.), has that > changed? Yes, we've switched to __asan_. > Also, oring in SANITIZER_ADDRESS means you add -lasan to link flags, I'd > guess that for -fsanitize=kernel-address you don't want to add any libraries > at link time? I suspect that we don't pass -fsanitize=kernel-address during linking in kernel today. But I agree that it's better to disable any processing during linking for now. Later we may want to do something special during linking if -fsanitize=kernel-address is supplied. > Do you error out on -fsanitize=thread -fsanitize=kernel-address ? > Perhaps -fsanitize=kernel-address -fsanitize=address should be invalid too? Yes, all these combinations are invalid.
>> Also, oring in SANITIZER_ADDRESS means you add -lasan to link flags, I'd >> guess that for -fsanitize=kernel-address you don't want to add any libraries >> at link time? > > I suspect that we don't pass -fsanitize=kernel-address during linking > in kernel today. But I agree that it's better to disable any > processing during linking for now. Later we may want to do something > special during linking if -fsanitize=kernel-address is supplied. AFAIK kernel is linked directly with ld so this may not be a big issue. >> Do you error out on -fsanitize=thread -fsanitize=kernel-address ? >> Perhaps -fsanitize=kernel-address -fsanitize=address should be >> invalid too? > > Yes, all these combinations are invalid. Ok, I'll add these. -Y
On Fri, Jul 18, 2014 at 05:19:39PM +0400, Dmitry Vyukov wrote: > On Fri, Jul 18, 2014 at 4:26 PM, Jakub Jelinek <jakub@redhat.com> wrote: > > On Fri, Jul 18, 2014 at 03:40:15PM +0400, Yury Gribov wrote: > >> This tiny patch adds support for KernelASan. KASan brings Asan error > >> detection capabilities to Linux kernel > >> (https://code.google.com/p/address-sanitizer/wiki/AddressSanitizerForKernel). > >> > >> KASan works similar to normal userspace ASan but disables some options which > >> are not yet supported by kernel (notably inline instrumentation, > >> stack/global protection and UAR). We would prefer to hide all necessary > >> tweaks under a user-friendly flag (-fsanitize=kernel-address) instead of > >> forcing them directly in kernel's CFLAGS. > >> > >> Kernel patches are currently under review in LKML > >> (https://lkml.org/lkml/2014/7/9/990). > > > > I thought KAsan used different entry points (__kasan_* etc.), has that > > changed? > > Yes, we've switched to __asan_. Ok. > > Also, oring in SANITIZER_ADDRESS means you add -lasan to link flags, I'd > > guess that for -fsanitize=kernel-address you don't want to add any libraries > > at link time? > > I suspect that we don't pass -fsanitize=kernel-address during linking > in kernel today. But I agree that it's better to disable any > processing during linking for now. Later we may want to do something > special during linking if -fsanitize=kernel-address is supplied. > > > Do you error out on -fsanitize=thread -fsanitize=kernel-address ? > > Perhaps -fsanitize=kernel-address -fsanitize=address should be invalid too? > > Yes, all these combinations are invalid. But you don't error out on that. If we want to diagnose the last, IMHO we can't have just SANITIZE_ADDRESS and SANITIZE_KERNEL_ADDRESS flags, but instead should have SANITIZE_ADDRESS (used when we don't care about kernel vs. user asan differences), SANITIZE_USER_ADDRESS and SANITIZE_KERNEL_ADDRESS bits. "address" would set SANITIZE_ADDRESS | SANITIZE_USER_ADDRESS, "kernel-address" SANITIZE_ADDRESS | SANITIZE_KERNEL_ADDRESS. Then in sanitize_spec_function supposedly for "address" check SANITIZE_USER_ADDRESS bit, for "kernel-address" added there SANITIZE_KERNEL_ADDRESS, add all the incompatibility diagnostics for the new invalid combinations. Plus, toplev.c has e.g.: /* Address Sanitizer needs porting to each target architecture. */ if ((flag_sanitize & SANITIZE_ADDRESS) && (targetm.asan_shadow_offset == NULL || !FRAME_GROWS_DOWNWARD)) { warning (0, "-fsanitize=address not supported for this target"); flag_sanitize &= ~SANITIZE_ADDRESS; } Now, is the same really the case for SANITIZE_KERNEL_ADDRESS? I guess we still inline the shadow memory accesses to poison/unpoison stack in function prologue/epilogue, right? In that case without asan_shadow_offset we can't do anything. If it was a function call instead it would be portable to all architectures. Jakub
> Then in sanitize_spec_function supposedly for "address" check > SANITIZE_USER_ADDRESS bit, for "kernel-address" added there > SANITIZE_KERNEL_ADDRESS, add all the incompatibility diagnostics for the new > invalid combinations. Ok. >Plus, toplev.c has e.g.: > ... > Now, is the same really the case for SANITIZE_KERNEL_ADDRESS? This is a good point, KASan does not use asan_shadow_offset so this check is redundant. >I guess we still inline the shadow memory accesses to poison/unpoison > stack in function prologue/epilogue, right? In that case without > asan_shadow_offset we can't do anything. If it was a function call instead > it would be portable to all architectures. Stack is not supported by current KASan. My local version indeed does replace asan_shadow_offset with function call. -Y
gcc/ 2014-07-18 Yury Gribov <y.gribov@samsung.com> * doc/invoke.texi (-fsanitize=kernel-address): Describe new option. * flag-types.h (SANITIZE_KERNEL_ADDRESS): New enum. * opts.c (common_handle_option): Handle new option. diff --git a/gcc/doc/invoke.texi b/gcc/doc/invoke.texi index a83f6c6..70f9c2b 100644 --- a/gcc/doc/invoke.texi +++ b/gcc/doc/invoke.texi @@ -5376,6 +5376,11 @@ more details. The run-time behavior can be influenced using the @url{https://code.google.com/p/address-sanitizer/wiki/Flags#Run-time_flags} for a list of supported options. +@item -fsanitize=kernel-address +@opindex fsanitize=kernel-address +Enable AddressSanitizer for Linux kernel. +See @uref{http://code.google.com/p/address-sanitizer/wiki/AddressSanitizerForKernel} for more details. + @item -fsanitize=thread @opindex fsanitize=thread Enable ThreadSanitizer, a fast data race detector. diff --git a/gcc/flag-types.h b/gcc/flag-types.h index 2849455..04038f6 100644 --- a/gcc/flag-types.h +++ b/gcc/flag-types.h @@ -231,6 +231,7 @@ enum sanitize_code { SANITIZE_FLOAT_DIVIDE = 1 << 12, SANITIZE_FLOAT_CAST = 1 << 13, SANITIZE_BOUNDS = 1 << 14, + SANITIZE_KERNEL_ADDRESS = 1 << 15, SANITIZE_UNDEFINED = SANITIZE_SHIFT | SANITIZE_DIVIDE | SANITIZE_UNREACHABLE | SANITIZE_VLA | SANITIZE_NULL | SANITIZE_RETURN | SANITIZE_SI_OVERFLOW | SANITIZE_BOOL | SANITIZE_ENUM diff --git a/gcc/opts.c b/gcc/opts.c index 419a074..42fef36 100644 --- a/gcc/opts.c +++ b/gcc/opts.c @@ -1475,6 +1475,7 @@ common_handle_option (struct gcc_options *opts, { "float-cast-overflow", SANITIZE_FLOAT_CAST, sizeof "float-cast-overflow" - 1 }, { "bounds", SANITIZE_BOUNDS, sizeof "bounds" - 1 }, + { "kernel-address", SANITIZE_KERNEL_ADDRESS, sizeof "kernel-address" - 1 }, { NULL, 0, 0 } }; const char *comma; @@ -1520,6 +1521,25 @@ common_handle_option (struct gcc_options *opts, the null pointer checks. */ if (flag_sanitize & SANITIZE_NULL) opts->x_flag_delete_null_pointer_checks = 0; + + /* Kernel ASan implies normal ASan but does not yet support + all features. */ + if (flag_sanitize & SANITIZE_KERNEL_ADDRESS) + { + flag_sanitize |= SANITIZE_ADDRESS; + maybe_set_param_value (PARAM_ASAN_INSTRUMENTATION_WITH_CALL_THRESHOLD, 0, + opts->x_param_values, + opts_set->x_param_values); + maybe_set_param_value (PARAM_ASAN_GLOBALS, 0, + opts->x_param_values, + opts_set->x_param_values); + maybe_set_param_value (PARAM_ASAN_STACK, 0, + opts->x_param_values, + opts_set->x_param_values); + maybe_set_param_value (PARAM_ASAN_USE_AFTER_RETURN, 0, + opts->x_param_values, + opts_set->x_param_values); + } break; }