Re: [PATCH] binfmt_elf: Fix bug in loading of PIE binaries

From: Kees Cook <keescook@chromium.org>
To: Sebastian Parschauer <s.parschauer@gmx.de>,
	Greg KH <gregkh@linuxfoundation.org>,
	Ingo Molnar <mingo@kernel.org>
Cc: Michael Davidson <md@google.com>,
	Alexander Viro <viro@zeniv.linux.org.uk>,
	Jiri Kosina <jkosina@suse.cz>,
	"linux-fsdevel@vger.kernel.org" <linux-fsdevel@vger.kernel.org>,
	Ubuntu Kernel Team <kernel-team@lists.ubuntu.com>
Subject: Re: [PATCH] binfmt_elf: Fix bug in loading of PIE binaries
Date: Thu, 16 Jul 2015 13:34:16 -0700	[thread overview]
Message-ID: <CAGXu5j+W4zBeime=D6ibuZsRw+RCgXJQCOg4V1OaMaEHAdfbZA@mail.gmail.com> (raw)
In-Reply-To: <55A80CA5.2000104@gmx.de>

On Thu, Jul 16, 2015 at 12:57 PM, Sebastian Parschauer
<s.parschauer@gmx.de> wrote:
> I'm a professional Linux game cheater and the co-maintainer of scanmem.
> With scanmem we determine the load addresses for PIC and PIE binaries to
> be able to support static memory cheating with ASLR. At the moment
> ugtrain is the only universal game trainer able to determine the PIE
> load address as well and to re-add it to the found match offset from
> scanmem.

scanmem is such a fun tool. :)

> I'd like to complain a bit about this patch as it makes the address
> space layout for the executable really ugly by loading unrelated stuff
> between .text and .rodata.
>
> Is it really required on top of 3.13 or 3.16 where Ubuntu has put it?
>
> I've also checked v4.2-rc1. There everything is beautiful again.
> Thank you very much for that!
>
> References:
> https://github.com/scanmem/scanmem/issues/122
> https://github.com/ugtrain/ugtrain

To summarize the two commits:

This commit fixed a problem where PIE binaries could collide with
other memory regions, but breaks scanmem:
https://git.kernel.org/linus/a87938b2e246b81b4fb713edb371a9fa3c5c3c86
It was sent to stable (and various distros like Ubuntu picked it up),
since it was (correctly) seen as a bug fix (without realizing it broke
other programs).

This commit reorganized PIE ASLR to split it from mmap ASLR:
https://git.kernel.org/linus/a87938b2e246b81b4fb713edb371a9fa3c5c3c86
This was part of a larger series of patches that did refactoring
across multiple architectures, and was not sent to stable (since it is
considered more of a feature than a bug fix).

I believe it should be safe to backport the series, but not every
kernel team will be willing to do that on their own. As the series has
been living fine since 4.1, I could be convinced that it should be
sent to stable. It does fix an ASLR weakness, and it does fix a
situation where the other commit that got sent to stable breaks
existing userspace tools. I think both of those factors together
warrants sending the series to stable.

Greg, would you be willing to take these into stable?

fbbc400f3924ce095b466c776dc294727ec0a202
82168140bc4cec7ec9bad39705518541149ff8b7
dd04cff1dceab18226853b555cf07914648a235f
1f0569df0b0285e7ec2432d804a4921b06a61618
ed6322746afb74c2509e2f3a6464182793b16eb9
8e89a356feb6f196824a72101861d931a97ac2d2
2b68f6caeac271620cd2f9362aeaed360e317df0
c6f5b001e65cdac592b65a08c5d2dd179cfba568
d1fd836dcf00d2028c700c7e44d2c23404062c90
204db6ed17743000691d930368a5abd6ea541c58

Thanks!

-Kees

-- 
Kees Cook
Chrome OS Security