[PATCH] structleak: disable BYREF_ALL in combination with KASAN_STACK

[PATCH] structleak: disable BYREF_ALL in combination with KASAN_STACK

[Arnd Bergmann]

The combination of KASAN_STACK and GCC_PLUGIN_STRUCTLEAK_BYREF_ALL
leads to much larger kernel stack usage, as seen from the warnings
about functions that now exceed the 2048 byte limit:

drivers/media/i2c/tvp5150.c:253:1: error: the frame size of 3936 bytes is larger than 2048 bytes
fs/ocfs2/dlm/dlmrecovery.c:737:1: error: the frame size of 2088 bytes is larger than 2048 bytes
fs/ocfs2/aops.c:1892:1: error: the frame size of 2088 bytes is larger than 2048 bytes
fs/ocfs2/namei.c:1677:1: error: the frame size of 2584 bytes is larger than 2048 bytes
fs/ocfs2/super.c:1186:1: error: the frame size of 2640 bytes is larger than 2048 bytes
fs/ocfs2/xattr.c:3678:1: error: the frame size of 2176 bytes is larger than 2048 bytes
net/bridge/br_netlink.c:1505:1: error: the frame size of 2448 bytes is larger than 2048 bytes
net/ieee802154/nl802154.c:548:1: error: the frame size of 2232 bytes is larger than 2048 bytes
net/wireless/nl80211.c:1726:1: error: the frame size of 2224 bytes is larger than 2048 bytes
net/wireless/nl80211.c:6472:1: error: the frame size of 2112 bytes is larger than 2048 bytes
net/wireless/nl80211.c:2357:1: error: the frame size of 4584 bytes is larger than 2048 bytes
net/wireless/nl80211.c:5108:1: error: the frame size of 2760 bytes is larger than 2048 bytes
drivers/media/tuners/r820t.c:1327:1: error: the frame size of 2816 bytes is larger than 2048 bytes

The warnings are distracting, and risking a kernel stack overflow is
generally not beneficial to performance, so it may be best to disallow
that particular combination. This can be done by turning off either
one. I picked the dependency in GCC_PLUGIN_STRUCTLEAK_BYREF_ALL, as
this option is designed to make uninitialized stack usage less harmful
when enabled on its own, but it also prevents KASAN from detecting those
cases in which it was in fact needed.

KASAN_STACK is currently implied by KASAN on gcc, but could be made a
user selectable option if we want to allow combining (non-stack) KASAN
wtih GCC_PLUGIN_STRUCTLEAK_BYREF_ALL.

Note that it woult be possible to specifically address the files that
print the warning, but presumably the overall stack usage is still
significantly higher than in other configurations, so this would not
address the full problem.

I could not test this with CONFIG_INIT_STACK_ALL, which may or may not
suffer from a similar problem.

Fixes: 81a56f6dcd20 ("gcc-plugins: structleak: Generalize to all variable types")
Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
 security/Kconfig.hardening | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/security/Kconfig.hardening b/security/Kconfig.hardening
index c6cb2d9b2905..e742d1006a4b 100644
--- a/security/Kconfig.hardening
+++ b/security/Kconfig.hardening
@@ -73,6 +73,7 @@ choice
 	config GCC_PLUGIN_STRUCTLEAK_BYREF_ALL
 		bool "zero-init anything passed by reference (very strong)"
 		depends on GCC_PLUGINS
+		depends on !(KASAN && KASAN_STACK=1)
 		select GCC_PLUGIN_STRUCTLEAK
 		help
 		  Zero-initialize any stack variables that may be passed
@@ -80,6 +81,10 @@ choice
 		  initialized. This is intended to eliminate all classes
 		  of uninitialized stack variable exploits and information
 		  exposures.
+		  As a side-effect, this keeps a lot of variables on the
+		  stack that can otherwise be optimized out, so combining
+		  this with CONFIG_KASAN_STACK can lead to a stack overflow
+		  and is disallowed.
 
 	config INIT_STACK_ALL
 		bool "0xAA-init everything on the stack (strongest)"
-- 
2.20.0

Re: [PATCH] structleak: disable BYREF_ALL in combination with KASAN_STACK

[Kees Cook]

On Tue, Jun 18, 2019 at 11:47:13AM +0200, Arnd Bergmann wrote:
> The combination of KASAN_STACK and GCC_PLUGIN_STRUCTLEAK_BYREF_ALL
> leads to much larger kernel stack usage, as seen from the warnings
> about functions that now exceed the 2048 byte limit:

Is the preference that this go into v5.2 (there's not much time left),
or should this be v5.3? (You didn't mark it as Cc: stable?)

> one. I picked the dependency in GCC_PLUGIN_STRUCTLEAK_BYREF_ALL, as
> this option is designed to make uninitialized stack usage less harmful
> when enabled on its own, but it also prevents KASAN from detecting those
> cases in which it was in fact needed.

Right -- there's not much sense in both being enabled. I'd agree with
this rationale.

-- 
Kees Cook

Re: [PATCH] structleak: disable BYREF_ALL in combination with KASAN_STACK

[Arnd Bergmann]

On Thu, Jun 20, 2019 at 7:36 PM Kees Cook <keescook@chromium.org> wrote:
>
> On Tue, Jun 18, 2019 at 11:47:13AM +0200, Arnd Bergmann wrote:
> > The combination of KASAN_STACK and GCC_PLUGIN_STRUCTLEAK_BYREF_ALL
> > leads to much larger kernel stack usage, as seen from the warnings
> > about functions that now exceed the 2048 byte limit:
>
> Is the preference that this go into v5.2 (there's not much time left),
> or should this be v5.3? (You didn't mark it as Cc: stable?)

Having it in 5.2 would be great. I had not done much build testing in the last
months, so I didn't actually realize that your patch was merged a while ago
rather than only in linux-next.

BTW, I have now run into a small number of files that are still affected
by a stack overflow warning from STRUCTLEAK_BYREF_ALL. I'm trying
to come up with patches for those as well, we can probably do it in a way
that also improves the affected drivers. I'll put you on Cc when I
find another one.

      Arnd

Re: [PATCH] structleak: disable BYREF_ALL in combination with KASAN_STACK

[Ard Biesheuvel]

On Fri, 21 Jun 2019 at 11:44, Arnd Bergmann <arnd@arndb.de> wrote:
>
> On Thu, Jun 20, 2019 at 7:36 PM Kees Cook <keescook@chromium.org> wrote:
> >
> > On Tue, Jun 18, 2019 at 11:47:13AM +0200, Arnd Bergmann wrote:
> > > The combination of KASAN_STACK and GCC_PLUGIN_STRUCTLEAK_BYREF_ALL
> > > leads to much larger kernel stack usage, as seen from the warnings
> > > about functions that now exceed the 2048 byte limit:
> >
> > Is the preference that this go into v5.2 (there's not much time left),
> > or should this be v5.3? (You didn't mark it as Cc: stable?)
>
> Having it in 5.2 would be great. I had not done much build testing in the last
> months, so I didn't actually realize that your patch was merged a while ago
> rather than only in linux-next.
>
> BTW, I have now run into a small number of files that are still affected
> by a stack overflow warning from STRUCTLEAK_BYREF_ALL. I'm trying
> to come up with patches for those as well, we can probably do it in a way
> that also improves the affected drivers. I'll put you on Cc when I
> find another one.
>

There is something fundamentally wrong here, though. BYREF_ALL only
initializes variables that have their address taken, which does not
explain why the size of the stack frame should increase (since in
order to have an address in the first place, the variable must already
have a stack slot assigned)

So I suspect that BYREF_ALL is defeating some optimizations where.
e.g., the call involving the address of the variable is optimized
away, but the the initialization remains, thus forcing the variable to
be allocated in the stack frame even though the initializer is the
only thing that references it.

Re: [PATCH] structleak: disable BYREF_ALL in combination with KASAN_STACK

[Arnd Bergmann]

On Fri, Jun 21, 2019 at 3:32 PM Ard Biesheuvel
<ard.biesheuvel@linaro.org> wrote:
> On Fri, 21 Jun 2019 at 11:44, Arnd Bergmann <arnd@arndb.de> wrote:
> > On Thu, Jun 20, 2019 at 7:36 PM Kees Cook <keescook@chromium.org> wrote:
> > > On Tue, Jun 18, 2019 at 11:47:13AM +0200, Arnd Bergmann wrote:
> > > > The combination of KASAN_STACK and GCC_PLUGIN_STRUCTLEAK_BYREF_ALL
> > > > leads to much larger kernel stack usage, as seen from the warnings
> > > > about functions that now exceed the 2048 byte limit:
> > >
> > > Is the preference that this go into v5.2 (there's not much time left),
> > > or should this be v5.3? (You didn't mark it as Cc: stable?)
> >
> > Having it in 5.2 would be great. I had not done much build testing in the last
> > months, so I didn't actually realize that your patch was merged a while ago
> > rather than only in linux-next.
> >
> > BTW, I have now run into a small number of files that are still affected
> > by a stack overflow warning from STRUCTLEAK_BYREF_ALL. I'm trying
> > to come up with patches for those as well, we can probably do it in a way
> > that also improves the affected drivers. I'll put you on Cc when I
> > find another one.
> >
>
> There is something fundamentally wrong here, though. BYREF_ALL only
> initializes variables that have their address taken, which does not
> explain why the size of the stack frame should increase (since in
> order to have an address in the first place, the variable must already
> have a stack slot assigned)
>
> So I suspect that BYREF_ALL is defeating some optimizations where.
> e.g., the call involving the address of the variable is optimized
> away, but the the initialization remains, thus forcing the variable to
> be allocated in the stack frame even though the initializer is the
> only thing that references it.

One pattern I have seen here is temporary variables from macros or
inline functions whose lifetime now extends over the entire function
rather than just the basic block in which they are defined, see e.g.
lpfc_debug_dump_qe() being inlined multiple times into
lpfc_debug_dump_all_queues(). Each instance of the local
"char line_buf[LPFC_LBUF_SZ];" seems to add on to the previous
one now, where the behavior without the structleak plugin is that
they don't.

      Arnd

Re: [PATCH] structleak: disable BYREF_ALL in combination with KASAN_STACK

[Ard Biesheuvel]

On Fri, 21 Jun 2019 at 15:44, Arnd Bergmann <arnd@arndb.de> wrote:
>
> On Fri, Jun 21, 2019 at 3:32 PM Ard Biesheuvel
> <ard.biesheuvel@linaro.org> wrote:
> > On Fri, 21 Jun 2019 at 11:44, Arnd Bergmann <arnd@arndb.de> wrote:
> > > On Thu, Jun 20, 2019 at 7:36 PM Kees Cook <keescook@chromium.org> wrote:
> > > > On Tue, Jun 18, 2019 at 11:47:13AM +0200, Arnd Bergmann wrote:
> > > > > The combination of KASAN_STACK and GCC_PLUGIN_STRUCTLEAK_BYREF_ALL
> > > > > leads to much larger kernel stack usage, as seen from the warnings
> > > > > about functions that now exceed the 2048 byte limit:
> > > >
> > > > Is the preference that this go into v5.2 (there's not much time left),
> > > > or should this be v5.3? (You didn't mark it as Cc: stable?)
> > >
> > > Having it in 5.2 would be great. I had not done much build testing in the last
> > > months, so I didn't actually realize that your patch was merged a while ago
> > > rather than only in linux-next.
> > >
> > > BTW, I have now run into a small number of files that are still affected
> > > by a stack overflow warning from STRUCTLEAK_BYREF_ALL. I'm trying
> > > to come up with patches for those as well, we can probably do it in a way
> > > that also improves the affected drivers. I'll put you on Cc when I
> > > find another one.
> > >
> >
> > There is something fundamentally wrong here, though. BYREF_ALL only
> > initializes variables that have their address taken, which does not
> > explain why the size of the stack frame should increase (since in
> > order to have an address in the first place, the variable must already
> > have a stack slot assigned)
> >
> > So I suspect that BYREF_ALL is defeating some optimizations where.
> > e.g., the call involving the address of the variable is optimized
> > away, but the the initialization remains, thus forcing the variable to
> > be allocated in the stack frame even though the initializer is the
> > only thing that references it.
>
> One pattern I have seen here is temporary variables from macros or
> inline functions whose lifetime now extends over the entire function
> rather than just the basic block in which they are defined, see e.g.
> lpfc_debug_dump_qe() being inlined multiple times into
> lpfc_debug_dump_all_queues(). Each instance of the local
> "char line_buf[LPFC_LBUF_SZ];" seems to add on to the previous
> one now, where the behavior without the structleak plugin is that
> they don't.
>

Right, that seems to be due to the fact that this code

/* split the first bb where we can put the forced initializers */
gcc_assert(single_succ_p(ENTRY_BLOCK_PTR_FOR_FN(cfun)));
bb = single_succ(ENTRY_BLOCK_PTR_FOR_FN(cfun));
if (!single_pred_p(bb)) {
    split_edge(single_succ_edge(ENTRY_BLOCK_PTR_FOR_FN(cfun)));
    gcc_assert(single_succ_p(ENTRY_BLOCK_PTR_FOR_FN(cfun)));
}

puts all the initializers at the beginning of the function rather than
inside the scope of the definition.

Re: [PATCH] structleak: disable BYREF_ALL in combination with KASAN_STACK

[Kees Cook]

On Fri, Jun 21, 2019 at 03:50:02PM +0200, Ard Biesheuvel wrote:
> On Fri, 21 Jun 2019 at 15:44, Arnd Bergmann <arnd@arndb.de> wrote:
> > One pattern I have seen here is temporary variables from macros or
> > inline functions whose lifetime now extends over the entire function
> > rather than just the basic block in which they are defined, see e.g.
> > lpfc_debug_dump_qe() being inlined multiple times into
> > lpfc_debug_dump_all_queues(). Each instance of the local
> > "char line_buf[LPFC_LBUF_SZ];" seems to add on to the previous
> > one now, where the behavior without the structleak plugin is that
> > they don't.

Ewww.

> Right, that seems to be due to the fact that this code
> 
> /* split the first bb where we can put the forced initializers */
> gcc_assert(single_succ_p(ENTRY_BLOCK_PTR_FOR_FN(cfun)));
> bb = single_succ(ENTRY_BLOCK_PTR_FOR_FN(cfun));
> if (!single_pred_p(bb)) {
>     split_edge(single_succ_edge(ENTRY_BLOCK_PTR_FOR_FN(cfun)));
>     gcc_assert(single_succ_p(ENTRY_BLOCK_PTR_FOR_FN(cfun)));
> }
> 
> puts all the initializers at the beginning of the function rather than
> inside the scope of the definition.

Do you see a sane way to improve this? I hadn't noticed that this
actually moved it up to the start of the function. :(

-- 
Kees Cook

Re: [PATCH] structleak: disable BYREF_ALL in combination with KASAN_STACK

[Ard Biesheuvel]

On Sat, 22 Jun 2019 at 22:26, Kees Cook <keescook@chromium.org> wrote:
>
> On Fri, Jun 21, 2019 at 03:50:02PM +0200, Ard Biesheuvel wrote:
> > On Fri, 21 Jun 2019 at 15:44, Arnd Bergmann <arnd@arndb.de> wrote:
> > > One pattern I have seen here is temporary variables from macros or
> > > inline functions whose lifetime now extends over the entire function
> > > rather than just the basic block in which they are defined, see e.g.
> > > lpfc_debug_dump_qe() being inlined multiple times into
> > > lpfc_debug_dump_all_queues(). Each instance of the local
> > > "char line_buf[LPFC_LBUF_SZ];" seems to add on to the previous
> > > one now, where the behavior without the structleak plugin is that
> > > they don't.
>
> Ewww.
>
> > Right, that seems to be due to the fact that this code
> >
> > /* split the first bb where we can put the forced initializers */
> > gcc_assert(single_succ_p(ENTRY_BLOCK_PTR_FOR_FN(cfun)));
> > bb = single_succ(ENTRY_BLOCK_PTR_FOR_FN(cfun));
> > if (!single_pred_p(bb)) {
> >     split_edge(single_succ_edge(ENTRY_BLOCK_PTR_FOR_FN(cfun)));
> >     gcc_assert(single_succ_p(ENTRY_BLOCK_PTR_FOR_FN(cfun)));
> > }
> >
> > puts all the initializers at the beginning of the function rather than
> > inside the scope of the definition.
>
> Do you see a sane way to improve this? I hadn't noticed that this
> actually moved it up to the start of the function. :(
>

Not from the top of my head, and I won't be able to spend any time on
this in the near future, unfortunately.