Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1651608 > unrolled thread

[PATCH v2 00/20] Introduce struct layout randomization plugin

Back to article view | Back to linux.kernel

Contents

  [PATCH v2 00/20] Introduce struct layout randomization plugin Kees Cook <keescook@chromium.org> - 2017-05-27 03:20 +0200
    [PATCH v2 06/20] randstruct: Whitelist UNIXCB cast Kees Cook <keescook@chromium.org> - 2017-05-27 03:20 +0200
      Re: [PATCH v2 06/20] randstruct: Whitelist UNIXCB cast Kees Cook <keescook@chromium.org> - 2017-05-27 03:50 +0200
      Re: [PATCH v2 06/20] randstruct: Whitelist UNIXCB cast Christoph Hellwig <hch@infradead.org> - 2017-05-28 10:00 +0200
    [PATCH v2 13/20] drm/amdgpu: Use designated initializers Kees Cook <keescook@chromium.org> - 2017-05-27 03:20 +0200
    [PATCH v2 05/20] randstruct: Whitelist struct security_hook_heads cast Kees Cook <keescook@chromium.org> - 2017-05-27 03:20 +0200
      Re: [PATCH v2 05/20] randstruct: Whitelist struct  security_hook_heads cast Christoph Hellwig <hch@infradead.org> - 2017-05-27 10:50 +0200
        Re: [PATCH v2 05/20] randstruct: Whitelist struct security_hook_heads cast Kees Cook <keescook@chromium.org> - 2017-05-27 22:20 +0200
          Re: [PATCH v2 05/20] randstruct: Whitelist struct security_hook_heads cast Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> - 2017-05-28 00:10 +0200
            Re: [kernel-hardening] Re: [PATCH v2 05/20] randstruct: Whitelist  struct security_hook_heads cast Kees Cook <keescook@chromium.org> - 2017-05-28 02:50 +0200
          Re: [PATCH v2 05/20] randstruct: Whitelist struct security_hook_heads  cast James Morris <jmorris@namei.org> - 2017-05-30 12:40 +0200
    [PATCH v2 02/20] gcc-plugins: Detail c-common.h location for GCC 4.6 Kees Cook <keescook@chromium.org> - 2017-05-27 03:20 +0200
    [PATCH v2 11/20] randstruct: Disable randomization of ACPICA structs Kees Cook <keescook@chromium.org> - 2017-05-27 03:20 +0200
      Re: [PATCH v2 11/20] randstruct: Disable randomization of ACPICA  structs Christoph Hellwig <hch@infradead.org> - 2017-05-27 10:50 +0200
        Re: [PATCH v2 11/20] randstruct: Disable randomization of ACPICA structs Kees Cook <keescook@chromium.org> - 2017-05-27 22:10 +0200
          Re: [PATCH v2 11/20] randstruct: Disable randomization of ACPICA  structs Christoph Hellwig <hch@infradead.org> - 2017-05-28 07:00 +0200
    Re: [PATCH v2 19/20] [RFC] task_struct: Allow randomized layout Kees Cook <keescook@chromium.org> - 2017-05-27 03:30 +0200
    Re: [PATCH v2 19/20] [RFC] task_struct: Allow randomized layout Linus Torvalds <torvalds@linux-foundation.org> - 2017-05-27 03:30 +0200
    [PATCH v2 15/20] mtk-vcodec: Use designated initializers Kees Cook <keescook@chromium.org> - 2017-05-27 03:50 +0200
    [PATCH v2 14/20] drm/amd/powerplay: Use designated initializers Kees Cook <keescook@chromium.org> - 2017-05-27 03:50 +0200
      Re: [PATCH v2 14/20] drm/amd/powerplay: Use designated initializers Christoph Hellwig <hch@infradead.org> - 2017-05-27 10:50 +0200
        Re: [PATCH v2 14/20] drm/amd/powerplay: Use designated initializers Kees Cook <keescook@chromium.org> - 2017-05-27 22:20 +0200
    [PATCH v2 20/20] ACPICA: Use designated initializers Kees Cook <keescook@chromium.org> - 2017-05-27 03:50 +0200
      Re: [PATCH v2 20/20] ACPICA: Use designated initializers Christoph Hellwig <hch@infradead.org> - 2017-05-28 09:50 +0200
    [PATCH v2 18/20] randstruct: Enable function pointer struct detection Kees Cook <keescook@chromium.org> - 2017-05-27 03:50 +0200
    [PATCH v2 16/20] ntfs: Use ERR_CAST() to avoid cross-structure cast Kees Cook <keescook@chromium.org> - 2017-05-27 03:50 +0200
    [PATCH v2 17/20] ocfs2: Use ERR_CAST() to avoid cross-structure cast Kees Cook <keescook@chromium.org> - 2017-05-27 03:50 +0200
    [PATCH v2 19/20] [RFC] task_struct: Allow randomized layout Kees Cook <keescook@chromium.org> - 2017-05-27 03:50 +0200
    [PATCH v2 10/20] randstruct: opt-out externally exposed function pointer structs Kees Cook <keescook@chromium.org> - 2017-05-27 04:00 +0200
    [PATCH v2 12/20] sgi-xp: Use designated initializers Kees Cook <keescook@chromium.org> - 2017-05-27 04:00 +0200
      Re: [PATCH v2 12/20] sgi-xp: Use designated initializers Christoph Hellwig <hch@infradead.org> - 2017-05-27 10:50 +0200
    [PATCH v2 01/20] NFS: Avoid cross-structure casting Kees Cook <keescook@chromium.org> - 2017-05-27 04:00 +0200
      Re: [PATCH v2 01/20] NFS: Avoid cross-structure casting Christoph Hellwig <hch@infradead.org> - 2017-05-28 10:00 +0200
        Re: [PATCH v2 01/20] NFS: Avoid cross-structure casting Kees Cook <keescook@chromium.org> - 2017-05-28 19:00 +0200
    [PATCH v2 03/20] compiler: Add __designated_init annotation Kees Cook <keescook@chromium.org> - 2017-05-27 04:00 +0200
    [PATCH v2 08/20] randstruct: Whitelist NIU struct page overloading Kees Cook <keescook@chromium.org> - 2017-05-27 04:00 +0200
      Re: [PATCH v2 08/20] randstruct: Whitelist NIU struct page  overloading Christoph Hellwig <hch@infradead.org> - 2017-05-28 10:20 +0200
        Re: [PATCH v2 08/20] randstruct: Whitelist NIU struct page overloading Kees Cook <keescook@chromium.org> - 2017-05-28 19:40 +0200
        Re: [PATCH v2 08/20] randstruct: Whitelist NIU struct page overloading Kees Cook <keescook@chromium.org> - 2017-05-28 19:40 +0200

Page 1 of 2  [1] 2  Next page →

#1651608 — [PATCH v2 00/20] Introduce struct layout randomization plugin

This series brings grsecurity's structure layout randomization plugin
to upstream. The plugin randomizes the layout of selected structures at
compile time, as a probabilistic defense against attacks that need to
know the layout of structures within the kernel. This is most useful for
"in-house" kernel builds where the neither the randomization seed nor
other build artifacts are made available to an attacker. While less useful
for distribution kernels (where the randomization seed must be exposed for
third party kernel module builds), it still has some value there since now
all kernel builds would need to be tracked by an attacker.

One requirement of the plugin is that randomized structures must use
designated initializers. Many of these have been landing already as
I've been sending them over the past couple months, but there are
still some stragglers, which are included here.

Another area to address are places where randomized structures are cast
to other structures, since there may be implicit positional details
that need to be addressed. Luckily, there are only a few of these false
positives, and they have been worked around either by adjusting the source
(e.g. correctly using ERR_CAST() or container_of()) or whitelisting them
in the plugin. Some of these fixes have landed already, again with
stragglers included here.

The plugin selects structures in two ways: manually marked with the
new __randomize_layout annotation, or automatically when a structure
is found to consist entirely of function pointers (which can be opted
out of with the new __no_randomize_layout annotation).

A structure that is especially sensitive and regularly abused in
exploits is task_struct, but randomizing it requires some special
handling due to some fields needing to be at the start and end. To
deal with this, an internal anonymous struct is used to mark the
portion that will be randomized. I'd love feedback on whether I
should bite the bullet and perform indenting or violate indenting
rules to avoid a massive white-space change. Also is the problem
that older GCC seems to balk at its use, which I have not figured
out yet.

As already mentioned, the bulk of this feature and annotations are ported
over from grsecurity. The implementation is almost entirely identical
to the original code written by the PaX Team and Brad Spengler. The
changes are an addition of improved designated initializer markings,
a whitelisting mechanism, many false positive fixes, and a different
approach to handling the task_struct randomization.

I've been doing boot tests with instrumentation showing successfully
changing offsets within the task_struct, which ran overnight without
problems. So far, the 0day builder hasn't alerted on anything either.

This series is based on next-20170525.

I intend to push patches 1 through 18 into linux-next if there are no
objections. The task_struct change needs some more attention, and I
continue to wait on ACPICA to take the changes in the final patch.

Patches are:

[PATCH 01/20] NFS: Avoid cross-structure casting
	Fix to use ERR_CAST()

[PATCH 02/20] gcc-plugins: Detail c-common.h location for GCC 4.6
	Update documentation about GCC 4.6 version file locations.

[PATCH 03/20] compiler: Add __designated_init annotation
	Introduce annotation for designated initializers.

[PATCH 04/20] gcc-plugins: Add the randstruct plugin
	The plugin itself, with struct auto-detection disabled.

[PATCH 05/20] randstruct: Whitelist struct security_hook_heads cast
[PATCH 06/20] randstruct: Whitelist UNIXCB cast
[PATCH 07/20] randstruct: Whitelist big_key path struct overloading
[PATCH 08/20] randstruct: Whitelist NIU struct page overloading
	Whitelist a number of false positives that do not have
	trivial source corrections to be made.

[PATCH 09/20] randstruct: Mark various structs for randomization
	Adds the manual annotation for structures to randomize.

[PATCH 10/20] randstruct: opt-out externally exposed function pointer
	Opt out of some externally-exposed structs that would be
	otherwise automatically randomized.

[PATCH 11/20] randstruct: Disable randomization of ACPICA structs
	Opt out of ACPICA randomization.

[PATCH 12/20] sgi-xp: Use designated initializers
[PATCH 13/20] drm/amdgpu: Use designated initializers
[PATCH 14/20] drm/amd/powerplay: Use designated initializers
[PATCH 15/20] mtk-vcodec: Use designated initializers
	The remaining designated initializer fixes for automatic
	struct randomization.

[PATCH 16/20] ntfs: Use ERR_CAST() to avoid cross-structure cast
[PATCH 17/20] ocfs2: Use ERR_CAST() to avoid cross-structure cast
	The remaining cast fixes for automatic struct randomization.

[PATCH 18/20] randstruct: Enable function pointer struct detection
	Enables automatic struct randomization.

[PATCH 19/20] [RFC] task_struct: Allow randomized layout
	Tricky anonymous struct within task_struct...

[PATCH 20/20] ACPICA: Use designated initializers
	Proposed upstream ACPICA solution for designated initializers...

Testing/feedback appreciated!

-Kees

[toc] | [next] | [standalone]

#1651610 — [PATCH v2 06/20] randstruct: Whitelist UNIXCB cast

This is another false positive in bad cast detection:

net/unix/af_unix.c: In function ‘unix_skb_scm_eq’:
net/unix/af_unix.c:1621:31: note: found mismatched rhs struct pointer types: ‘struct unix_skb_parms’ and ‘char’

  const struct unix_skb_parms *u = &UNIXCB(skb);
                               ^

UNIXCB is:

	#define UNIXCB(skb)     (*(struct unix_skb_parms *)&((skb)->cb))

And ->cb is:

	char                    cb[48] __aligned(8);

This is a rather crazy cast, but appears to be safe in the face of
randomization, so whitelist it in the plugin.

Cc: Hannes Frederic Sowa <hannes@stressinduktion.org>
Cc: Signed-off-by: David S. Miller <davem@davemloft.net>
Signed-off-by: Kees Cook <keescook@chromium.org>
---
 scripts/gcc-plugins/randomize_layout_plugin.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/scripts/gcc-plugins/randomize_layout_plugin.c b/scripts/gcc-plugins/randomize_layout_plugin.c
index e126ac7874af..bf110915a5aa 100644
--- a/scripts/gcc-plugins/randomize_layout_plugin.c
+++ b/scripts/gcc-plugins/randomize_layout_plugin.c
@@ -43,6 +43,8 @@ struct whitelist_entry {
 };
 
 static const struct whitelist_entry whitelist[] = {
+	/* unix_skb_parms via UNIXCB() buffer */
+	{ "net/unix/af_unix.c", "unix_skb_parms", "char" },
 	/* walk struct security_hook_heads as an array of struct list_head */
 	{ "security/security.c", "list_head", "security_hook_heads" },
 	{ }
-- 
2.7.4

[toc] | [prev] | [next] | [standalone]

#1651713 — Re: [PATCH v2 06/20] randstruct: Whitelist UNIXCB cast

On Fri, May 26, 2017 at 1:17 PM, Kees Cook <keescook@chromium.org> wrote:
> This is another false positive in bad cast detection:
>
> net/unix/af_unix.c: In function ‘unix_skb_scm_eq’:
> net/unix/af_unix.c:1621:31: note: found mismatched rhs struct pointer types: ‘struct unix_skb_parms’ and ‘char’
>
>   const struct unix_skb_parms *u = &UNIXCB(skb);
>                                ^
>
> UNIXCB is:
>
>         #define UNIXCB(skb)     (*(struct unix_skb_parms *)&((skb)->cb))
>
> And ->cb is:
>
>         char                    cb[48] __aligned(8);
>
> This is a rather crazy cast, but appears to be safe in the face of
> randomization, so whitelist it in the plugin.
>
> Cc: Hannes Frederic Sowa <hannes@stressinduktion.org>
> Cc: Signed-off-by: David S. Miller <davem@davemloft.net>

Argh, paste-o. I was adding git history folks here to Cc, pardon the
glitch. This should be Cc: and absolutely not a S-o-B from Dave.

-Kees

> Signed-off-by: Kees Cook <keescook@chromium.org>
> ---
>  scripts/gcc-plugins/randomize_layout_plugin.c | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/scripts/gcc-plugins/randomize_layout_plugin.c b/scripts/gcc-plugins/randomize_layout_plugin.c
> index e126ac7874af..bf110915a5aa 100644
> --- a/scripts/gcc-plugins/randomize_layout_plugin.c
> +++ b/scripts/gcc-plugins/randomize_layout_plugin.c
> @@ -43,6 +43,8 @@ struct whitelist_entry {
>  };
>
>  static const struct whitelist_entry whitelist[] = {
> +       /* unix_skb_parms via UNIXCB() buffer */
> +       { "net/unix/af_unix.c", "unix_skb_parms", "char" },
>         /* walk struct security_hook_heads as an array of struct list_head */
>         { "security/security.c", "list_head", "security_hook_heads" },
>         { }
> --
> 2.7.4
>



-- 
Kees Cook
Pixel Security

[toc] | [prev] | [next] | [standalone]

#1652054 — Re: [PATCH v2 06/20] randstruct: Whitelist UNIXCB cast

On Fri, May 26, 2017 at 01:17:10PM -0700, Kees Cook wrote:
> This is another false positive in bad cast detection:
> 
> net/unix/af_unix.c: In function ‘unix_skb_scm_eq’:
> net/unix/af_unix.c:1621:31: note: found mismatched rhs struct pointer types: ‘struct unix_skb_parms’ and ‘char’
> 
>   const struct unix_skb_parms *u = &UNIXCB(skb);
>                                ^
> 
> UNIXCB is:
> 
> 	#define UNIXCB(skb)     (*(struct unix_skb_parms *)&((skb)->cb))
> 
> And ->cb is:
> 
> 	char                    cb[48] __aligned(8);
> 
> This is a rather crazy cast, but appears to be safe in the face of
> randomization, so whitelist it in the plugin.

We have a lot of network protocol that use the ->cb area, which makes me
wonder why this one would be so special.

It seems like everyone is just using a plain cast to a pointer without
doing the address taking trick that doesn't make sense for arrays
anyway.

Maybe we just need to fix up the af_unix code to work the same as all
other protocols?

[toc] | [prev] | [next] | [standalone]

#1651611 — [PATCH v2 13/20] drm/amdgpu: Use designated initializers

The randstruct plugin requires structures that are entirely function
pointers be initialized using designated initializers.

Cc: Alex Deucher <alexander.deucher@amd.com>
Cc: Christian König <christian.koenig@amd.com>
Signed-off-by: Kees Cook <keescook@chromium.org>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_vram_mgr.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vram_mgr.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vram_mgr.c
index a4831fe0223b..a2c59a08b2bd 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vram_mgr.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vram_mgr.c
@@ -220,9 +220,9 @@ static void amdgpu_vram_mgr_debug(struct ttm_mem_type_manager *man,
 }
 
 const struct ttm_mem_type_manager_func amdgpu_vram_mgr_func = {
-	amdgpu_vram_mgr_init,
-	amdgpu_vram_mgr_fini,
-	amdgpu_vram_mgr_new,
-	amdgpu_vram_mgr_del,
-	amdgpu_vram_mgr_debug
+	.init		= amdgpu_vram_mgr_init,
+	.takedown	= amdgpu_vram_mgr_fini,
+	.get_node	= amdgpu_vram_mgr_new,
+	.put_node	= amdgpu_vram_mgr_del,
+	.debug		= amdgpu_vram_mgr_debug
 };
-- 
2.7.4

[toc] | [prev] | [next] | [standalone]

#1651612 — [PATCH v2 05/20] randstruct: Whitelist struct security_hook_heads cast

The LSM initialization routines walk security_hook_heads as an array
of struct list_head instead of via names to avoid a ton of needless
source. Whitelist this to avoid the false positive warning from the
plugin:

security/security.c: In function ‘security_init’:
security/security.c:59:20: note: found mismatched op0 struct pointer types: ‘struct list_head’ and ‘struct security_hook_heads’

  struct list_head *list = (struct list_head *) &security_hook_heads;
                    ^

Cc: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
Cc: James Morris <james.l.morris@oracle.com>
Signed-off-by: Kees Cook <keescook@chromium.org>
---
 scripts/gcc-plugins/randomize_layout_plugin.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/scripts/gcc-plugins/randomize_layout_plugin.c b/scripts/gcc-plugins/randomize_layout_plugin.c
index bccbec2af0e4..e126ac7874af 100644
--- a/scripts/gcc-plugins/randomize_layout_plugin.c
+++ b/scripts/gcc-plugins/randomize_layout_plugin.c
@@ -43,6 +43,8 @@ struct whitelist_entry {
 };
 
 static const struct whitelist_entry whitelist[] = {
+	/* walk struct security_hook_heads as an array of struct list_head */
+	{ "security/security.c", "list_head", "security_hook_heads" },
 	{ }
 };
 
-- 
2.7.4

[toc] | [prev] | [next] | [standalone]

#1651820 — Re: [PATCH v2 05/20] randstruct: Whitelist struct security_hook_heads cast

On Fri, May 26, 2017 at 01:17:09PM -0700, Kees Cook wrote:
> The LSM initialization routines walk security_hook_heads as an array
> of struct list_head instead of via names to avoid a ton of needless
> source. Whitelist this to avoid the false positive warning from the
> plugin:

I think this crap just needs to be fixed properly.  If not it almost
defeats the protections as the "security" ops are just about everywhere.

[toc] | [prev] | [next] | [standalone]

#1651977 — Re: [PATCH v2 05/20] randstruct: Whitelist struct security_hook_heads cast

On Sat, May 27, 2017 at 1:41 AM, Christoph Hellwig <hch@infradead.org> wrote:
> On Fri, May 26, 2017 at 01:17:09PM -0700, Kees Cook wrote:
>> The LSM initialization routines walk security_hook_heads as an array
>> of struct list_head instead of via names to avoid a ton of needless
>> source. Whitelist this to avoid the false positive warning from the
>> plugin:
>
> I think this crap just needs to be fixed properly.  If not it almost
> defeats the protections as the "security" ops are just about everywhere.

There's nothing unsafe about 3dfc9b02864b19f4dab376f14479ee4ad1de6c9e,
it just avoids tons of needless code. Tetsuo has some other ideas for
cleaning it up further, but I don't like it because it removes
compile-time verification of function types. There have been a lot of
trade-offs in getting this working correctly, so I don't have any
problem with how it looks currently. It's just a collision of
assumptions between randstruct (omg, you're accessing a randomized
struct with a different struct!) and the security head list (all
entries are lists, and we're just initializing them).

-Kees

-- 
Kees Cook
Pixel Security

[toc] | [prev] | [next] | [standalone]

#1651995 — Re: [PATCH v2 05/20] randstruct: Whitelist struct security_hook_heads cast

Kees Cook wrote:
> On Sat, May 27, 2017 at 1:41 AM, Christoph Hellwig <hch@infradead.org> wrote:
> > On Fri, May 26, 2017 at 01:17:09PM -0700, Kees Cook wrote:
> >> The LSM initialization routines walk security_hook_heads as an array
> >> of struct list_head instead of via names to avoid a ton of needless
> >> source. Whitelist this to avoid the false positive warning from the
> >> plugin:
> >
> > I think this crap just needs to be fixed properly.  If not it almost
> > defeats the protections as the "security" ops are just about everywhere.
> 
> There's nothing unsafe about 3dfc9b02864b19f4dab376f14479ee4ad1de6c9e,
> it just avoids tons of needless code. Tetsuo has some other ideas for
> cleaning it up further, but I don't like it because it removes
> compile-time verification of function types.

Excuse me, but why you think that compile-time verification of function
types is removed?

-	{ .head = &security_hook_heads.HEAD, .hook = { .HEAD = HOOK } }
+	{ .idx = LSM_##HEAD, .hook = { .HEAD = HOOK } }

This change removes dependency on absolute address of security_hook_heads
being known at compile-time. If function types of .hook.HEAD and HOOK
mismatches, the compiler can still warn it.

[toc] | [prev] | [next] | [standalone]

#1652019 — Re: [kernel-hardening] Re: [PATCH v2 05/20] randstruct: Whitelist struct security_hook_heads cast

On Sat, May 27, 2017 at 3:04 PM, Tetsuo Handa
<penguin-kernel@i-love.sakura.ne.jp> wrote:
> Kees Cook wrote:
>> On Sat, May 27, 2017 at 1:41 AM, Christoph Hellwig <hch@infradead.org> wrote:
>> > On Fri, May 26, 2017 at 01:17:09PM -0700, Kees Cook wrote:
>> >> The LSM initialization routines walk security_hook_heads as an array
>> >> of struct list_head instead of via names to avoid a ton of needless
>> >> source. Whitelist this to avoid the false positive warning from the
>> >> plugin:
>> >
>> > I think this crap just needs to be fixed properly.  If not it almost
>> > defeats the protections as the "security" ops are just about everywhere.
>>
>> There's nothing unsafe about 3dfc9b02864b19f4dab376f14479ee4ad1de6c9e,
>> it just avoids tons of needless code. Tetsuo has some other ideas for
>> cleaning it up further, but I don't like it because it removes
>> compile-time verification of function types.
>
> Excuse me, but why you think that compile-time verification of function
> types is removed?
>
> -       { .head = &security_hook_heads.HEAD, .hook = { .HEAD = HOOK } }
> +       { .idx = LSM_##HEAD, .hook = { .HEAD = HOOK } }
>
> This change removes dependency on absolute address of security_hook_heads
> being known at compile-time. If function types of .hook.HEAD and HOOK
> mismatches, the compiler can still warn it.

Sorry, misremembered, that was the other patch. I'll go review this
current one...

-Kees

-- 
Kees Cook
Pixel Security

[toc] | [prev] | [next] | [standalone]

#1653091 — Re: [PATCH v2 05/20] randstruct: Whitelist struct security_hook_heads cast

On Sat, 27 May 2017, Kees Cook wrote:

> On Sat, May 27, 2017 at 1:41 AM, Christoph Hellwig <hch@infradead.org> wrote:
> > On Fri, May 26, 2017 at 01:17:09PM -0700, Kees Cook wrote:
> >> The LSM initialization routines walk security_hook_heads as an array
> >> of struct list_head instead of via names to avoid a ton of needless
> >> source. Whitelist this to avoid the false positive warning from the
> >> plugin:
> >
> > I think this crap just needs to be fixed properly.  If not it almost
> > defeats the protections as the "security" ops are just about everywhere.
> 
> There's nothing unsafe about 3dfc9b02864b19f4dab376f14479ee4ad1de6c9e,
> it just avoids tons of needless code. 

Removing needless code is a security feature, ideally.

> Tetsuo has some other ideas for
> cleaning it up further, but I don't like it because it removes
> compile-time verification of function types. There have been a lot of
> trade-offs in getting this working correctly, so I don't have any
> problem with how it looks currently. It's just a collision of
> assumptions between randstruct (omg, you're accessing a randomized
> struct with a different struct!) and the security head list (all
> entries are lists, and we're just initializing them).

Fix randstruct perhaps, rather than modifying kernel code to shut it up.

-- 
James Morris
<jmorris@namei.org>

[toc] | [prev] | [next] | [standalone]

#1651613 — [PATCH v2 02/20] gcc-plugins: Detail c-common.h location for GCC 4.6

The c-common.h file moved in stock gcc 4.7, not gcc 4.6. However, most
people building plugins with gcc 4.6 are using the Debian or Ubuntu
version, which includes a patch to move the headers to the 4.7 location.
In case anyone trips over this with a stock gcc 4.6, add a pointer to the
patch used by Debian/Ubuntu.

Signed-off-by: Kees Cook <keescook@chromium.org>
---
 scripts/gcc-plugins/gcc-common.h | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/scripts/gcc-plugins/gcc-common.h b/scripts/gcc-plugins/gcc-common.h
index b232ab15624c..918953ca4527 100644
--- a/scripts/gcc-plugins/gcc-common.h
+++ b/scripts/gcc-plugins/gcc-common.h
@@ -63,6 +63,13 @@
 #endif
 
 #if BUILDING_GCC_VERSION >= 4006
+/*
+ * The c-family headers were moved into a subdirectory in GCC version
+ * 4.7, but most plugin-building users of GCC 4.6 are using the Debian
+ * or Ubuntu package, which has an out-of-tree patch to move this to the
+ * same location as found in 4.7 and later:
+ * https://sources.debian.net/src/gcc-4.6/4.6.3-14/debian/patches/pr45078.diff/
+ */
 #include "c-family/c-common.h"
 #else
 #include "c-common.h"
-- 
2.7.4

[toc] | [prev] | [next] | [standalone]

#1651614 — [PATCH v2 11/20] randstruct: Disable randomization of ACPICA structs

Since the ACPICA source is maintained externally to the kernel, we can
neither switch it to designated initializers nor mark it
__no_randomize_layout. Until ACPICA-upstream changes[1] land to handle the
designated initialization, explicitly skip it in the plugin.

[1] https://github.com/acpica/acpica/pull/248

Signed-off-by: Kees Cook <keescook@chromium.org>
---
 scripts/gcc-plugins/randomize_layout_plugin.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/scripts/gcc-plugins/randomize_layout_plugin.c b/scripts/gcc-plugins/randomize_layout_plugin.c
index f777ead58ba8..e6e02a40d522 100644
--- a/scripts/gcc-plugins/randomize_layout_plugin.c
+++ b/scripts/gcc-plugins/randomize_layout_plugin.c
@@ -346,6 +346,10 @@ static int relayout_struct(tree type)
 	    !strcmp((const char *)ORIG_TYPE_NAME(type), "RAWPCIFACTORY"))
 		return 0;
 
+	/* Skip ACPICA structs until refreshed with designated_init. */
+	if (!strcmp((const char *)ORIG_TYPE_NAME(type), "acpi_sleep_functions"))
+		return 0;
+
 	/* throw out any structs in uapi */
 	xloc = expand_location(DECL_SOURCE_LOCATION(TYPE_FIELDS(type)));
 
-- 
2.7.4

[toc] | [prev] | [next] | [standalone]

#1651819 — Re: [PATCH v2 11/20] randstruct: Disable randomization of ACPICA structs

On Fri, May 26, 2017 at 01:17:15PM -0700, Kees Cook wrote:
> Since the ACPICA source is maintained externally to the kernel, we can
> neither switch it to designated initializers nor mark it
> __no_randomize_layout. Until ACPICA-upstream changes[1] land to handle the
> designated initialization, explicitly skip it in the plugin.
> 
> [1] https://github.com/acpica/acpica/pull/248

I'd just overried the ACPIA bullshit process and just include these
changes, as they are a major improvement independent of any
reandomization.

[toc] | [prev] | [next] | [standalone]

#1651974 — Re: [PATCH v2 11/20] randstruct: Disable randomization of ACPICA structs

On Sat, May 27, 2017 at 1:42 AM, Christoph Hellwig <hch@infradead.org> wrote:
> On Fri, May 26, 2017 at 01:17:15PM -0700, Kees Cook wrote:
>> Since the ACPICA source is maintained externally to the kernel, we can
>> neither switch it to designated initializers nor mark it
>> __no_randomize_layout. Until ACPICA-upstream changes[1] land to handle the
>> designated initialization, explicitly skip it in the plugin.
>>
>> [1] https://github.com/acpica/acpica/pull/248
>
> I'd just overried the ACPIA bullshit process and just include these
> changes, as they are a major improvement independent of any
> reandomization.

Well... I'd rather not. It's been explicitly NAKed by them already,
which is why I sent the upstream solution (though it's being ignored
currently).

I don't want this to block randstruct any longer, so that's why I took
a whitelisting approach here.

-Kees

-- 
Kees Cook
Pixel Security

[toc] | [prev] | [next] | [standalone]

#1652028 — Re: [PATCH v2 11/20] randstruct: Disable randomization of ACPICA structs

On Sat, May 27, 2017 at 01:03:23PM -0700, Kees Cook wrote:
> On Sat, May 27, 2017 at 1:42 AM, Christoph Hellwig <hch@infradead.org> wrote:
> > On Fri, May 26, 2017 at 01:17:15PM -0700, Kees Cook wrote:
> >> Since the ACPICA source is maintained externally to the kernel, we can
> >> neither switch it to designated initializers nor mark it
> >> __no_randomize_layout. Until ACPICA-upstream changes[1] land to handle the
> >> designated initialization, explicitly skip it in the plugin.
> >>
> >> [1] https://github.com/acpica/acpica/pull/248
> >
> > I'd just overried the ACPIA bullshit process and just include these
> > changes, as they are a major improvement independent of any
> > reandomization.
> 
> Well... I'd rather not. It's been explicitly NAKed by them already,
> which is why I sent the upstream solution (though it's being ignored
> currently).

Send it to linux-keernel again and we'll override their bulllshit.
We can make exceptions from our coding style and preference just because
they are idiots.  And the whole ACPI mess needs urgent de-obsfucation
anyway.

[toc] | [prev] | [next] | [standalone]

#1651636 — Re: [PATCH v2 19/20] [RFC] task_struct: Allow randomized layout

On Fri, May 26, 2017 at 1:23 PM, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
> On Fri, May 26, 2017 at 1:17 PM, Kees Cook <keescook@chromium.org> wrote:
>> This marks most of the layout of task_struct as randomizable, but leaves
>> thread_info and scheduler state untouched at the start, and thread_struct
>> untouched at the end.
>
> I think you want to abstract this out somehow, because this is both
> ugly and bad:
>
>> +       /* This begins the randomizable portion of task_struct... */
>> +#if GCC_VERSION >= 40600
>> +       struct {
>> +#endif
>
> when you could instead just introduce something like
>
> #if GCC_VERSION >= 40600
>   #define randomized_struct_fields_start struct {
>   #define randomized_struct_fields_end } __randomize_layout;
> #else
>   #define randomized_struct_fields_start
>   #define randomized_struct_fields_end
> #endif
>
> and then this pattern is
>  (a) more-or-less self-documenting
>  (b) usable in other places too.
>  (c) maybe some future compiler wants that struct field to have some
> "randomize-me attribute" or something
>
> Hmm?

There were so many options and they all seems weird for various
reason. :) I'll use your idea, it looks much cleaner, thanks!

-Kees

-- 
Kees Cook
Pixel Security

[toc] | [prev] | [next] | [standalone]

#1651667 — Re: [PATCH v2 19/20] [RFC] task_struct: Allow randomized layout

On Fri, May 26, 2017 at 1:17 PM, Kees Cook <keescook@chromium.org> wrote:
> This marks most of the layout of task_struct as randomizable, but leaves
> thread_info and scheduler state untouched at the start, and thread_struct
> untouched at the end.

I think you want to abstract this out somehow, because this is both
ugly and bad:

> +       /* This begins the randomizable portion of task_struct... */
> +#if GCC_VERSION >= 40600
> +       struct {
> +#endif

when you could instead just introduce something like

#if GCC_VERSION >= 40600
  #define randomized_struct_fields_start struct {
  #define randomized_struct_fields_end } __randomize_layout;
#else
  #define randomized_struct_fields_start
  #define randomized_struct_fields_end
#endif

and then this pattern is
 (a) more-or-less self-documenting
 (b) usable in other places too.
 (c) maybe some future compiler wants that struct field to have some
"randomize-me attribute" or something

Hmm?

[toc] | [prev] | [next] | [standalone]

#1651708 — [PATCH v2 15/20] mtk-vcodec: Use designated initializers

The randstruct plugin requires designated initializers for structures
that are entirely function pointers.

Cc: Wu-Cheng Li <wuchengli@chromium.org>
Cc: PC Chen <pc.chen@mediatek.com>
Cc: Tiffany Lin <tiffany.lin@mediatek.com>
Cc: Hans Verkuil <hans.verkuil@cisco.com>
Cc: Mauro Carvalho Chehab <mchehab@s-opensource.com>
Signed-off-by: Kees Cook <keescook@chromium.org>
---
 drivers/media/platform/mtk-vcodec/vdec/vdec_h264_if.c | 8 ++++----
 drivers/media/platform/mtk-vcodec/vdec/vdec_vp8_if.c  | 8 ++++----
 drivers/media/platform/mtk-vcodec/vdec/vdec_vp9_if.c  | 8 ++++----
 3 files changed, 12 insertions(+), 12 deletions(-)

diff --git a/drivers/media/platform/mtk-vcodec/vdec/vdec_h264_if.c b/drivers/media/platform/mtk-vcodec/vdec/vdec_h264_if.c
index 57a842ff3097..b7731b18ecae 100644
--- a/drivers/media/platform/mtk-vcodec/vdec/vdec_h264_if.c
+++ b/drivers/media/platform/mtk-vcodec/vdec/vdec_h264_if.c
@@ -493,10 +493,10 @@ static int vdec_h264_get_param(unsigned long h_vdec,
 }
 
 static struct vdec_common_if vdec_h264_if = {
-	vdec_h264_init,
-	vdec_h264_decode,
-	vdec_h264_get_param,
-	vdec_h264_deinit,
+	.init		= vdec_h264_init,
+	.decode		= vdec_h264_decode,
+	.get_param	= vdec_h264_get_param,
+	.deinit		= vdec_h264_deinit,
 };
 
 struct vdec_common_if *get_h264_dec_comm_if(void);
diff --git a/drivers/media/platform/mtk-vcodec/vdec/vdec_vp8_if.c b/drivers/media/platform/mtk-vcodec/vdec/vdec_vp8_if.c
index 6e7a62ae0842..b9fad6a48879 100644
--- a/drivers/media/platform/mtk-vcodec/vdec/vdec_vp8_if.c
+++ b/drivers/media/platform/mtk-vcodec/vdec/vdec_vp8_if.c
@@ -620,10 +620,10 @@ static void vdec_vp8_deinit(unsigned long h_vdec)
 }
 
 static struct vdec_common_if vdec_vp8_if = {
-	vdec_vp8_init,
-	vdec_vp8_decode,
-	vdec_vp8_get_param,
-	vdec_vp8_deinit,
+	.init		= vdec_vp8_init,
+	.decode		= vdec_vp8_decode,
+	.get_param	= vdec_vp8_get_param,
+	.deinit		= vdec_vp8_deinit,
 };
 
 struct vdec_common_if *get_vp8_dec_comm_if(void);
diff --git a/drivers/media/platform/mtk-vcodec/vdec/vdec_vp9_if.c b/drivers/media/platform/mtk-vcodec/vdec/vdec_vp9_if.c
index 5539b1853f16..1daee1207469 100644
--- a/drivers/media/platform/mtk-vcodec/vdec/vdec_vp9_if.c
+++ b/drivers/media/platform/mtk-vcodec/vdec/vdec_vp9_if.c
@@ -979,10 +979,10 @@ static int vdec_vp9_get_param(unsigned long h_vdec,
 }
 
 static struct vdec_common_if vdec_vp9_if = {
-	vdec_vp9_init,
-	vdec_vp9_decode,
-	vdec_vp9_get_param,
-	vdec_vp9_deinit,
+	.init		= vdec_vp9_init,
+	.decode		= vdec_vp9_decode,
+	.get_param	= vdec_vp9_get_param,
+	.deinit		= vdec_vp9_deinit,
 };
 
 struct vdec_common_if *get_vp9_dec_comm_if(void);
-- 
2.7.4

[toc] | [prev] | [next] | [standalone]

#1651710 — [PATCH v2 14/20] drm/amd/powerplay: Use designated initializers

The randstruct plugin requires designated initializers for structures
that are entirely function pointers.

Cc: Christian König <christian.koenig@amd.com>
Cc: Eric Huang <JinHuiEric.Huang@amd.com>
Cc: Alex Deucher <alexander.deucher@amd.com>
Signed-off-by: Kees Cook <keescook@chromium.org>
---
 .../gpu/drm/amd/powerplay/hwmgr/vega10_thermal.c   | 28 ++++++++++++++--------
 1 file changed, 18 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/amd/powerplay/hwmgr/vega10_thermal.c b/drivers/gpu/drm/amd/powerplay/hwmgr/vega10_thermal.c
index d5f53d04fa08..cfa187185dcc 100644
--- a/drivers/gpu/drm/amd/powerplay/hwmgr/vega10_thermal.c
+++ b/drivers/gpu/drm/amd/powerplay/hwmgr/vega10_thermal.c
@@ -709,17 +709,22 @@ static int tf_vega10_thermal_disable_alert(struct pp_hwmgr *hwmgr,
 
 static struct phm_master_table_item
 vega10_thermal_start_thermal_controller_master_list[] = {
-	{NULL, tf_vega10_thermal_initialize},
-	{NULL, tf_vega10_thermal_set_temperature_range},
-	{NULL, tf_vega10_thermal_enable_alert},
+	{ .isFunctionNeededInRuntimeTable = NULL,
+	  .tableFunction = tf_vega10_thermal_initialize },
+	{ .isFunctionNeededInRuntimeTable = NULL,
+	  .tableFunction = tf_vega10_thermal_set_temperature_range },
+	{ .isFunctionNeededInRuntimeTable = NULL,
+	  .tableFunction = tf_vega10_thermal_enable_alert },
 /* We should restrict performance levels to low before we halt the SMC.
  * On the other hand we are still in boot state when we do this
  * so it would be pointless.
  * If this assumption changes we have to revisit this table.
  */
-	{NULL, tf_vega10_thermal_setup_fan_table},
-	{NULL, tf_vega10_thermal_start_smc_fan_control},
-	{NULL, NULL}
+	{ .isFunctionNeededInRuntimeTable = NULL,
+	  .tableFunction = tf_vega10_thermal_setup_fan_table },
+	{ .isFunctionNeededInRuntimeTable = NULL,
+	  .tableFunction = tf_vega10_thermal_start_smc_fan_control },
+	{ }
 };
 
 static struct phm_master_table_header
@@ -731,10 +736,13 @@ vega10_thermal_start_thermal_controller_master = {
 
 static struct phm_master_table_item
 vega10_thermal_set_temperature_range_master_list[] = {
-	{NULL, tf_vega10_thermal_disable_alert},
-	{NULL, tf_vega10_thermal_set_temperature_range},
-	{NULL, tf_vega10_thermal_enable_alert},
-	{NULL, NULL}
+	{ .isFunctionNeededInRuntimeTable = NULL,
+	  .tableFunction = tf_vega10_thermal_disable_alert },
+	{ .isFunctionNeededInRuntimeTable = NULL,
+	  .tableFunction = tf_vega10_thermal_set_temperature_range },
+	{ .isFunctionNeededInRuntimeTable = NULL,
+	  .tableFunction = tf_vega10_thermal_enable_alert },
+	{ }
 };
 
 struct phm_master_table_header
-- 
2.7.4

[toc] | [prev] | [next] | [standalone]

Page 1 of 2  [1] 2  Next page →

Back to top | Article view | linux.kernel

csiph-web