| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <CAFd5g47-3kM7NscbjXMOMnL3kHt2zURM+7K4ex4T454xNaL2Yw@mail.gmail.com>
Date: Mon, 13 Jan 2020 17:50:25 -0800
From: Brendan Higgins <brendanhiggins@...gle.com>
To: Arnd Bergmann <arnd@...db.de>
Cc: Stephen Boyd <sboyd@...nel.org>, PMWG CI <pmwg-ci@...aro.org>,
Dmitry Torokhov <dmitry.torokhov@...il.com>,
"Rafael J. Wysocki" <rjw@...ysocki.net>,
Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
Private Kernel Alias <private-kwg@...aro.org>,
Kees Cook <keescook@...omium.org>,
Ard Biesheuvel <ardb@...nel.org>
Subject: Re: kunit stack usage, was: pmwg-ci report v5.5-rc4-147-gc62d43442481
On Fri, Jan 10, 2020 at 12:05 AM Arnd Bergmann <arnd@...db.de> wrote:
>
> On Fri, Jan 10, 2020 at 7:27 AM Stephen Boyd <sboyd@...nel.org> wrote:
> > Quoting Arnd Bergmann (2020-01-08 07:13:46)
> > > On Wed, Jan 8, 2020 at 3:41 PM Brendan Higgins <brendanhiggins@...gle.com> wrote:
> > > > On Tue, Jan 7, 2020 at 4:37 AM Arnd Bergmann <arnd@...db.de> wrote:
> > > > test function, which allocates the object, and then calls the unit
> > > > test function with a reference to the object allocation; then we could
> > > > just reuse that allocation and we can avoid making a bunch of
> > > > piecemeal heap allocations.
> > > >
> > > > What do people think? Any other ideas?
> >
> > How about forcing inlining of kunit_do_assertion()? That may allow the
> > compiler to remove all the assertion structs and inline the arguments
> > from the struct to whatever functions the assertion functions call? It
> > may bloat the text size.
>
> I haven't tried it, but I'm fairly sure that would not reliably fix it. The
> problem is that the local 'struct kunit_assert' structure escapes to
> an extern function call it is passed to by reference. If we inline
> kunit_do_assertion(), nothing really changes in that regard as
> the compiler still has to construct and initialize that structure on
> the stack.
>
> However, the reverse would be possible. Turning
> KUNIT_BASE_BINARY_ASSERTION() into an extern
> function that takes all the arguments without passing a
> structure would solve it. I've prototyped this by changing
> KUNIT_BINARY_EQ_ASSERTION() and
> KUNIT_BINARY_NE_ASSERTION() like
>
> @@ -651,13 +649,19 @@ do {
> \
> fmt, \
> ##__VA_ARGS__)
>
> -#define KUNIT_BINARY_NE_ASSERTION(test, assert_type, left, right) \
> +#define __KUNIT_BINARY_NE_ASSERTION(test, assert_type, left, right) \
> KUNIT_BINARY_NE_MSG_ASSERTION(test, \
> assert_type, \
> left, \
> right, \
> NULL)
>
> +static __maybe_unused noinline void KUNIT_BINARY_NE_ASSERTION(struct
> kunit *test, int assert_type,
> + long long left, long long right)
> +{
> + __KUNIT_BINARY_NE_ASSERTION(test, assert_type, left, right);
> +}
> +
> #define KUNIT_BINARY_PTR_NE_MSG_ASSERTION(test,
> \
> assert_type, \
> left, \
>
>
> A little more work is needed to make the varargs and
> code location passing all work correctly.
>
> > > The idea of annotating it got me thinking about what could be
> > > done to improve the structleak plugin, and that in turn got me on
> > > the right track to a silly but trivial fix for the issue: The only thing
> > > that structleak does here is to initialize the implied padding in
> > > the kunit_binary_assert structure. If there is no padding, it all
> > > works out find and the structures don't get pinned to the stack
> > > because the plugin can simply ignore them.
> > >
> > > I tried out this patch and it works:
> > >
> > > diff --git a/include/kunit/assert.h b/include/kunit/assert.h
> > > index db6a0fca09b4..5b09439fa8ae 100644
> > > --- a/include/kunit/assert.h
> > > +++ b/include/kunit/assert.h
> > > @@ -200,8 +200,9 @@ struct kunit_binary_assert {
> > > struct kunit_assert assert;
> > > const char *operation;
> > > const char *left_text;
> > > - long long left_value;
> > > const char *right_text;
> > > + long __pad;
> > > + long long left_value;
> > > long long right_value;
> > > };
> > >
> > > There may also be a problem in 'struct kunit_assert' depending on the
> > > architecture, if there are any on which the 'enum kunit_assert_type'
> > > type is 64 bit wide (which I think is allowed in C, but may not happen
> > > on any toolchain that builds kernels).
> > >
> >
> > What does the padding do? This is all magical!
>
> It turned out to not work after all, The change above fixed some of the
> cases I saw, but not others.
>
> I'm still struggling to fully understand why the structleak gcc plugin
> sometimes forces the structures on the stack and sometimes doesn't.
> The idea for the patch above was to avoid implicit padding by making
> the padding explicit. What happens with the implicit padding is that
> the expanded macro containing code like
>
> struct { const char *left_text; long long left_value; } assert =
> { .left_text = # _left, .left_value = _left };
> func(&assert);
>
> produces a partially initialized object on a 32-bit architecture, with the
> padding between left_text and left_value being old stack data. The
> structleak plugin forces this to be initialized to zero, which in turn
> forces the structure to be allocated on the stack during the execution
> of the function, not just within the surrounding basic block (this
> is a known deficiency in structleak).
>
> The theory so far made sense to me, except that as I said above the
> padding alone did not fix the problem. :(
The padding idea makes sense to me; however, it isn't going to address
the problem with using too much stack space, right? I think the
union/single copy idea would address that, no? (Not that I am excited
by the prospect of making these macros any more magical than they
already are.)
Powered by blists - more mailing lists