| Message ID | 5448AA21.9080601@samsung.com |
|---|---|
| State | New |
| Headers | show |
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? Jakub
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. -Y
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.
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. Jakub
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... Jakub
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 >
On Thu, Oct 23, 2014 at 02:09:47PM +0400, Andrey Ryabinin wrote: > 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. Actually, -fsanitize=kernel-address forcibly uses function calls (i.e. __asan_load* etc. rather than __asan_report_load* only if inline shadow memory test suggested there is a problem). 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. Jakub
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 >
On 10/23/2014 02:20 PM, Andrey Ryabinin wrote: > 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. +1
On 10/23/2014 02:16 PM, Jakub Jelinek wrote: > On Thu, Oct 23, 2014 at 02:09:47PM +0400, Andrey Ryabinin wrote: >> 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. > > Actually, -fsanitize=kernel-address forcibly uses function calls (i.e. > __asan_load* etc. rather than __asan_report_load* only if inline shadow > memory test suggested there is a problem). Actually this is a historical artifact. If inlining proves to be significantly faster, they may want to switch. > 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). -Y
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. Jakub
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.
On 10/23/2014 03:10 PM, Andrey Ryabinin wrote: > 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. I'd still suggest to emit __asan_load_noabort so that we match userspace (where __asan_load strictly matches __asan_report in terminating the program). Behavior of __asan_load_noabort can further be restricted by user via various environment settings (kernel parameters, /proc, etc.). @Dmitry: what's your opinion on this? -Y
On Fri, Oct 24, 2014 at 12:28 PM, Yury Gribov <y.gribov@samsung.com> wrote: > On 10/23/2014 03:10 PM, Andrey Ryabinin wrote: >> >> 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. > > > I'd still suggest to emit __asan_load_noabort so that we match userspace > (where __asan_load strictly matches __asan_report in terminating the > program). Behavior of __asan_load_noabort can further be restricted by user > via various environment settings (kernel parameters, /proc, etc.). > > @Dmitry: what's your opinion on this? I am somewhat lost in this thread and probably missing something. But why do we need __asan_load (which is not noabort) at all? Outline instrumentation is non a default mode for both user-space asan and kasan (at least in the envisioned future). I would expect that these non-typical cases that use outline instrumentation can also bear the overhead of non-noreturn functions. Can we use just one version of __asan_load and let runtime decide on abort?
On Fri, Oct 24, 2014 at 01:44:27PM +0400, Dmitry Vyukov wrote: > I am somewhat lost in this thread and probably missing something. > But why do we need __asan_load (which is not noabort) at all? Outline > instrumentation is non a default mode for both user-space asan and > kasan (at least in the envisioned future). I would expect that these > non-typical cases that use outline instrumentation can also bear the > overhead of non-noreturn functions. Can we use just one version of > __asan_load and let runtime decide on abort? __asan_load actually must never be noreturn, because in the common case where the load is valid it of course returns. Jakub
On Fri, Oct 24, 2014 at 1:50 PM, Jakub Jelinek <jakub@redhat.com> wrote: > On Fri, Oct 24, 2014 at 01:44:27PM +0400, Dmitry Vyukov wrote: >> I am somewhat lost in this thread and probably missing something. >> But why do we need __asan_load (which is not noabort) at all? Outline >> instrumentation is non a default mode for both user-space asan and >> kasan (at least in the envisioned future). I would expect that these >> non-typical cases that use outline instrumentation can also bear the >> overhead of non-noreturn functions. Can we use just one version of >> __asan_load and let runtime decide on abort? > > __asan_load actually must never be noreturn, because in the common > case where the load is valid it of course returns. Right! Then I am puzzled by that message by Yury: "I'd still suggest to emit __asan_load_noabort so that we match userspace (where __asan_load strictly matches __asan_report in terminating the program)" Why are we discussing __asan_load_noabort?
On Fri, Oct 24, 2014 at 11:50:58AM +0200, Jakub Jelinek wrote: > On Fri, Oct 24, 2014 at 01:44:27PM +0400, Dmitry Vyukov wrote: > > I am somewhat lost in this thread and probably missing something. > > But why do we need __asan_load (which is not noabort) at all? Outline > > instrumentation is non a default mode for both user-space asan and > > kasan (at least in the envisioned future). I would expect that these > > non-typical cases that use outline instrumentation can also bear the > > overhead of non-noreturn functions. Can we use just one version of > > __asan_load and let runtime decide on abort? > > __asan_load actually must never be noreturn, because in the common > case where the load is valid it of course returns. The point of __asan_load*_noabort (vs. __asan_load*) and __asan_report*_noabort (vs. __asan_report*) is to allow the choice what is fatal and what is not fatal to be done at compile time, per compilation unit. For __asan_report* that is a must, as __asan_report* without noabort is noreturn, for __asan_load* which is not noreturn the implementation can of course choose not to make something fatal when it wishes. Without -fsanitize-recover={address,kernel-address} support, the choice could be done only per program or kernel globally, without a way for programmer to differentiate. Jakub
On 10/24/2014 01:44 PM, Dmitry Vyukov wrote: > On Fri, Oct 24, 2014 at 12:28 PM, Yury Gribov <y.gribov@samsung.com> wrote: >> On 10/23/2014 03:10 PM, Andrey Ryabinin wrote: >>> >>> 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. >> >> >> I'd still suggest to emit __asan_load_noabort so that we match userspace >> (where __asan_load strictly matches __asan_report in terminating the >> program). Behavior of __asan_load_noabort can further be restricted by user >> via various environment settings (kernel parameters, /proc, etc.). >> >> @Dmitry: what's your opinion on this? > > I am somewhat lost in this thread and probably missing something. > But why do we need __asan_load (which is not noabort) at all? Outline > instrumentation is non a default mode for both user-space asan and > kasan (at least in the envisioned future). Well, it's still enabled automatically if number of memory accesses is getting large enough (I think it was 7000?) so it is default in a way. > I would expect that these > non-typical cases that use outline instrumentation can also bear the > overhead of non-noreturn functions. As Jakub mentioned both __asan_load and __asan_load_noabort will _not_ be NORETURN. Two different versions are necessary so that compiler can insert outline instrumentation that matches the -fsanitize-recover setting. > Can we use just one version of > __asan_load and let runtime decide on abort? In this case semantics of inline and outline instrumentation will differ (the former depending on -fsanitize-recover compile-time setting and the latter depending on runtime options) which may be undesirable given that compiler may automatically choose to switch from inline to outline depending on function size. -Y
On Fri, Oct 24, 2014 at 1:59 PM, Yury Gribov <y.gribov@samsung.com> wrote: > On 10/24/2014 01:44 PM, Dmitry Vyukov wrote: >> >> On Fri, Oct 24, 2014 at 12:28 PM, Yury Gribov <y.gribov@samsung.com> >> wrote: >>> >>> On 10/23/2014 03:10 PM, Andrey Ryabinin wrote: >>>> >>>> >>>> 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. >>> >>> >>> >>> I'd still suggest to emit __asan_load_noabort so that we match userspace >>> (where __asan_load strictly matches __asan_report in terminating the >>> program). Behavior of __asan_load_noabort can further be restricted by >>> user >>> via various environment settings (kernel parameters, /proc, etc.). >>> >>> @Dmitry: what's your opinion on this? >> >> >> I am somewhat lost in this thread and probably missing something. >> But why do we need __asan_load (which is not noabort) at all? Outline >> instrumentation is non a default mode for both user-space asan and >> kasan (at least in the envisioned future). > > > Well, it's still enabled automatically if number of memory accesses is > getting large enough (I think it was 7000?) so it is default in a way. > >> I would expect that these >> non-typical cases that use outline instrumentation can also bear the >> overhead of non-noreturn functions. > > > As Jakub mentioned both __asan_load and __asan_load_noabort will _not_ be > NORETURN. Two different versions are necessary so that compiler can insert > outline instrumentation that matches the -fsanitize-recover setting. > >> Can we use just one version of >> >> __asan_load and let runtime decide on abort? > > > In this case semantics of inline and outline instrumentation will differ > (the former depending on -fsanitize-recover compile-time setting and the > latter depending on runtime options) which may be undesirable given that > compiler may automatically choose to switch from inline to outline depending > on function size. Ok, thank you, now I am on the same page. Yury, I would expect that the inline instrumentation will become the default in the kernel as well (it's 2 times faster, or maybe even more if happens to affect loop registration). Do you agree? If we consider inline as default, then the user won't be able to simply switch between abort/noabort with a runtime flag w/o rebuilding the kernel, because for __asan_report* we have to have 2 versions. I would also consider that abort/noabort is pretty persistent in a particular testing environment -- you either use one or another and do not switch frequently between them. Taking that into account and the fact that __asan_load* can be emitted due to call threshold, I am mildly in favor of Jakub's position of making it all consistent between user/kernel and load/report and explicit. To make it clear, I mean the runtime interface: __asan_load* -- not-noreturn, always aborts in runtime on failure __asan_load*_noabort - not-noreturn, never aborts in runtime on failure
From 75586eb21f1272a4fcf7c626d9b740eed7150c2c Mon Sep 17 00:00:00 2001 From: Yury Gribov <y.gribov@samsung.com> Date: Wed, 22 Oct 2014 17:24:55 +0400 Subject: [PATCH] Enable -fsanitize-recover for KASan. 2014-10-22 Yury Gribov <y.gribov@samsung.com> gcc/ * asan.c (report_error_func): Add noabort path. (check_func): Formatting. (asan_expand_check_ifn): Handle noabort path. * common.opt (flag_sanitize_recover): Add SANITIZE_KERNEL_ADDRESS to default value. * doc/invoke.texi (-fsanitize-recover=): Mention KASan. * opts.c (finish_options): Reword comment. * sanitizer.def: Add noabort ASan builtins. gcc/testsuite/ * c-c++-common/asan/kasan-recover-1.c: New test. --- gcc/asan.c | 52 ++++++++++++++------- gcc/common.opt | 2 +- gcc/doc/invoke.texi | 8 ++-- gcc/opts.c | 4 +- gcc/sanitizer.def | 38 +++++++++++++++ gcc/testsuite/c-c++-common/asan/kasan-recover-1.c | 12 +++++ 6 files changed, 93 insertions(+), 23 deletions(-) create mode 100644 gcc/testsuite/c-c++-common/asan/kasan-recover-1.c diff --git a/gcc/asan.c b/gcc/asan.c index 97f0b4c..6b04591 100644 --- a/gcc/asan.c +++ b/gcc/asan.c @@ -1344,22 +1344,36 @@ asan_protect_global (tree decl) IS_STORE is either 1 (for a store) or 0 (for a load). */ static tree -report_error_func (bool is_store, HOST_WIDE_INT size_in_bytes, int *nargs) -{ - static enum built_in_function report[2][6] - = { { BUILT_IN_ASAN_REPORT_LOAD1, BUILT_IN_ASAN_REPORT_LOAD2, - BUILT_IN_ASAN_REPORT_LOAD4, BUILT_IN_ASAN_REPORT_LOAD8, - BUILT_IN_ASAN_REPORT_LOAD16, BUILT_IN_ASAN_REPORT_LOAD_N }, - { BUILT_IN_ASAN_REPORT_STORE1, BUILT_IN_ASAN_REPORT_STORE2, - BUILT_IN_ASAN_REPORT_STORE4, BUILT_IN_ASAN_REPORT_STORE8, - BUILT_IN_ASAN_REPORT_STORE16, BUILT_IN_ASAN_REPORT_STORE_N } }; +report_error_func (bool is_store, bool recover_p, HOST_WIDE_INT size_in_bytes, + int *nargs) +{ + static enum built_in_function report[2][2][6] + = { { { BUILT_IN_ASAN_REPORT_LOAD1, BUILT_IN_ASAN_REPORT_LOAD2, + BUILT_IN_ASAN_REPORT_LOAD4, BUILT_IN_ASAN_REPORT_LOAD8, + BUILT_IN_ASAN_REPORT_LOAD16, BUILT_IN_ASAN_REPORT_LOAD_N }, + { BUILT_IN_ASAN_REPORT_STORE1, BUILT_IN_ASAN_REPORT_STORE2, + BUILT_IN_ASAN_REPORT_STORE4, BUILT_IN_ASAN_REPORT_STORE8, + BUILT_IN_ASAN_REPORT_STORE16, BUILT_IN_ASAN_REPORT_STORE_N } }, + { { BUILT_IN_ASAN_REPORT_LOAD1_NOABORT, + BUILT_IN_ASAN_REPORT_LOAD2_NOABORT, + BUILT_IN_ASAN_REPORT_LOAD4_NOABORT, + BUILT_IN_ASAN_REPORT_LOAD8_NOABORT, + BUILT_IN_ASAN_REPORT_LOAD16_NOABORT, + BUILT_IN_ASAN_REPORT_LOAD_N_NOABORT }, + { BUILT_IN_ASAN_REPORT_STORE1_NOABORT, + BUILT_IN_ASAN_REPORT_STORE2_NOABORT, + BUILT_IN_ASAN_REPORT_STORE4_NOABORT, + BUILT_IN_ASAN_REPORT_STORE8_NOABORT, + BUILT_IN_ASAN_REPORT_STORE16_NOABORT, + BUILT_IN_ASAN_REPORT_STORE_N_NOABORT } } }; if (size_in_bytes == -1) { *nargs = 2; - return builtin_decl_implicit (report[is_store][5]); + return builtin_decl_implicit (report[recover_p][is_store][5]); } *nargs = 1; - return builtin_decl_implicit (report[is_store][exact_log2 (size_in_bytes)]); + int size_log2 = exact_log2 (size_in_bytes); + return builtin_decl_implicit (report[recover_p][is_store][size_log2]); } /* Construct a function tree for __asan_{load,store}{1,2,4,8,16,_n}. @@ -1370,11 +1384,11 @@ check_func (bool is_store, int size_in_bytes, int *nargs) { static enum built_in_function check[2][6] = { { BUILT_IN_ASAN_LOAD1, BUILT_IN_ASAN_LOAD2, - BUILT_IN_ASAN_LOAD4, BUILT_IN_ASAN_LOAD8, - BUILT_IN_ASAN_LOAD16, BUILT_IN_ASAN_LOADN }, + BUILT_IN_ASAN_LOAD4, BUILT_IN_ASAN_LOAD8, + BUILT_IN_ASAN_LOAD16, BUILT_IN_ASAN_LOADN }, { BUILT_IN_ASAN_STORE1, BUILT_IN_ASAN_STORE2, - BUILT_IN_ASAN_STORE4, BUILT_IN_ASAN_STORE8, - BUILT_IN_ASAN_STORE16, BUILT_IN_ASAN_STOREN } }; + BUILT_IN_ASAN_STORE4, BUILT_IN_ASAN_STORE8, + BUILT_IN_ASAN_STORE16, BUILT_IN_ASAN_STOREN } }; if (size_in_bytes == -1) { *nargs = 2; @@ -2589,9 +2603,11 @@ asan_expand_check_ifn (gimple_stmt_iterator *iter, bool use_calls) /* Get an iterator on the point where we can add the condition statement for the instrumentation. */ basic_block then_bb, else_bb; + bool create_then_fallthru_edge + = (flag_sanitize & flag_sanitize_recover & SANITIZE_KERNEL_ADDRESS) != 0; gsi = create_cond_insert_point (&gsi, /*before_p*/false, /*then_more_likely_p=*/false, - /*create_then_fallthru_edge=*/false, + create_then_fallthru_edge, &then_bb, &else_bb); @@ -2700,7 +2716,9 @@ asan_expand_check_ifn (gimple_stmt_iterator *iter, bool use_calls) /* Generate call to the run-time library (e.g. __asan_report_load8). */ gsi = gsi_start_bb (then_bb); int nargs; - tree fun = report_error_func (is_store, size_in_bytes, &nargs); + bool recover_p + = (flag_sanitize & flag_sanitize_recover & SANITIZE_KERNEL_ADDRESS) != 0; + tree fun = report_error_func (is_store, recover_p, size_in_bytes, &nargs); g = gimple_build_call (fun, nargs, base_addr, len); gimple_set_location (g, loc); gsi_insert_after (&gsi, g, GSI_NEW_STMT); diff --git a/gcc/common.opt b/gcc/common.opt index da5250b..e385615 100644 --- a/gcc/common.opt +++ b/gcc/common.opt @@ -213,7 +213,7 @@ unsigned int flag_sanitize ; What sanitizers should recover from errors Variable -unsigned int flag_sanitize_recover = SANITIZE_UNDEFINED | SANITIZE_NONDEFAULT +unsigned int flag_sanitize_recover = SANITIZE_UNDEFINED | SANITIZE_NONDEFAULT | SANITIZE_KERNEL_ADDRESS ; Flag whether a prefix has been added to dump_base_name Variable diff --git a/gcc/doc/invoke.texi b/gcc/doc/invoke.texi index c9ca404..6a425c0 100644 --- a/gcc/doc/invoke.texi +++ b/gcc/doc/invoke.texi @@ -5657,13 +5657,13 @@ and program will exit after that with non-zero exit code. Currently this feature only works for @option{-fsanitize=undefined} (and its suboptions except for @option{-fsanitize=unreachable} and @option{-fsanitize=return}), -@option{-fsanitize=float-cast-overflow} and @option{-fsanitize=float-divide-by-zero}. -For these sanitizers error recovery is turned on by default. +@option{-fsanitize=float-cast-overflow}, @option{-fsanitize=float-divide-by-zero} and +@option{-fsanitize=kernel-address}. For these sanitizers error recovery is turned on by default. Syntax without explicit @var{opts} parameter is deprecated. It is equivalent to -@option{-fsanitize-recover=undefined,float-cast-overflow,float-divide-by-zero}. +@option{-fsanitize-recover=undefined,float-cast-overflow,float-divide-by-zero,kernel-address}. Similarly @option{-fno-sanitize-recover} is equivalent to -@option{-fno-sanitize-recover=undefined,float-cast-overflow,float-divide-by-zero}. +@option{-fno-sanitize-recover=undefined,float-cast-overflow,float-divide-by-zero,kernel-address}. @item -fsanitize-undefined-trap-on-error @opindex fsanitize-undefined-trap-on-error diff --git a/gcc/opts.c b/gcc/opts.c index 25f5235..7157865 100644 --- a/gcc/opts.c +++ b/gcc/opts.c @@ -877,7 +877,7 @@ finish_options (struct gcc_options *opts, struct gcc_options *opts_set, if (opts->x_dwarf_split_debug_info) opts->x_debug_generate_pub_sections = 2; - /* Userspace and kernel ASan conflict with each other and with TSan. */ + /* Userspace and kernel ASan conflict with each other. */ if ((opts->x_flag_sanitize & SANITIZE_USER_ADDRESS) && (opts->x_flag_sanitize & SANITIZE_KERNEL_ADDRESS)) @@ -885,6 +885,8 @@ finish_options (struct gcc_options *opts, struct gcc_options *opts_set, "-fsanitize=address is incompatible with " "-fsanitize=kernel-address"); + /* And with TSan. */ + if ((opts->x_flag_sanitize & SANITIZE_ADDRESS) && (opts->x_flag_sanitize & SANITIZE_THREAD)) error_at (loc, diff --git a/gcc/sanitizer.def b/gcc/sanitizer.def index 722311a..63c6e6d 100644 --- a/gcc/sanitizer.def +++ b/gcc/sanitizer.def @@ -57,6 +57,44 @@ DEF_SANITIZER_BUILTIN(BUILT_IN_ASAN_REPORT_STORE16, "__asan_report_store16", DEF_SANITIZER_BUILTIN(BUILT_IN_ASAN_REPORT_STORE_N, "__asan_report_store_n", BT_FN_VOID_PTR_PTRMODE, ATTR_TMPURE_NORETURN_NOTHROW_LEAF_LIST) +DEF_SANITIZER_BUILTIN(BUILT_IN_ASAN_REPORT_LOAD1_NOABORT, + "__asan_report_load1_noabort", + BT_FN_VOID_PTR, ATTR_TMPURE_NOTHROW_LEAF_LIST) +DEF_SANITIZER_BUILTIN(BUILT_IN_ASAN_REPORT_LOAD2_NOABORT, + "__asan_report_load2_noabort", + BT_FN_VOID_PTR, ATTR_TMPURE_NOTHROW_LEAF_LIST) +DEF_SANITIZER_BUILTIN(BUILT_IN_ASAN_REPORT_LOAD4_NOABORT, + "__asan_report_load4_noabort", + BT_FN_VOID_PTR, ATTR_TMPURE_NOTHROW_LEAF_LIST) +DEF_SANITIZER_BUILTIN(BUILT_IN_ASAN_REPORT_LOAD8_NOABORT, + "__asan_report_load8_noabort", + BT_FN_VOID_PTR, ATTR_TMPURE_NOTHROW_LEAF_LIST) +DEF_SANITIZER_BUILTIN(BUILT_IN_ASAN_REPORT_LOAD16_NOABORT, + "__asan_report_load16_noabort", + BT_FN_VOID_PTR, ATTR_TMPURE_NOTHROW_LEAF_LIST) +DEF_SANITIZER_BUILTIN(BUILT_IN_ASAN_REPORT_LOAD_N_NOABORT, + "__asan_report_load_n_noabort", + BT_FN_VOID_PTR_PTRMODE, + ATTR_TMPURE_NOTHROW_LEAF_LIST) +DEF_SANITIZER_BUILTIN(BUILT_IN_ASAN_REPORT_STORE1_NOABORT, + "__asan_report_store1_noabort", + BT_FN_VOID_PTR, ATTR_TMPURE_NOTHROW_LEAF_LIST) +DEF_SANITIZER_BUILTIN(BUILT_IN_ASAN_REPORT_STORE2_NOABORT, + "__asan_report_store2_noabort", + BT_FN_VOID_PTR, ATTR_TMPURE_NOTHROW_LEAF_LIST) +DEF_SANITIZER_BUILTIN(BUILT_IN_ASAN_REPORT_STORE4_NOABORT, + "__asan_report_store4_noabort", + BT_FN_VOID_PTR, ATTR_TMPURE_NOTHROW_LEAF_LIST) +DEF_SANITIZER_BUILTIN(BUILT_IN_ASAN_REPORT_STORE8_NOABORT, + "__asan_report_store8_noabort", + BT_FN_VOID_PTR, ATTR_TMPURE_NOTHROW_LEAF_LIST) +DEF_SANITIZER_BUILTIN(BUILT_IN_ASAN_REPORT_STORE16_NOABORT, + "__asan_report_store16_noabort", + BT_FN_VOID_PTR, ATTR_TMPURE_NOTHROW_LEAF_LIST) +DEF_SANITIZER_BUILTIN(BUILT_IN_ASAN_REPORT_STORE_N_NOABORT, + "__asan_report_store_n_noabort", + BT_FN_VOID_PTR_PTRMODE, + ATTR_TMPURE_NOTHROW_LEAF_LIST) DEF_SANITIZER_BUILTIN(BUILT_IN_ASAN_LOAD1, "__asan_load1", BT_FN_VOID_PTR, ATTR_TMPURE_NOTHROW_LEAF_LIST) DEF_SANITIZER_BUILTIN(BUILT_IN_ASAN_LOAD2, "__asan_load2", diff --git a/gcc/testsuite/c-c++-common/asan/kasan-recover-1.c b/gcc/testsuite/c-c++-common/asan/kasan-recover-1.c new file mode 100644 index 0000000..6e04e5d --- /dev/null +++ b/gcc/testsuite/c-c++-common/asan/kasan-recover-1.c @@ -0,0 +1,12 @@ +/* { dg-do compile } */ +/* { dg-options "-fno-sanitize=address -fsanitize=kernel-address --param asan-instrumentation-with-call-threshold=100 -save-temps" } */ + +void +foo (int *p) +{ + *p = 0; +} + +/* { dg-final { scan-assembler "__asan_report_store4_noabort" } } */ +/* { dg-final { cleanup-saved-temps } } */ + -- 1.7.9.5