| Message ID | 1495829844-69341-6-git-send-email-keescook@chromium.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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.
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 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.
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
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.
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" }, { } };
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(+)