[PATCH] task_struct: Only use anon struct under randstruct plugin

[PATCH] task_struct: Only use anon struct under randstruct plugin

[Kees Cook]

The original intent for always adding the anonymous struct in task_struct
was to make sure we had compiler coverage. However, this caused
pathological padding of 40 bytes at the start of task_struct. Instead,
move the anonymous struct to being only used when struct layout
randomization is enabled.

Reported-by: Peter Zijlstra <peterz@infradead.org>
Fixes: 29e48ce87f1e ("task_struct: Allow randomized")
Cc: stable@vger.kernel.org
Signed-off-by: Kees Cook <keescook@chromium.org>
---
 include/linux/compiler-clang.h |  3 ---
 include/linux/compiler-gcc.h   | 12 +++---------
 2 files changed, 3 insertions(+), 12 deletions(-)

diff --git a/include/linux/compiler-clang.h b/include/linux/compiler-clang.h
index d3f264a5b04d..ceb96ecab96e 100644
--- a/include/linux/compiler-clang.h
+++ b/include/linux/compiler-clang.h
@@ -17,9 +17,6 @@
  */
 #define __UNIQUE_ID(prefix) __PASTE(__PASTE(__UNIQUE_ID_, prefix), __COUNTER__)
 
-#define randomized_struct_fields_start	struct {
-#define randomized_struct_fields_end	};
-
 /* all clang versions usable with the kernel support KASAN ABI version 5 */
 #define KASAN_ABI_VERSION 5
 
diff --git a/include/linux/compiler-gcc.h b/include/linux/compiler-gcc.h
index e2c7f4369eff..b4bf73f5e38f 100644
--- a/include/linux/compiler-gcc.h
+++ b/include/linux/compiler-gcc.h
@@ -242,6 +242,9 @@
 #if defined(RANDSTRUCT_PLUGIN) && !defined(__CHECKER__)
 #define __randomize_layout __attribute__((randomize_layout))
 #define __no_randomize_layout __attribute__((no_randomize_layout))
+/* This anon struct can add padding, so only enable it under randstruct. */
+#define randomized_struct_fields_start	struct {
+#define randomized_struct_fields_end	} __randomize_layout;
 #endif
 
 #endif /* GCC_VERSION >= 40500 */
@@ -256,15 +259,6 @@
  */
 #define __visible	__attribute__((externally_visible))
 
-/*
- * RANDSTRUCT_PLUGIN wants to use an anonymous struct, but it is only
- * possible since GCC 4.6. To provide as much build testing coverage
- * as possible, this is used for all GCC 4.6+ builds, and not just on
- * RANDSTRUCT_PLUGIN builds.
- */
-#define randomized_struct_fields_start	struct {
-#define randomized_struct_fields_end	} __randomize_layout;
-
 #endif /* GCC_VERSION >= 40600 */
 
 
-- 
2.7.4


-- 
Kees Cook
Pixel Security

Re: [PATCH] task_struct: Only use anon struct under randstruct plugin

[Andrew Morton]

On Tue, 27 Mar 2018 14:36:09 -0700 Kees Cook <keescook@chromium.org> wrote:

> The original intent for always adding the anonymous struct in task_struct
> was to make sure we had compiler coverage. However, this caused
> pathological padding of 40 bytes at the start of task_struct.

Why?  What caused this padding?  It happens in all configs?

> Instead,
> move the anonymous struct to being only used when struct layout
> randomization is enabled.

So the mysterious 40 byte bloat is still present in this case?

> Reported-by: Peter Zijlstra <peterz@infradead.org>
> Fixes: 29e48ce87f1e ("task_struct: Allow randomized")
> Cc: stable@vger.kernel.org

Why cc:stable?

Re: [PATCH] task_struct: Only use anon struct under randstruct plugin

[Linus Torvalds]

On Tue, Mar 27, 2018 at 1:03 PM, Andrew Morton
<akpm@linux-foundation.org> wrote:
>
> Why?  What caused this padding?  It happens in all configs?

I assume what happens is that the anonymous struct ends up containing
fields that are cacheline-aligned, and then the whole anonymous struct
is cacheline-aligned.

Which is all kinds of stupid, since the anonymous struct itself does
not exist outside of the outer struct. So it would be entirely
sufficient to just make the outer struct cacheline aligned (like it
used to be), but not align the inner anonymous one - just the fields
in it.

But there may be "reasons" why the inner anonymous  one needs to be
aligned. Maybe it's some standards requirement, or maybe it's just an
internal gcc implementation detail.

Regardless, it's a bit sad. It also means that when randomization is
on, that unnecessary padding will be there.

I wonder if there is some acceptable trick to avoid it. Maybe the
anonymous struct can be marked as not needing alignment, even if the
fields inside of it would still need to be aligned wrt the outer
struct.

>> Instead,
>> move the anonymous struct to being only used when struct layout
>> randomization is enabled.
>
> So the mysterious 40 byte bloat is still present in this case?

Almost certainly. And the struct randomization will possibly add much
*more* padding elsewhere, since at least some of  our structures are
laid out to try to avoid padding, and then the randomization might be
breaking that.

               Linus

Re: [PATCH] task_struct: Only use anon struct under randstruct plugin

[Peter Zijlstra]

On Tue, Mar 27, 2018 at 02:22:31PM -1000, Linus Torvalds wrote:
> On Tue, Mar 27, 2018 at 1:03 PM, Andrew Morton
> <akpm@linux-foundation.org> wrote:
> >
> > Why?  What caused this padding?  It happens in all configs?
> 
> I assume what happens is that the anonymous struct ends up containing
> fields that are cacheline-aligned, and then the whole anonymous struct
> is cacheline-aligned.

Yes, structures inherit the alignment requirements of their constituent
members.

> Which is all kinds of stupid, since the anonymous struct itself does
> not exist outside of the outer struct. So it would be entirely
> sufficient to just make the outer struct cacheline aligned (like it
> used to be), but not align the inner anonymous one - just the fields
> in it.
> 
> But there may be "reasons" why the inner anonymous  one needs to be
> aligned. Maybe it's some standards requirement, or maybe it's just an
> internal gcc implementation detail.

Last time I read the standard there wasn't a distinction between
anonymous and regular structures for this. So in that regards a strict
reading of the standard would mandate this behaviour.

> Regardless, it's a bit sad. It also means that when randomization is
> on, that unnecessary padding will be there.

The other complaint is that the anonymous structure makes the pahole
output (which is what I showed) unnecessarily ugly.

Re: [PATCH] task_struct: Only use anon struct under randstruct plugin

[Kees Cook]

On Tue, Mar 27, 2018 at 4:03 PM, Andrew Morton
<akpm@linux-foundation.org> wrote:
> On Tue, 27 Mar 2018 14:36:09 -0700 Kees Cook <keescook@chromium.org> wrote:
>> Reported-by: Peter Zijlstra <peterz@infradead.org>
>> Fixes: 29e48ce87f1e ("task_struct: Allow randomized")
>> Cc: stable@vger.kernel.org
>
> Why cc:stable?

Since the padding existed in all configs, it's kind of an ugly wart
and should likely be fixed up for 4.14 and 4.15 -stable.

> So the mysterious 40 byte bloat is still present in this case?

Given how insane[1] task_struct can end up under randstruct, these 40
bytes aren't too bad. I've added fixing this to the randstruct to-do
list, but I don't view it as high priority.

-Kees
[1] https://git.kernel.org/linus/ffa47aa678cfaa9b88e8a26cfb115b4768325121

-- 
Kees Cook
Pixel Security

Re: [PATCH] task_struct: Only use anon struct under randstruct plugin

[Andrew Morton]

On Tue, 27 Mar 2018 17:30:47 -0700 Kees Cook <keescook@chromium.org> wrote:

> On Tue, Mar 27, 2018 at 4:03 PM, Andrew Morton
> <akpm@linux-foundation.org> wrote:
> > On Tue, 27 Mar 2018 14:36:09 -0700 Kees Cook <keescook@chromium.org> wrote:
> >> Reported-by: Peter Zijlstra <peterz@infradead.org>
> >> Fixes: 29e48ce87f1e ("task_struct: Allow randomized")
> >> Cc: stable@vger.kernel.org
> >
> > Why cc:stable?
> 
> Since the padding existed in all configs, it's kind of an ugly wart
> and should likely be fixed up for 4.14 and 4.15 -stable.

That didn't tell us much :(
Documentation/process/stable-kernel-rules.rst doesn't mention "ugly
wart".

I think what you're hearing here is that this patch needs a better
changelog, please.  Not an uncommon failing, sigh.  A better
explanation of the origins of this padding and a better explanation of
the reasons for backporting the fix.