[PATCH v2] KASAN: prohibit KASAN+STRUCTLEAK combination

[PATCH v2] KASAN: prohibit KASAN+STRUCTLEAK combination

[Dmitry Vyukov]

Currently STRUCTLEAK inserts initialization out of live scope of
variables from KASAN point of view. This leads to KASAN false
positive reports. Prohibit this combination for now.

Signed-off-by: Dmitry Vyukov <dvyukov@google.com>
Cc: linux-mm@kvack.org
Cc: kasan-dev@googlegroups.com
Cc: Fengguang Wu <fengguang.wu@intel.com>
Cc: Sergey Senozhatsky <sergey.senozhatsky.work@gmail.com>
Cc: Andrey Ryabinin <aryabinin@virtuozzo.com>
Cc: Kees Cook <keescook@google.com>

---

This combination leads to periodic confusion
and pointless debugging:

https://marc.info/?l=linux-kernel&m=151991367323082
https://marc.info/?l=linux-kernel&m=151992229326243
https://lkml.org/lkml/2017/11/30/33

Changes since v1:
 - replace KASAN with KASAN_EXTRA
   Only KASAN_EXTRA enables variable scope checking
---
 arch/Kconfig | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/arch/Kconfig b/arch/Kconfig
index 8e0d665c8d53..75dd23acf133 100644
--- a/arch/Kconfig
+++ b/arch/Kconfig
@@ -464,6 +464,10 @@ config GCC_PLUGIN_LATENT_ENTROPY
 config GCC_PLUGIN_STRUCTLEAK
 	bool "Force initialization of variables containing userspace addresses"
 	depends on GCC_PLUGINS
+	# Currently STRUCTLEAK inserts initialization out of live scope of
+	# variables from KASAN point of view. This leads to KASAN false
+	# positive reports. Prohibit this combination for now.
+	depends on !KASAN_EXTRA
 	help
 	  This plugin zero-initializes any structures containing a
 	  __user attribute. This can prevent some classes of information
-- 
2.17.0.484.g0c8726318c-goog

Re: [PATCH v2] KASAN: prohibit KASAN+STRUCTLEAK combination

[Kees Cook]

On Thu, Apr 19, 2018 at 10:24 AM, Dmitry Vyukov <dvyukov@google.com> wrote:
> Currently STRUCTLEAK inserts initialization out of live scope of
> variables from KASAN point of view. This leads to KASAN false
> positive reports. Prohibit this combination for now.
>
> Signed-off-by: Dmitry Vyukov <dvyukov@google.com>
> Cc: linux-mm@kvack.org
> Cc: kasan-dev@googlegroups.com
> Cc: Fengguang Wu <fengguang.wu@intel.com>
> Cc: Sergey Senozhatsky <sergey.senozhatsky.work@gmail.com>
> Cc: Andrey Ryabinin <aryabinin@virtuozzo.com>
> Cc: Kees Cook <keescook@google.com>

This seems fine until we have a better solution. Thanks!

Acked-by: Kees Cook <keescook@chromium.org>

-Kees

>
> ---
>
> This combination leads to periodic confusion
> and pointless debugging:
>
> https://marc.info/?l=linux-kernel&m=151991367323082
> https://marc.info/?l=linux-kernel&m=151992229326243
> https://lkml.org/lkml/2017/11/30/33
>
> Changes since v1:
>  - replace KASAN with KASAN_EXTRA
>    Only KASAN_EXTRA enables variable scope checking
> ---
>  arch/Kconfig | 4 ++++
>  1 file changed, 4 insertions(+)
>
> diff --git a/arch/Kconfig b/arch/Kconfig
> index 8e0d665c8d53..75dd23acf133 100644
> --- a/arch/Kconfig
> +++ b/arch/Kconfig
> @@ -464,6 +464,10 @@ config GCC_PLUGIN_LATENT_ENTROPY
>  config GCC_PLUGIN_STRUCTLEAK
>         bool "Force initialization of variables containing userspace addresses"
>         depends on GCC_PLUGINS
> +       # Currently STRUCTLEAK inserts initialization out of live scope of
> +       # variables from KASAN point of view. This leads to KASAN false
> +       # positive reports. Prohibit this combination for now.
> +       depends on !KASAN_EXTRA
>         help
>           This plugin zero-initializes any structures containing a
>           __user attribute. This can prevent some classes of information
> --
> 2.17.0.484.g0c8726318c-goog
>



-- 
Kees Cook
Pixel Security

Re: [PATCH v2] KASAN: prohibit KASAN+STRUCTLEAK combination

[Dennis Zhou]

Hi,

On Thu, Apr 19, 2018 at 01:43:11PM -0700, Kees Cook wrote:
> On Thu, Apr 19, 2018 at 10:24 AM, Dmitry Vyukov <dvyukov@google.com> wrote:
> > Currently STRUCTLEAK inserts initialization out of live scope of
> > variables from KASAN point of view. This leads to KASAN false
> > positive reports. Prohibit this combination for now.
> >

I remember looking at this false positive in November. Please bear with
me as this is my first time digging through into gcc. It seems address
sanitizing is a process that starts adding instructions in the ompexp
pass, with I presume additional passes later doing other things.

It seems the structleak plugin isn't respecting the ASAN markings as it
also runs after ASAN starts adding instructions and before inlining.
Thus, the use-after-scope bugs from [1] and [2] get triggered by
subsequent iterations when looping over an inlined building block.

Would it be possible to run the structleak plugin say before
"*all_optimizations" instead of "early_optimizations"? Doing so has the
plugin run after inlining has been done placing initialization code in
an earlier block that is not a part of the loop. This seems to resolve
the issue for the latest one from [1] and the November repro case I had
in [2].

[1] https://lkml.org/lkml/2018/4/18/825
[2] https://lkml.org/lkml/2017/11/29/868

Thanks,
Dennis

--------
diff --git a/scripts/gcc-plugins/structleak_plugin.c b/scripts/gcc-plugins/structleak_plugin.c
index 10292f7..0061040 100644
--- a/scripts/gcc-plugins/structleak_plugin.c
+++ b/scripts/gcc-plugins/structleak_plugin.c
@@ -211,7 +211,7 @@ __visible int plugin_init(struct plugin_name_args *plugin_info, struct plugin_gc
 	const struct plugin_argument * const argv = plugin_info->argv;
 	bool enable = true;
 
-	PASS_INFO(structleak, "early_optimizations", 1, PASS_POS_INSERT_BEFORE);
+	PASS_INFO(structleak, "*all_optimizations", 1, PASS_POS_INSERT_BEFORE);
 
 	if (!plugin_default_version_check(version, &gcc_version)) {
 		error(G_("incompatible gcc/plugin versions"));

Re: [PATCH v2] KASAN: prohibit KASAN+STRUCTLEAK combination

[Dmitry Vyukov]

On Fri, Apr 20, 2018 at 7:33 AM, Dennis Zhou <dennisszhou@gmail.com> wrote:
> Hi,
>
> On Thu, Apr 19, 2018 at 01:43:11PM -0700, Kees Cook wrote:
>> On Thu, Apr 19, 2018 at 10:24 AM, Dmitry Vyukov <dvyukov@google.com> wrote:
>> > Currently STRUCTLEAK inserts initialization out of live scope of
>> > variables from KASAN point of view. This leads to KASAN false
>> > positive reports. Prohibit this combination for now.
>> >
>
> I remember looking at this false positive in November. Please bear with
> me as this is my first time digging through into gcc. It seems address
> sanitizing is a process that starts adding instructions in the ompexp
> pass, with I presume additional passes later doing other things.
>
> It seems the structleak plugin isn't respecting the ASAN markings as it
> also runs after ASAN starts adding instructions and before inlining.
> Thus, the use-after-scope bugs from [1] and [2] get triggered by
> subsequent iterations when looping over an inlined building block.
>
> Would it be possible to run the structleak plugin say before
> "*all_optimizations" instead of "early_optimizations"? Doing so has the
> plugin run after inlining has been done placing initialization code in
> an earlier block that is not a part of the loop. This seems to resolve
> the issue for the latest one from [1] and the November repro case I had
> in [2].


In general, we either need to move the structleak pass or make it
insert instructions at proper locations. I don't know what is the
right way. Moving the pass looks easier.

As a sanity check, I would count number of zeroing inserted by the
plugin it both cases and ensure that now it does not insert order of
magnitude more/less. It's easy with function calls (count them in
objdump output), not sure what's the easiest way to do it for inline
instrumentation. We could insert printf into the pass itself, but it
if runs before inlining and other optimization, it's not the final
number.

Also note that asan pass is at different locations in the pipeline
depending on optimization level:
https://gcc.gnu.org/viewcvs/gcc/trunk/gcc/passes.def?view=markup


> [1] https://lkml.org/lkml/2018/4/18/825
> [2] https://lkml.org/lkml/2017/11/29/868
>
> Thanks,
> Dennis
>
> --------
> diff --git a/scripts/gcc-plugins/structleak_plugin.c b/scripts/gcc-plugins/structleak_plugin.c
> index 10292f7..0061040 100644
> --- a/scripts/gcc-plugins/structleak_plugin.c
> +++ b/scripts/gcc-plugins/structleak_plugin.c
> @@ -211,7 +211,7 @@ __visible int plugin_init(struct plugin_name_args *plugin_info, struct plugin_gc
>         const struct plugin_argument * const argv = plugin_info->argv;
>         bool enable = true;
>
> -       PASS_INFO(structleak, "early_optimizations", 1, PASS_POS_INSERT_BEFORE);
> +       PASS_INFO(structleak, "*all_optimizations", 1, PASS_POS_INSERT_BEFORE);
>
>         if (!plugin_default_version_check(version, &gcc_version)) {
>                 error(G_("incompatible gcc/plugin versions"));

Re: [PATCH v2] KASAN: prohibit KASAN+STRUCTLEAK combination

[Dennis Zhou]

Hi,

On Fri, Apr 20, 2018 at 07:56:56AM +0200, Dmitry Vyukov wrote:
> As a sanity check, I would count number of zeroing inserted by the
> plugin it both cases and ensure that now it does not insert order of
> magnitude more/less. It's easy with function calls (count them in
> objdump output), not sure what's the easiest way to do it for inline
> instrumentation. We could insert printf into the pass itself, but it
> if runs before inlining and other optimization, it's not the final
> number.

I modified the structleak_plugin to count the number of initializations
and output if the function was an inline function or not. The aggregated
values are below.

declared inline       no       yes
----------------------------------
early_optimizations:  12168   7114
*all_optimizations:   12554     13

These numbers seem appropriate. The structleak initializes in declared
inline functions are redundant.

> Also note that asan pass is at different locations in the pipeline
> depending on optimization level:
> https://gcc.gnu.org/viewcvs/gcc/trunk/gcc/passes.def?view=markup

The *all_optimizations pass happens before any of the asan pass
locations so I think this shouldn't change those semantics.

Thanks,
Dennis

Re: [PATCH v2] KASAN: prohibit KASAN+STRUCTLEAK combination

[Kees Cook]

On Sat, Apr 21, 2018 at 2:06 PM, Dennis Zhou <dennisszhou@gmail.com> wrote:
> Hi,
>
> On Fri, Apr 20, 2018 at 07:56:56AM +0200, Dmitry Vyukov wrote:
>> As a sanity check, I would count number of zeroing inserted by the
>> plugin it both cases and ensure that now it does not insert order of
>> magnitude more/less. It's easy with function calls (count them in
>> objdump output), not sure what's the easiest way to do it for inline
>> instrumentation. We could insert printf into the pass itself, but it
>> if runs before inlining and other optimization, it's not the final
>> number.
>
> I modified the structleak_plugin to count the number of initializations
> and output if the function was an inline function or not. The aggregated
> values are below.
>
> declared inline       no       yes
> ----------------------------------
> early_optimizations:  12168   7114
> *all_optimizations:   12554     13
>
> These numbers seem appropriate. The structleak initializes in declared
> inline functions are redundant.

Does this mean we end up with redundant initializers, or are they
optimized away in later passes?

-Kees

-- 
Kees Cook
Pixel Security

Re: [PATCH v2] KASAN: prohibit KASAN+STRUCTLEAK combination

[Dennis Zhou]

On Sat, Apr 21, 2018 at 02:13:30PM -0700, Kees Cook wrote:
> Does this mean we end up with redundant initializers, or are they
> optimized away in later passes?

I believe the plugin results in redundant initializers because the early
inline phase puts the appropriate declarations in the caller's scope.
I guess updating the inline function to have an initializer propagates
the duplicate initializer. I don't understand the complete interactions
here, but this is what I'm seeing. I also can't comment on why they
aren't being optimized out, but I assume it's because they live in
different basic blocks.

By waiting to do it after inlining is done, the inlined functions are
not modified to have initializers as the function that uses the inlined
function should have the initializing code.

Thanks,
Dennis

Re: [PATCH v2] KASAN: prohibit KASAN+STRUCTLEAK combination

[Kees Cook]

On Thu, Apr 19, 2018 at 10:24 AM, Dmitry Vyukov <dvyukov@google.com> wrote:
> Currently STRUCTLEAK inserts initialization out of live scope of
> variables from KASAN point of view. This leads to KASAN false
> positive reports. Prohibit this combination for now.
>
> Signed-off-by: Dmitry Vyukov <dvyukov@google.com>
> Cc: linux-mm@kvack.org
> Cc: kasan-dev@googlegroups.com
> Cc: Fengguang Wu <fengguang.wu@intel.com>
> Cc: Sergey Senozhatsky <sergey.senozhatsky.work@gmail.com>
> Cc: Andrey Ryabinin <aryabinin@virtuozzo.com>
> Cc: Kees Cook <keescook@google.com>

Acked-by: Kees Cook <keescook@chromium.org>

I prefer this change over moving the plugin earlier since that ends up
creating redundant initializers...

Andrew, can you carry this (and possibly include it in bug-fixes for v4.17)?

Thanks!

-Kees

>
> ---
>
> This combination leads to periodic confusion
> and pointless debugging:
>
> https://marc.info/?l=linux-kernel&m=151991367323082
> https://marc.info/?l=linux-kernel&m=151992229326243
> https://lkml.org/lkml/2017/11/30/33
>
> Changes since v1:
>  - replace KASAN with KASAN_EXTRA
>    Only KASAN_EXTRA enables variable scope checking
> ---
>  arch/Kconfig | 4 ++++
>  1 file changed, 4 insertions(+)
>
> diff --git a/arch/Kconfig b/arch/Kconfig
> index 8e0d665c8d53..75dd23acf133 100644
> --- a/arch/Kconfig
> +++ b/arch/Kconfig
> @@ -464,6 +464,10 @@ config GCC_PLUGIN_LATENT_ENTROPY
>  config GCC_PLUGIN_STRUCTLEAK
>         bool "Force initialization of variables containing userspace addresses"
>         depends on GCC_PLUGINS
> +       # Currently STRUCTLEAK inserts initialization out of live scope of
> +       # variables from KASAN point of view. This leads to KASAN false
> +       # positive reports. Prohibit this combination for now.
> +       depends on !KASAN_EXTRA
>         help
>           This plugin zero-initializes any structures containing a
>           __user attribute. This can prevent some classes of information
> --
> 2.17.0.484.g0c8726318c-goog
>



-- 
Kees Cook
Pixel Security

Re: [PATCH v2] KASAN: prohibit KASAN+STRUCTLEAK combination

[Dennis Zhou]

Hi Kees,

On Mon, Apr 30, 2018 at 04:41:24PM -0700, Kees Cook wrote:
> I prefer this change over moving the plugin earlier since that ends up
> creating redundant initializers...

To be clear, what I was proposing was to move the plugin to execute
later rather than earlier. It currently runs before the
early_optimizations pass, while *all_optimizations is after inlining.
Apologizes for this being a half baked idea due to my limited
understanding.

I am hoping someone could chime in and help me understand how gcc
handles inlining. My assumption is that at the beginning, inlined
defined functions will be processed by the pass as any other function.
If the function can be inlined, it is inlined and no longer needs to be
kept around. If it cannot be inlined, it is kept around. An assumption
that I'm not sure is correct is that a function is either always inlined
or not inlined in a translation unit.

The current plugin puts an initializer in both the inlined function and
the locations that it will be inlined as both functions are around,
hence duplicate initializers. Below is a snippet of pass output from
earlier reproducing code of the issue.

My understanding is initializer 1 is created due to inlining moving
variable declarations to the encompassing functions scope. Then the
structleak_plugin performs the pass not finding an initializer and
creates one. Initializer 2 is created for the inlined function and is
propagated. So I guess this problem is also order dependent in which the
functions are processed.

An important difference in running in a later pass, which may be a deal
breaker, is that objects will only be initialized once. So if a function
gets inlined inside a for loop, the initializer will only be a part of
the encompassing function rather than also in each iteration. In the
example below, initializer 2 would not be there as the inlined function
wouldn't be around and processed by the structleak_plugin.

Thanks for taking the time to humor me, this is the extent of my
understanding of the problem and gcc.

Thanks,
Dennis

------

union
{
struct list_head * __val;
char __c[1];
} __u;

<bb 2> [0.00%]:
__u = {};    <---- initializer 1
p_8 = malloc (160);
i_9 = 0;
goto <bb 10>; [0.00%]

<bb 3> [0.00%]:
_1 = (long unsigned int) i_4;
_2 = _1 * 16;
_3 = p_8 + _2;
list_14 = _3;
__u = {};    <---- initializer 2
ASAN_MARK (UNPOISON, &__u, 8);