linux-kernel - Re: [PATCH v2] task_struct: stack_canary is not needed without CC

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20090818134328.GA28366@elte.hu>
Date:	Tue, 18 Aug 2009 15:43:28 +0200
From:	Ingo Molnar <mingo@...e.hu>
To:	Hiroshi Shimamoto <h-shimamoto@...jp.nec.com>
Cc:	Andi Kleen <andi@...stfloor.org>, linux-kernel@...r.kernel.org,
	Arjan van de Ven <arjan@...radead.org>,
	Thomas Gleixner <tglx@...utronix.de>
Subject: Re: [PATCH v2] task_struct: stack_canary is not needed without
	CC_STACKPROTECTOR

* Hiroshi Shimamoto <h-shimamoto@...jp.nec.com> wrote:

> Andi Kleen wrote:
> > Hiroshi Shimamoto <h-shimamoto@...jp.nec.com> writes:
> > 
> >> From: Hiroshi Shimamoto <h-shimamoto@...jp.nec.com>
> >>
> >> The field stack_canary is only used with CC_STACKPROTECTOR.
> >> This patch reduces task_struct size without CC_STACKPROTECTOR.
> > 
> > Adding a ifdef in the middle of a widely used structure is 
> > nasty.  It means that if someone changes the option then the 
> > newly loaded modules don't work anymore (yes that's not 
> > officially supported, but works most of the time and is often 
> > convenient in practice)

( Ugh. Not having clean builds and clean modules is utterly 
  dangerous and taints the kernel. I ignore all bugreports from
  people that do that - a kernel that has been butchered like that
  is just not trustable. )

> > So when you add a ifdef please move the field to the end at 
> > least.

Moving the stack canary it last is futile and makes no sense 
whatsoever, for three independent reasons:

It's stupidly shortsighted: there's 20 other config options in the 
middle of struct task struct already. Half of struct task_struct is
#ifdef-ed, and there can only be one 'last' field.

It's merge unfriendly: moving fields last in structs can cause 
patch conflict problems: new subsystems/features tend to append to 
task_struct, colliding with this patch. task_struct is frequently 
patched.

It hurts performance: the canary is used very frequently on 
stackprotector kernels and has been placed on a hot cacheline 
intentionally. Moving it last just adds a small but real 
performance regression.

Really, Andi, if you give 'advice' like this you should be declared 
armed and dangerous ... ;-)

> Here's the update.

I've applied v1, thanks Hiroshi!

	Ingo
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/