diff mbox

Add support for KernelAddressSanitizer

Message ID 53C9079F.3060907@samsung.com
State New
Headers show

Commit Message

Yury Gribov July 18, 2014, 11:40 a.m. UTC 
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?

-Y

Comments

Dmitry Vyukov July 18, 2014, 11:57 a.m. UTC | #1 
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.
Jakub Jelinek July 18, 2014, 12:26 p.m. UTC | #2 
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
Dmitry Vyukov July 18, 2014, 1:19 p.m. UTC | #3 
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.
Yury Gribov July 18, 2014, 1:31 p.m. UTC | #4 
>> 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
Jakub Jelinek July 18, 2014, 1:38 p.m. UTC | #5 
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
Yury Gribov July 18, 2014, 2:15 p.m. UTC | #6 
> 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
diff mbox

Patch

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;
       }