Re: [PATCH] Enable -fsanitize-recover for KASan
On 09/05/2014 10:54 AM, Yury Gribov wrote: > Hi all, > > This patch enables -fsanitize-recover for KASan by default. This causes KASan > to continue execution after error in case of inline instrumentation. This > feature is needed because > - reports during early bootstrap won't even be printed > - needed to run all tests w/o rebooting machine for every test > - needed for interactive work on desktop > I just add that this option is required for inline instrumentation in kernel. There is some places in kernel where we validly touch poisoned memory and we need to disable error reporting in runtime. For that we use per task variable and check it __asan_report function and don't print anything if flag is raised. So there is must be the way to return from report functions. And how does it work if someone wants to try -fsanitize=address -fsanitize-recover. Seems you didn't touch libsanitzer in this patch, so I guess this will cause link time error, right ? > Bootstrapped and regtested on x64. > > Ok to commit? > > -Y
Re: [PATCHv4] Enable -fsanitize-recover for KASan
On 10/23/2014 11:28 AM, Yury Gribov wrote: > On 10/23/2014 11:13 AM, Jakub Jelinek wrote: >> On Thu, Oct 23, 2014 at 11:11:29AM +0400, Yury Gribov wrote: >>> Hi all, >>> >>> On 09/29/2014 09:21 PM, Yury Gribov wrote: >> This patch enables -fsanitize-recover for KASan by default. This causes >> KASan to continue execution after error in case of inline >> instrumentation. This feature is needed because >> - reports during early bootstrap won't even be printed >> - needed to run all tests w/o rebooting machine for every test >> - needed for interactive work on desktop This is the third version of patch which renames -fsanitize-recover to -fubsan-recover and introduces -fasan-recover (enabled by default for KASan). It also moves flag handling to finish_options per Jakub's request. >>> >>> A new version of patch based upon Jakub's recent changes to >>> -fsanitize-recover=. I've renamed __asan_report_recover_load* to >>> __asan_report_load*_noabort to match UBSan's style. >>> >>> Note that currently -fsanitize=kernel-address >>> -fno-sanitize-recover=kernel-address won't work as expected because we miss >>> __asan_load*_abort family of functions in libasan. >> >> I thought __asan_* functions are provided by the kernel, not libasan, for >> -fsanitize=kernel-address. Or is kernel linked with real libasan.a or >> some stripped down version thereof? > > Hm, right, libasan is not linked to kernel so it indeed does not need any > changes. But now I see that for -fsanitize=kernel-address we need both > __asan_load* and __asan_load*_noabort (the latter > being default) depending on -fsanitize-recover setting. Let me update the > patch for this. > IMO we don't need different versions of __asan_load* and __asan_load*_noabort, because -fno-sanitize-recover=kernel-address will never work with the linux kernel. I already said this before, and repeat this once again: There is few places in kernel where we validly touch poisoned memory, so we need to disable error reporting in runtime for such memory accesses. I use per-thread flag which is raised before the valid access to poisoned memory. This flag checked in __asan_report*() function. If it raised then we shouldn't print any error message, just silently exit from report. -fno-sanitize-recover=kernel-address will just cause early kernel crash on boot, so we will never use it.
Re: [PATCHv4] Enable -fsanitize-recover for KASan
On 10/23/2014 01:55 PM, Jakub Jelinek wrote: > On Thu, Oct 23, 2014 at 01:51:12PM +0400, Andrey Ryabinin wrote: >> IMO we don't need different versions of __asan_load* and >> __asan_load*_noabort, because >> -fno-sanitize-recover=kernel-address will never work with the linux kernel. >> >> I already said this before, and repeat this once again: >> There is few places in kernel where we validly touch poisoned memory, >> so we need to disable error reporting in runtime for such memory accesses. >> I use per-thread flag which is raised before the valid access to poisoned >> memory. >> This flag checked in __asan_report*() function. If it raised then we >> shouldn't print any error message, >> just silently exit from report. > > Can't you just use __attribute__((no_sanitize_address)) on the functions > that have such a code? Or you could use special macros for those accesses > (which could e.g. call function to read memory or write memory, implemented > in assembly or in __attribute__((no_sanitize_address)) function), or Those are quite generic functions used from a lot of places. So we want to instrument them in general, but there are few call sites which use those functions for poisoned memory. > temporarily unpoison and poison again. > That's a bit tricky. State of shadow memory is unknown, so we would need to store shadow somewhere before unpoisoning to restore it later. > Jakub >
Re: [PATCHv4] Enable -fsanitize-recover for KASan
On 10/23/2014 02:00 PM, Jakub Jelinek wrote: > On Thu, Oct 23, 2014 at 11:55:32AM +0200, Jakub Jelinek wrote: >> On Thu, Oct 23, 2014 at 01:51:12PM +0400, Andrey Ryabinin wrote: >>> IMO we don't need different versions of __asan_load* and >>> __asan_load*_noabort, because >>> -fno-sanitize-recover=kernel-address will never work with the linux kernel. >>> >>> I already said this before, and repeat this once again: >>> There is few places in kernel where we validly touch poisoned memory, >>> so we need to disable error reporting in runtime for such memory accesses. >>> I use per-thread flag which is raised before the valid access to poisoned >>> memory. >>> This flag checked in __asan_report*() function. If it raised then we >>> shouldn't print any error message, >>> just silently exit from report. >> >> Can't you just use __attribute__((no_sanitize_address)) on the functions >> that have such a code? Or you could use special macros for those accesses >> (which could e.g. call function to read memory or write memory, implemented >> in assembly or in __attribute__((no_sanitize_address)) function), or >> temporarily unpoison and poison again. > > Also, if you always rely on recovery for kernel-address, wonder why all the > effort to make it optional (when it could be decided based on > flag_sanitize & SANITIZE_KERNEL_ADDRESS), and whether I should wait > with 4.9.2-rc1 for that (given that 4.9 branch now has kasan support > backported, but not -fsanitize-recover (neither old style, nor new style)). > I'd really like to release 4.9.2 soon... > -fsanitize-recover needed only for inline instrumentation, and 4.9 don't support inline instrumentation for kernel-address. There is no reason to delay release unless you want to see inline support in 4.9. > Jakub >
Re: [PATCHv4] Enable -fsanitize-recover for KASan
On 10/23/2014 02:38 PM, Jakub Jelinek wrote:
> On Thu, Oct 23, 2014 at 02:33:42PM +0400, Yury Gribov wrote:
>> Actually this is a historical artifact. If inlining proves to be
>> significantly faster, they may want to switch.
>
> Ok.
>
>>> So, at that point you can include your ugly hacks in __asan_load* logic in
>>> the kernel, the difference between __asan_load4 and __asan_load4_noabort
>>> will be just that the latter will always return, while the former will not
>>> if some error has been reported.
>>> All the __asan_load* and __asan_store* entrypoints, regardless of
>>> -f{,no-}sanitize-recover=kernel-address are by definition not noreturn, they
>>> in the common case (if the code is not buggy) return.
>>
>> Perhaps we should just keep __asan_load* as is and leave the decision
>> whether to abort or continue for the runtime? This would make semantics of
>> -fsanitize-recover cumbersome though (because it wouldn't work if user
>> selects outline instrumentation).
>
> Well, the "don't ever report anything while some per-CPU flag is set" thing
> can be considered as part of the "is this memory access ok" test, it is
> pretending everything is accessible.
>
> But, otherwise, if it is supposed to be developer's decision at compile
> time, __asan_load*_noabort should better always continue, even if it
> reported issues, and __asan_load* should better not return after reporting
> errors.
>
True, but why we need new functions for that.
__asan_load could also abort or not depending on what user/developer wants.
Why we have to rebuild the entire kernel if someone wants to switch from abort
to noabort?
I'm not against __asan_load_noabort, I'm just saying that this is no point to
have separate
__asan_load/__asan_load_noabort functions in kernel.
Re: asan: support for globals in kernel
On 12/02/2014 08:56 PM, Dmitry Vyukov wrote:
> Hi,
>
> The following patch adds support for instrumentation of globals for
> Linux kernel (-fsanitize=kernel-address). Kernel only supports
> constructors with default priority, but the rest works fine.
>
> OK for trunk?
>
I know this is too late already, but why we need this?
IMO it's much better to add support for constructors with priorities in kernel.
We need to do this in kernel anyway, because GCC 4.9.2 don't have this patch and
I assume that we want to make instrumentation of globals work in kernel with
4.9.2
>
> https://codereview.appspot.com/176570043
>
> Index: gcc/ChangeLog
> ===
> --- gcc/ChangeLog (revision 218280)
> +++ gcc/ChangeLog (working copy)
> @@ -1,3 +1,8 @@
> +2014-12-02 Dmitry Vyukov
> +
> + * asan.c: (asan_finish_file): Use default priority for constructors
> + in kernel mode.
> +
> 2014-12-02 Ulrich Weigand
>
> PR target/64115
> Index: gcc/asan.c
> ===
> --- gcc/asan.c (revision 218280)
> +++ gcc/asan.c (working copy)
> @@ -1348,7 +1348,9 @@
> the var that is selected by the linker will have
> padding or not. */
>|| DECL_ONE_ONLY (decl)
> - /* Similarly for common vars. People can use -fno-common. */
> + /* Similarly for common vars. People can use -fno-common.
> + Note: Linux kernel is built with -fno-common, so we do instrument
> + globals there even if it is C. */
>|| (DECL_COMMON (decl) && TREE_PUBLIC (decl))
>/* Don't protect if using user section, often vars placed
> into user section from multiple TUs are then assumed
> @@ -2440,6 +2442,7 @@
> {
>varpool_node *vnode;
>unsigned HOST_WIDE_INT gcount = 0;
> + int priority;
>
>if (shadow_ptr_types[0] == NULL_TREE)
> asan_init_shadow_ptr_types ();
> @@ -2448,6 +2451,13 @@
> nor after .LASAN* array. */
>flag_sanitize &= ~SANITIZE_ADDRESS;
>
> + /* For user-space we want asan constructors to run first.
> + Linux kernel does not support priorities other than default, and the
> only
> + other user of constructors is coverage. So we run with the default
> + priority. */
> + priority = flag_sanitize & SANITIZE_USER_ADDRESS ?
> + MAX_RESERVED_INIT_PRIORITY - 1 : DEFAULT_INIT_PRIORITY;
> +
>if (flag_sanitize & SANITIZE_USER_ADDRESS)
> {
>tree fn = builtin_decl_implicit (BUILT_IN_ASAN_INIT);
> @@ -2503,12 +2513,10 @@
> build_fold_addr_expr (var),
> gcount_tree),
> &dtor_statements);
> - cgraph_build_static_cdtor ('D', dtor_statements,
> - MAX_RESERVED_INIT_PRIORITY - 1);
> + cgraph_build_static_cdtor ('D', dtor_statements, priority);
> }
>if (asan_ctor_statements)
> -cgraph_build_static_cdtor ('I', asan_ctor_statements,
> - MAX_RESERVED_INIT_PRIORITY - 1);
> +cgraph_build_static_cdtor ('I', asan_ctor_statements, priority);
>flag_sanitize |= SANITIZE_ADDRESS;
> }
>
Re: asan: support for globals in kernel
On 12/04/2014 10:10 PM, Dmitry Vyukov wrote:
> On Wed, Dec 3, 2014 at 9:19 AM, Andrey Ryabinin
> wrote:
>> On 12/02/2014 08:56 PM, Dmitry Vyukov wrote:
>>> Hi,
>>>
>>> The following patch adds support for instrumentation of globals for
>>> Linux kernel (-fsanitize=kernel-address). Kernel only supports
>>> constructors with default priority, but the rest works fine.
>>>
>>> OK for trunk?
>>>
>>
>> I know this is too late already, but why we need this?
>> IMO it's much better to add support for constructors with priorities in
>> kernel.
>>
>> We need to do this in kernel anyway, because GCC 4.9.2 don't have this patch
>> and
>> I assume that we want to make instrumentation of globals work in kernel with
>> 4.9.2
>
>
> That would be an option too. I don't know whether it is much better or not.
> Kernel lives without constructors, they are used only by coverage. And
> coverage does not need priorities. So it is only kasan that needs
> priorities. That would be a plenty of code in lib/module.c only for
> kasan...
It will be a very little piece of code. I don't think that it will be a problem
Perhaps, I'll cook a patch today.
>
> Meanwhile here is backport to 4.9. Applied w/o conflicts (not counting
> ChangeLog).
>
> OK to commit to gcc.gnu.org/svn/gcc/branches/gcc-4_9-branch ?
>
>
>
> Index: gcc/ChangeLog
> ===
> --- gcc/ChangeLog (revision 218382)
> +++ gcc/ChangeLog (working copy)
> @@ -1,3 +1,8 @@
> +2014-12-04 Dmitry Vyukov
> +
> + * asan.c: (asan_finish_file): Use default priority for constructors
> + in kernel mode.
> +
> 2014-12-04 Jakub Jelinek
>
> PR c++/56493
> Index: gcc/asan.c
> ===
> --- gcc/asan.c (revision 218382)
> +++ gcc/asan.c (working copy)
> @@ -1295,7 +1295,9 @@
> the var that is selected by the linker will have
> padding or not. */
>|| DECL_ONE_ONLY (decl)
> - /* Similarly for common vars. People can use -fno-common. */
> + /* Similarly for common vars. People can use -fno-common.
> + Note: Linux kernel is built with -fno-common, so we do instrument
> + globals there even if it is C. */
>|| (DECL_COMMON (decl) && TREE_PUBLIC (decl))
>/* Don't protect if using user section, often vars placed
> into user section from multiple TUs are then assumed
> @@ -2383,6 +2385,13 @@
> nor after .LASAN* array. */
>flag_sanitize &= ~SANITIZE_ADDRESS;
>
> + /* For user-space we want asan constructors to run first.
> + Linux kernel does not support priorities other than default, and the
> only
> + other user of constructors is coverage. So we run with the default
> + priority. */
> + int priority = flag_sanitize & SANITIZE_USER_ADDRESS
> + ? MAX_RESERVED_INIT_PRIORITY - 1 : DEFAULT_INIT_PRIORITY;
> +
>if (flag_sanitize & SANITIZE_USER_ADDRESS)
> {
>tree fn = builtin_decl_implicit (BUILT_IN_ASAN_INIT);
> @@ -2436,12 +2445,10 @@
> build_fold_addr_expr (var),
> gcount_tree),
> &dtor_statements);
> - cgraph_build_static_cdtor ('D', dtor_statements,
> - MAX_RESERVED_INIT_PRIORITY - 1);
> + cgraph_build_static_cdtor ('D', dtor_statements, priority);
> }
>if (asan_ctor_statements)
> -cgraph_build_static_cdtor ('I', asan_ctor_statements,
> - MAX_RESERVED_INIT_PRIORITY - 1);
> +cgraph_build_static_cdtor ('I', asan_ctor_statements, priority);
>flag_sanitize |= SANITIZE_ADDRESS;
> }
>
Re: [PATCH] Support asan-fixed-shadow-offset in GCC
On 07/21/14 23:00, Alexey Preobrazhensky wrote: > Hi all, > > This patch adds support for non-fixed shadow in asan stack instrumentation. > > It is required for Kernel AddressSanitizer, as the shadow offset is > not known at the compile time, To get shadow offset this patch uses function __asan_get_shadow_ptr. Wouldn't be more effective just to read variable instead of function call? > and the shadow may not be allocated > during the early boot stages. > It's true for now, but at some future point I want to make shadow's allocation very early, before running any instrumented code, so check for __asan_get_shadow_ptr() == 0 will be useless. > This option is intended to be triggered by -fsanitize=kernel-address > option, together with enabling instrumentation with calls. > > Bootstrapped®tested on x86_64. > > Codereview: https://codereview.appspot.com/118040043/ > > -- > Alexey >
Re: [PATCH] Support asan-fixed-shadow-offset in GCC
On 07/22/14 14:30, Yury Gribov wrote: >>> It is required for Kernel AddressSanitizer, as the shadow offset is >>> not known at the compile time, >> >> To get shadow offset this patch uses function __asan_get_shadow_ptr. >> Wouldn't be more effective just to read variable instead of function call? > > Depends on how much logic you want to hide there. If it's just "return > something" than sure > but if you need some synchronization or complex calculations, accessing > global would not be enough. > This function just returns some global variable, and I don't think we will need something more complex in future. > -Y >
