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

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

[Sebastian Parschauer]

Hi,

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.

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

Thanks,
Sebastian

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

[Kees Cook]

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

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

[Sebastian Parschauer]

On 16.07.2015 22:34, Kees Cook wrote:
> 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. :)

Thank you very much for your kind words!

Scanmem and GameConqueror were especially very inactive tools before
I've pushed further development to lead game cheating on Linux. PIE made
all noob tools unusable and discontinued. :-)

>> 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

Thank you very much for your efforts!

Backporting the patch set to 4.0.y was no problem but I ran into major
issues backporting it to 3.18.y. It plain touches too many archs and too
many other patches would be required additionally.
The easiest and most stable way would be to just create a completely new
patch to "beautify the ugly pie" but the linux-stable rules don't allow
this.

"It or an equivalent fix must already exist in Linus' tree (upstream)."

So I guess we have to "eat the ugly pie" and fix it in scanmem where the
complexity literally explodes. PIC/PIE regions detection is already
complex and has already a long explanation text in the source. Let's see
if I can explain the support for ugly PIE I've developed for scanmem to
the original maintainer Lu Wang to get it merged.

For the future please always keep the four ELF segments .text, .rodata,
.data and .bss together in memory so that we don't run into this
situation again. Thanks!

Cheers,
Sebastian

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

[Greg KH]

On Thu, Jul 16, 2015 at 01:34:16PM -0700, Kees Cook wrote:
> 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

Given that 4.0-stable is now end-of-life, and these are all in 4.1, I'm
not going to worry about these anymore, thanks.

greg k-h

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

[Michael Davidson]

With CONFIG_ARCH_BINFMT_ELF_RANDOMIZE_PIE enabled, and a normal
top-down address allocation strategy, load_elf_binary() will
attempt to map a PIE binary into an address range immediately
below mm->mmap_base.

Unfortunately, load_elf_ binary() does not take account of the
need to allocate sufficient space for the entire binary which
means that, while the first PT_LOAD segment is mapped below
mm->mmap_base, the subsequent PT_LOAD segment(s) end up being
mapped above mm->mmap_base into the are that is supposed to
be the "gap" between the stack and the binary.

Since the size of the "gap" on x86_64 is only guaranteed to be
128MB this means that binaries with large data segments > 128MB
can end up mapping part of their data segment over their stack
resulting in corruption of the stack (and the data segment once
the binary starts to run).

Any PIE binary with a data segment > 128MB is vulnerable to this
although address randomization means that the actual gap between
the stack and the end of the binary is normally greater than 128MB.
The larger the data segment of the binary the higher the probability
of failure.

Fix this by calculating the total size of the binary in the same
way as load_elf_interp().

Signed-off-by: Michael Davidson <md@google.com>
---
 fs/binfmt_elf.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/fs/binfmt_elf.c b/fs/binfmt_elf.c
index 995986b..d925f55 100644
--- a/fs/binfmt_elf.c
+++ b/fs/binfmt_elf.c
@@ -862,6 +862,7 @@ static int load_elf_binary(struct linux_binprm *bprm)
 	    i < loc->elf_ex.e_phnum; i++, elf_ppnt++) {
 		int elf_prot = 0, elf_flags;
 		unsigned long k, vaddr;
+		unsigned long total_size = 0;
 
 		if (elf_ppnt->p_type != PT_LOAD)
 			continue;
@@ -924,10 +925,16 @@ static int load_elf_binary(struct linux_binprm *bprm)
 #else
 			load_bias = ELF_PAGESTART(ELF_ET_DYN_BASE - vaddr);
 #endif
+			total_size = total_mapping_size(elf_phdata,
+							loc->elf_ex.e_phnum);
+			if (!total_size) {
+				error = -EINVAL;
+				goto out_free_dentry;
+			}
 		}
 
 		error = elf_map(bprm->file, load_bias + vaddr, elf_ppnt,
-				elf_prot, elf_flags, 0);
+				elf_prot, elf_flags, total_size);
 		if (BAD_ADDR(error)) {
 			retval = IS_ERR((void *)error) ?
 				PTR_ERR((void*)error) : -EINVAL;
-- 
2.2.0.rc0.207.ga3a616c

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

[James Hogan]

Hi Michael,

On 13 April 2015 at 23:49, Michael Davidson <md@google.com> wrote:
> With CONFIG_ARCH_BINFMT_ELF_RANDOMIZE_PIE enabled, and a normal
> top-down address allocation strategy, load_elf_binary() will
> attempt to map a PIE binary into an address range immediately
> below mm->mmap_base.
>
> Unfortunately, load_elf_ binary() does not take account of the
> need to allocate sufficient space for the entire binary which
> means that, while the first PT_LOAD segment is mapped below
> mm->mmap_base, the subsequent PT_LOAD segment(s) end up being
> mapped above mm->mmap_base into the are that is supposed to
> be the "gap" between the stack and the binary.
>
> Since the size of the "gap" on x86_64 is only guaranteed to be
> 128MB this means that binaries with large data segments > 128MB
> can end up mapping part of their data segment over their stack
> resulting in corruption of the stack (and the data segment once
> the binary starts to run).
>
> Any PIE binary with a data segment > 128MB is vulnerable to this
> although address randomization means that the actual gap between
> the stack and the end of the binary is normally greater than 128MB.
> The larger the data segment of the binary the higher the probability
> of failure.
>
> Fix this by calculating the total size of the binary in the same
> way as load_elf_interp().
>
> Signed-off-by: Michael Davidson <md@google.com>
> ---
>  fs/binfmt_elf.c | 9 ++++++++-
>  1 file changed, 8 insertions(+), 1 deletion(-)
>
> diff --git a/fs/binfmt_elf.c b/fs/binfmt_elf.c
> index 995986b..d925f55 100644
> --- a/fs/binfmt_elf.c
> +++ b/fs/binfmt_elf.c
> @@ -862,6 +862,7 @@ static int load_elf_binary(struct linux_binprm *bprm)
>             i < loc->elf_ex.e_phnum; i++, elf_ppnt++) {
>                 int elf_prot = 0, elf_flags;
>                 unsigned long k, vaddr;
> +               unsigned long total_size = 0;
>
>                 if (elf_ppnt->p_type != PT_LOAD)
>                         continue;
> @@ -924,10 +925,16 @@ static int load_elf_binary(struct linux_binprm *bprm)
>  #else
>                         load_bias = ELF_PAGESTART(ELF_ET_DYN_BASE - vaddr);
>  #endif
> +                       total_size = total_mapping_size(elf_phdata,
> +                                                       loc->elf_ex.e_phnum);
> +                       if (!total_size) {
> +                               error = -EINVAL;

I was just printk debugging this function, and this stood out. Should
that be retval instead of error?

Cheers
James

> +                               goto out_free_dentry;
> +                       }
>                 }
>
>                 error = elf_map(bprm->file, load_bias + vaddr, elf_ppnt,
> -                               elf_prot, elf_flags, 0);
> +                               elf_prot, elf_flags, total_size);
>                 if (BAD_ADDR(error)) {
>                         retval = IS_ERR((void *)error) ?
>                                 PTR_ERR((void*)error) : -EINVAL;
> --
> 2.2.0.rc0.207.ga3a616c
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/