[RFC PATCH] binfmt_elf: Fix PIE execution with randomization disabled

[RFC PATCH] binfmt_elf: Fix PIE execution with randomization disabled

[Josh Boyer]

We've had a bug report[1] of some PIE programs getting a SIGKILL upon exec
if you disable address randomization with:

	echo 0 > /proc/sys/kernel/randomize_va_space

I tracked this down to get_unmapped_area_prot returning -ENOMEM because
the address being passed in is larger than TASK_SIZE - len for the bss
section of the test executable.  That filters back to set_brk returning
an error to load_elf_binary and the SIGKILL being sent around line 872
of binfmt_elf.c.

H.J. submitted an upstream bug report [2] as well, but got no feedback
and we can't view it with kernel.org being down anyway.  He came up with
the patch below as well, which is what I'm sending on for comments.  The
changelog is my addition, so if that is wrong yell at me.

I wanted to get some more eyes on this, because the current code sets
load_bias to 0 unconditionally on CONFIG_X86 or CONFIG_ARM.  I have no
idea why that is.  The original execshield patches had an #ifdef on
__i386__ but the patch that was commited to add PIE support has the
CONFIG_X86 setting.

Thoughts welcome.

[1] https://bugzilla.redhat.com/show_bug.cgi?id=708563
[2] http://bugzilla.kernel.org/show_bug.cgi?id=36372

josh

---

From: H.J. Lu <hongjiu.lu@intel.com>

Set the load_bias for PIE executables to a non-zero address if no virtual
address is specified.  This prevents us from running out of room for all
the various loadable segments when ASLR is disabled.

Signed-off-by: H.J. Lu <hongjiu.lu@intel.com>
Signed-off-by: Josh Boyer <jwboyer@redhat.com>

---

diff --git a/fs/binfmt_elf.c b/fs/binfmt_elf.c
index 303983f..069ee29 100644
--- a/fs/binfmt_elf.c
+++ b/fs/binfmt_elf.c
@@ -794,9 +794,14 @@ static int load_elf_binary(struct linux_binprm *bprm, struct pt_regs *regs)
 			/* Try and get dynamic programs out of the way of the
 			 * default mmap base, as well as whatever program they
 			 * might try to exec.  This is because the brk will
-			 * follow the loader, and is not movable.  */
+			 * follow the loader, and is not movable.  Don't use
+			 * 0 load address since we may not have room for
+			 * all loadable segements.  */
 #if defined(CONFIG_X86) || defined(CONFIG_ARM)
-			load_bias = 0;
+			if (vaddr)
+				load_bias = 0;
+			else
+				load_bias = ELF_PAGESTART(ELF_ET_DYN_BASE);
 #else
 			load_bias = ELF_PAGESTART(ELF_ET_DYN_BASE - vaddr);
 #endif

Re: [RFC PATCH] binfmt_elf: Fix PIE execution with randomization disabled

[Andrew Morton]

On Thu, 29 Sep 2011 15:53:59 -0400
Josh Boyer <jwboyer@redhat.com> wrote:

> We've had a bug report[1] of some PIE programs getting a SIGKILL upon exec
> if you disable address randomization with:
> 
> 	echo 0 > /proc/sys/kernel/randomize_va_space
> 
> I tracked this down to get_unmapped_area_prot returning -ENOMEM because
> the address being passed in is larger than TASK_SIZE - len for the bss
> section of the test executable.  That filters back to set_brk returning
> an error to load_elf_binary and the SIGKILL being sent around line 872
> of binfmt_elf.c.
> 
> H.J. submitted an upstream bug report [2] as well, but got no feedback
> and we can't view it with kernel.org being down anyway.  He came up with
> the patch below as well, which is what I'm sending on for comments.  The
> changelog is my addition, so if that is wrong yell at me.
> 
> I wanted to get some more eyes on this, because the current code sets
> load_bias to 0 unconditionally on CONFIG_X86 or CONFIG_ARM.  I have no
> idea why that is.  The original execshield patches had an #ifdef on
> __i386__ but the patch that was commited to add PIE support has the
> CONFIG_X86 setting.
> 

It appears that Nicolas understood what's going on in there when he
wrote e4eab08d6050ad0 ("ARM: 6342/1: fix ASLR of PIE executables"). 
Alas, that patch's changelog is rather useless.

Help?

Also, please: review and test?

> 
> [1] https://bugzilla.redhat.com/show_bug.cgi?id=708563
> [2] http://bugzilla.kernel.org/show_bug.cgi?id=36372
> 
> josh
> 
> ---
> 
> From: H.J. Lu <hongjiu.lu@intel.com>
> 
> Set the load_bias for PIE executables to a non-zero address if no virtual
> address is specified.  This prevents us from running out of room for all
> the various loadable segments when ASLR is disabled.
> 
> Signed-off-by: H.J. Lu <hongjiu.lu@intel.com>
> Signed-off-by: Josh Boyer <jwboyer@redhat.com>
> 
> ---
> 
> diff --git a/fs/binfmt_elf.c b/fs/binfmt_elf.c
> index 303983f..069ee29 100644
> --- a/fs/binfmt_elf.c
> +++ b/fs/binfmt_elf.c
> @@ -794,9 +794,14 @@ static int load_elf_binary(struct linux_binprm *bprm, struct pt_regs *regs)
>  			/* Try and get dynamic programs out of the way of the
>  			 * default mmap base, as well as whatever program they
>  			 * might try to exec.  This is because the brk will
> -			 * follow the loader, and is not movable.  */
> +			 * follow the loader, and is not movable.  Don't use
> +			 * 0 load address since we may not have room for
> +			 * all loadable segements.  */
>  #if defined(CONFIG_X86) || defined(CONFIG_ARM)
> -			load_bias = 0;
> +			if (vaddr)
> +				load_bias = 0;
> +			else
> +				load_bias = ELF_PAGESTART(ELF_ET_DYN_BASE);
>  #else
>  			load_bias = ELF_PAGESTART(ELF_ET_DYN_BASE - vaddr);
>  #endif

RE: [RFC PATCH] binfmt_elf: Fix PIE execution with randomization disabled

[Lu, Hongjiu]

> On Thu, 29 Sep 2011 15:53:59 -0400
> Josh Boyer <jwboyer@redhat.com> wrote:
> 
> > We've had a bug report[1] of some PIE programs getting a SIGKILL upon exec
> > if you disable address randomization with:
> >
> > 	echo 0 > /proc/sys/kernel/randomize_va_space
> >
> > I tracked this down to get_unmapped_area_prot returning -ENOMEM because
> > the address being passed in is larger than TASK_SIZE - len for the bss
> > section of the test executable.  That filters back to set_brk returning
> > an error to load_elf_binary and the SIGKILL being sent around line 872
> > of binfmt_elf.c.
> >
> > H.J. submitted an upstream bug report [2] as well, but got no feedback
> > and we can't view it with kernel.org being down anyway.  He came up with
> > the patch below as well, which is what I'm sending on for comments.  The
> > changelog is my addition, so if that is wrong yell at me.
> >
> > I wanted to get some more eyes on this, because the current code sets
> > load_bias to 0 unconditionally on CONFIG_X86 or CONFIG_ARM.  I have no
> > idea why that is.  The original execshield patches had an #ifdef on
> > __i386__ but the patch that was commited to add PIE support has the
> > CONFIG_X86 setting.
> >
> 
> It appears that Nicolas understood what's going on in there when he
> wrote e4eab08d6050ad0 ("ARM: 6342/1: fix ASLR of PIE executables").
> Alas, that patch's changelog is rather useless.

There is a very small testcase in the kernel bug report.  If I remember
it correctly, it has something to do with the size of bss section.  It
fails with about 180MB bss section.

H.J.
> Help?
> 
> Also, please: review and test?
> 
> >
> > [1] https://bugzilla.redhat.com/show_bug.cgi?id=708563
> > [2] http://bugzilla.kernel.org/show_bug.cgi?id=36372
> >
> > josh
> >
> > ---
> >
> > From: H.J. Lu <hongjiu.lu@intel.com>
> >
> > Set the load_bias for PIE executables to a non-zero address if no virtual
> > address is specified.  This prevents us from running out of room for all
> > the various loadable segments when ASLR is disabled.
> >
> > Signed-off-by: H.J. Lu <hongjiu.lu@intel.com>
> > Signed-off-by: Josh Boyer <jwboyer@redhat.com>
> >
> > ---
> >
> > diff --git a/fs/binfmt_elf.c b/fs/binfmt_elf.c
> > index 303983f..069ee29 100644
> > --- a/fs/binfmt_elf.c
> > +++ b/fs/binfmt_elf.c
> > @@ -794,9 +794,14 @@ static int load_elf_binary(struct linux_binprm *bprm,
> struct pt_regs *regs)
> >  			/* Try and get dynamic programs out of the way of the
> >  			 * default mmap base, as well as whatever program they
> >  			 * might try to exec.  This is because the brk will
> > -			 * follow the loader, and is not movable.  */
> > +			 * follow the loader, and is not movable.  Don't use
> > +			 * 0 load address since we may not have room for
> > +			 * all loadable segements.  */
> >  #if defined(CONFIG_X86) || defined(CONFIG_ARM)
> > -			load_bias = 0;
> > +			if (vaddr)
> > +				load_bias = 0;
> > +			else
> > +				load_bias = ELF_PAGESTART(ELF_ET_DYN_BASE);
> >  #else
> >  			load_bias = ELF_PAGESTART(ELF_ET_DYN_BASE - vaddr);
> >  #endif

Re: [RFC PATCH] binfmt_elf: Fix PIE execution with randomization disabled

[Nicolas Pitre]

On Thu, 29 Sep 2011, Andrew Morton wrote:

> On Thu, 29 Sep 2011 15:53:59 -0400
> Josh Boyer <jwboyer@redhat.com> wrote:
> 
> > We've had a bug report[1] of some PIE programs getting a SIGKILL upon exec
> > if you disable address randomization with:
> > 
> > 	echo 0 > /proc/sys/kernel/randomize_va_space
> > 
> > I tracked this down to get_unmapped_area_prot returning -ENOMEM because
> > the address being passed in is larger than TASK_SIZE - len for the bss
> > section of the test executable.  That filters back to set_brk returning
> > an error to load_elf_binary and the SIGKILL being sent around line 872
> > of binfmt_elf.c.
> > 
> > H.J. submitted an upstream bug report [2] as well, but got no feedback
> > and we can't view it with kernel.org being down anyway.  He came up with
> > the patch below as well, which is what I'm sending on for comments.  The
> > changelog is my addition, so if that is wrong yell at me.
> > 
> > I wanted to get some more eyes on this, because the current code sets
> > load_bias to 0 unconditionally on CONFIG_X86 or CONFIG_ARM.  I have no
> > idea why that is.  The original execshield patches had an #ifdef on
> > __i386__ but the patch that was commited to add PIE support has the
> > CONFIG_X86 setting.
> > 
> 
> It appears that Nicolas understood what's going on in there when he
> wrote e4eab08d6050ad0 ("ARM: 6342/1: fix ASLR of PIE executables"). 
> Alas, that patch's changelog is rather useless.
> 
> Help?

Well, in order to obtain randomization, the addr argument to elf_map() 
must be zero to eventually let arch_get_unmapped_area() do its job of 
selecting a random address.

Since only X86 supported ASLR at the time, I simply did the same for ARM 
i.e. let load_bias be set to 0 so elf_map() would get a zero address.

> Also, please: review and test?
> 
> >  #if defined(CONFIG_X86) || defined(CONFIG_ARM)
> > -			load_bias = 0;
> > +			if (vaddr)
> > +				load_bias = 0;
> > +			else
> > +				load_bias = ELF_PAGESTART(ELF_ET_DYN_BASE);
> >  #else
> >  			load_bias = ELF_PAGESTART(ELF_ET_DYN_BASE - vaddr);
> >  #endif

Simply looking at this patch, I don't see how the second argument to 
elf_map() called as follows could ever be zero anymore, effectively 
breaking ASLR.

                error = elf_map(bprm->file, load_bias + vaddr, elf_ppnt,
                                elf_prot, elf_flags, 0);


Nicolas

Re: [RFC PATCH] binfmt_elf: Fix PIE execution with randomization disabled

[Josh Boyer]

On Thu, Sep 29, 2011 at 08:41:33PM -0400, Nicolas Pitre wrote:
> On Thu, 29 Sep 2011, Andrew Morton wrote:
> 
> > On Thu, 29 Sep 2011 15:53:59 -0400
> > Josh Boyer <jwboyer@redhat.com> wrote:
> > 
> > > We've had a bug report[1] of some PIE programs getting a SIGKILL upon exec
> > > if you disable address randomization with:
> > > 
> > > 	echo 0 > /proc/sys/kernel/randomize_va_space
> > > 
> > > I tracked this down to get_unmapped_area_prot returning -ENOMEM because
> > > the address being passed in is larger than TASK_SIZE - len for the bss
> > > section of the test executable.  That filters back to set_brk returning
> > > an error to load_elf_binary and the SIGKILL being sent around line 872
> > > of binfmt_elf.c.
> > > 
> > > H.J. submitted an upstream bug report [2] as well, but got no feedback
> > > and we can't view it with kernel.org being down anyway.  He came up with
> > > the patch below as well, which is what I'm sending on for comments.  The
> > > changelog is my addition, so if that is wrong yell at me.
> > > 
> > > I wanted to get some more eyes on this, because the current code sets
> > > load_bias to 0 unconditionally on CONFIG_X86 or CONFIG_ARM.  I have no
> > > idea why that is.  The original execshield patches had an #ifdef on
> > > __i386__ but the patch that was commited to add PIE support has the
> > > CONFIG_X86 setting.
> > > 
> > 
> > It appears that Nicolas understood what's going on in there when he
> > wrote e4eab08d6050ad0 ("ARM: 6342/1: fix ASLR of PIE executables"). 
> > Alas, that patch's changelog is rather useless.
> > 
> > Help?
> 
> Well, in order to obtain randomization, the addr argument to elf_map() 
> must be zero to eventually let arch_get_unmapped_area() do its job of 
> selecting a random address.

Hm.  But it doesn't do that if ASLR is disabled at runtime, so the
address causes issues?

> Since only X86 supported ASLR at the time, I simply did the same for ARM 
> i.e. let load_bias be set to 0 so elf_map() would get a zero address.

So as of right now, only X86 and ARM support ASLR?  Maybe we could
change the define to be more descriptive.  Something like ARCH_HAS_ASLR?

> > Also, please: review and test?
> > 
> > >  #if defined(CONFIG_X86) || defined(CONFIG_ARM)
> > > -			load_bias = 0;
> > > +			if (vaddr)
> > > +				load_bias = 0;
> > > +			else
> > > +				load_bias = ELF_PAGESTART(ELF_ET_DYN_BASE);
> > >  #else
> > >  			load_bias = ELF_PAGESTART(ELF_ET_DYN_BASE - vaddr);
> > >  #endif
> 
> Simply looking at this patch, I don't see how the second argument to 
> elf_map() called as follows could ever be zero anymore, effectively 
> breaking ASLR.

Perhaps another check here for randomize?  Something like:

#if defined(CONFIG_X86) || defined(CONFIG_ARM)
		if (current->flags & PF_RANDOMIZE)
			load_bias = 0;
		else if (vaddr)
			load_bias = 0;
		else
			load_bias = ELF_PAGESTART(ELF_ET_DYN_BASE);
#else
		load_bias = ELF_PAGESTART(ELF_ET_DYN_BASE - vaddr);
#endif

If that's stupid, then feel free to tell me.  I won't pretend like I
understand what is going on here yet, but based on the explanation you
provided that might work.

josh

Re: [RFC PATCH] binfmt_elf: Fix PIE execution with randomization disabled

[Jiri Kosina]

On Thu, 29 Sep 2011, Josh Boyer wrote:

> We've had a bug report[1] of some PIE programs getting a SIGKILL upon exec
> if you disable address randomization with:
> 
> 	echo 0 > /proc/sys/kernel/randomize_va_space
> 
> I tracked this down to get_unmapped_area_prot returning -ENOMEM because
> the address being passed in is larger than TASK_SIZE - len for the bss
> section of the test executable.  That filters back to set_brk returning
> an error to load_elf_binary and the SIGKILL being sent around line 872
> of binfmt_elf.c.
> 
> H.J. submitted an upstream bug report [2] as well, but got no feedback
> and we can't view it with kernel.org being down anyway.  He came up with
> the patch below as well, which is what I'm sending on for comments.  The
> changelog is my addition, so if that is wrong yell at me.
> 
> I wanted to get some more eyes on this, because the current code sets
> load_bias to 0 unconditionally on CONFIG_X86 or CONFIG_ARM.  I have no
> idea why that is.  The original execshield patches had an #ifdef on
> __i386__ but the patch that was commited to add PIE support has the
> CONFIG_X86 setting.
> 
> Thoughts welcome.
> 
> [1] https://bugzilla.redhat.com/show_bug.cgi?id=708563
> [2] http://bugzilla.kernel.org/show_bug.cgi?id=36372
> 
> josh
> 
> ---
> 
> From: H.J. Lu <hongjiu.lu@intel.com>
> 
> Set the load_bias for PIE executables to a non-zero address if no virtual
> address is specified.  This prevents us from running out of room for all
> the various loadable segments when ASLR is disabled.
> 
> Signed-off-by: H.J. Lu <hongjiu.lu@intel.com>
> Signed-off-by: Josh Boyer <jwboyer@redhat.com>
> 
> ---
> 
> diff --git a/fs/binfmt_elf.c b/fs/binfmt_elf.c
> index 303983f..069ee29 100644
> --- a/fs/binfmt_elf.c
> +++ b/fs/binfmt_elf.c
> @@ -794,9 +794,14 @@ static int load_elf_binary(struct linux_binprm *bprm, struct pt_regs *regs)
>  			/* Try and get dynamic programs out of the way of the
>  			 * default mmap base, as well as whatever program they
>  			 * might try to exec.  This is because the brk will
> -			 * follow the loader, and is not movable.  */
> +			 * follow the loader, and is not movable.  Don't use
> +			 * 0 load address since we may not have room for
> +			 * all loadable segements.  */
>  #if defined(CONFIG_X86) || defined(CONFIG_ARM)
> -			load_bias = 0;
> +			if (vaddr)
> +				load_bias = 0;
> +			else
> +				load_bias = ELF_PAGESTART(ELF_ET_DYN_BASE);

If you do this unconditionally, I can't see how ASLR could be working at 
all -- this patch causes that elf_addr() is always called with non-zero 
'addr' argument, right? (haven't verified by actually testing the patch 
yet, but I fail to see how come this simply doesn't break ASLR 
completely).

Checking for PF_RANDOMIZE flag should be the way to go here.

-- 
Jiri Kosina
SUSE Labs

Re: [RFC PATCH] binfmt_elf: Fix PIE execution with randomization disabled

[Josh Boyer]

On Mon, Oct 03, 2011 at 02:10:26PM +0200, Jiri Kosina wrote:
> On Thu, 29 Sep 2011, Josh Boyer wrote:
> 
> > We've had a bug report[1] of some PIE programs getting a SIGKILL upon exec
> > if you disable address randomization with:
> > 
> > 	echo 0 > /proc/sys/kernel/randomize_va_space
> > 
> > I tracked this down to get_unmapped_area_prot returning -ENOMEM because
> > the address being passed in is larger than TASK_SIZE - len for the bss
> > section of the test executable.  That filters back to set_brk returning
> > an error to load_elf_binary and the SIGKILL being sent around line 872
> > of binfmt_elf.c.
> > 
> > H.J. submitted an upstream bug report [2] as well, but got no feedback
> > and we can't view it with kernel.org being down anyway.  He came up with
> > the patch below as well, which is what I'm sending on for comments.  The
> > changelog is my addition, so if that is wrong yell at me.
> > 
> > I wanted to get some more eyes on this, because the current code sets
> > load_bias to 0 unconditionally on CONFIG_X86 or CONFIG_ARM.  I have no
> > idea why that is.  The original execshield patches had an #ifdef on
> > __i386__ but the patch that was commited to add PIE support has the
> > CONFIG_X86 setting.
> > 
> > Thoughts welcome.
> > 
> > [1] https://bugzilla.redhat.com/show_bug.cgi?id=708563
> > [2] http://bugzilla.kernel.org/show_bug.cgi?id=36372
> > 
> > josh
> > 
> > ---
> > 
> > From: H.J. Lu <hongjiu.lu@intel.com>
> > 
> > Set the load_bias for PIE executables to a non-zero address if no virtual
> > address is specified.  This prevents us from running out of room for all
> > the various loadable segments when ASLR is disabled.
> > 
> > Signed-off-by: H.J. Lu <hongjiu.lu@intel.com>
> > Signed-off-by: Josh Boyer <jwboyer@redhat.com>
> > 
> > ---
> > 
> > diff --git a/fs/binfmt_elf.c b/fs/binfmt_elf.c
> > index 303983f..069ee29 100644
> > --- a/fs/binfmt_elf.c
> > +++ b/fs/binfmt_elf.c
> > @@ -794,9 +794,14 @@ static int load_elf_binary(struct linux_binprm *bprm, struct pt_regs *regs)
> >  			/* Try and get dynamic programs out of the way of the
> >  			 * default mmap base, as well as whatever program they
> >  			 * might try to exec.  This is because the brk will
> > -			 * follow the loader, and is not movable.  */
> > +			 * follow the loader, and is not movable.  Don't use
> > +			 * 0 load address since we may not have room for
> > +			 * all loadable segements.  */
> >  #if defined(CONFIG_X86) || defined(CONFIG_ARM)
> > -			load_bias = 0;
> > +			if (vaddr)
> > +				load_bias = 0;
> > +			else
> > +				load_bias = ELF_PAGESTART(ELF_ET_DYN_BASE);
> 
> If you do this unconditionally, I can't see how ASLR could be working at 
> all -- this patch causes that elf_addr() is always called with non-zero 
> 'addr' argument, right? (haven't verified by actually testing the patch 
> yet, but I fail to see how come this simply doesn't break ASLR 
> completely).
> 
> Checking for PF_RANDOMIZE flag should be the way to go here.

I sent out a pseudo-patch that did that (I think).  Could you take a
look and let me know if that seems more appropriate?

josh

Re: [RFC PATCH] binfmt_elf: Fix PIE execution with randomization disabled

[Jiri Kosina]

On Thu, 29 Sep 2011, Josh Boyer wrote:

> Perhaps another check here for randomize?  Something like:
> 
> #if defined(CONFIG_X86) || defined(CONFIG_ARM)
> 		if (current->flags & PF_RANDOMIZE)
> 			load_bias = 0;
> 		else if (vaddr)
> 			load_bias = 0;
> 		else
> 			load_bias = ELF_PAGESTART(ELF_ET_DYN_BASE);
> #else
> 		load_bias = ELF_PAGESTART(ELF_ET_DYN_BASE - vaddr);
> #endif
> 
> If that's stupid, then feel free to tell me.  I won't pretend like I
> understand what is going on here yet, but based on the explanation you
> provided that might work.

I have just verified my hunch that the original patch from H.J. / Josh 
breaks ASLR completely, so Andrew, please drop it for now.

I am now looking into how to fix things properly.

Josh, looking at what you are proposing -- do you see any reason to make 
the behavior different in #else branch and in !(current->flags & 
PF_RANDOMIZE) case?

-- 
Jiri Kosina
SUSE Labs

Re: [RFC PATCH] binfmt_elf: Fix PIE execution with randomization disabled

[Josh Boyer]

On Mon, Oct 03, 2011 at 04:53:34PM +0200, Jiri Kosina wrote:
> On Thu, 29 Sep 2011, Josh Boyer wrote:
> 
> > Perhaps another check here for randomize?  Something like:
> > 
> > #if defined(CONFIG_X86) || defined(CONFIG_ARM)
> > 		if (current->flags & PF_RANDOMIZE)
> > 			load_bias = 0;
> > 		else if (vaddr)
> > 			load_bias = 0;
> > 		else
> > 			load_bias = ELF_PAGESTART(ELF_ET_DYN_BASE);
> > #else
> > 		load_bias = ELF_PAGESTART(ELF_ET_DYN_BASE - vaddr);
> > #endif
> > 
> > If that's stupid, then feel free to tell me.  I won't pretend like I
> > understand what is going on here yet, but based on the explanation you
> > provided that might work.
> 
> I have just verified my hunch that the original patch from H.J. / Josh 
> breaks ASLR completely, so Andrew, please drop it for now.

Yes, please drop the original.

> I am now looking into how to fix things properly.
> 
> Josh, looking at what you are proposing -- do you see any reason to make 
> the behavior different in #else branch and in !(current->flags & 
> PF_RANDOMIZE) case?

I was mostly just trying to adapt H.J.'s patch to account for the
PF_RANDOMIZE case.  Looking at it a bit more, I'm not sure why they
would need to be different.  H.J., do you recall why you made that
change originally?

josh

[PATCH v2] binfmt_elf: Fix PIE execution with randomization disabled (was Re: [RFC PATCH] binfmt_elf: Fix PIE execution with randomization disabled)

[Jiri Kosina]

On Mon, 3 Oct 2011, Josh Boyer wrote:

> > > Perhaps another check here for randomize?  Something like:
> > > 
> > > #if defined(CONFIG_X86) || defined(CONFIG_ARM)
> > > 		if (current->flags & PF_RANDOMIZE)
> > > 			load_bias = 0;
> > > 		else if (vaddr)
> > > 			load_bias = 0;
> > > 		else
> > > 			load_bias = ELF_PAGESTART(ELF_ET_DYN_BASE);
> > > #else
> > > 		load_bias = ELF_PAGESTART(ELF_ET_DYN_BASE - vaddr);
> > > #endif
> > > 
> > > If that's stupid, then feel free to tell me.  I won't pretend like I
> > > understand what is going on here yet, but based on the explanation you
> > > provided that might work.
> > 
> > I have just verified my hunch that the original patch from H.J. / Josh 
> > breaks ASLR completely, so Andrew, please drop it for now.
> 
> Yes, please drop the original.

Thanks.

> > I am now looking into how to fix things properly.
> > 
> > Josh, looking at what you are proposing -- do you see any reason to make 
> > the behavior different in #else branch and in !(current->flags & 
> > PF_RANDOMIZE) case?
> 
> I was mostly just trying to adapt H.J.'s patch to account for the
> PF_RANDOMIZE case.  Looking at it a bit more, I'm not sure why they
> would need to be different.  H.J., do you recall why you made that
> change originally?

How about the patch below instead? It survives my testing, and I believe 
it handles both cases properly.

Confirmation from the original bug reporter would obviously be a nice 
bonus too :)




From: Jiri Kosina <jkosina@suse.cz>
Subject: [PATCH] binfmt_elf: fix PIE execution with randomization disabled

The case of address space randomization being disabled in runtime through
randomize_va_space sysctl is not treated properly in load_elf_binary(),
resulting in SIGKILL coming at exec() time for certain PIE-linked binaries
in case the randomization has been disabled at runtime prior to calling
exec().

Handle the randomize_va_space == 0 case the same way as if we were not
supporting .text randomization at all.

Based on original patch by H.J. Lu <hongjiu.lu@intel.com> and
Josh Boyer <jwboyer@redhat.com>

Cc: Ingo Molnar <mingo@elte.hu>
Cc: Jiri Kosina <jkosina@suse.cz>
Cc: Nicolas Pitre <nicolas.pitre@linaro.org>
Cc: Russell King <rmk@arm.linux.org.uk>
Signed-off-by: Jiri Kosina <jkosina@suse.cz>
---
 fs/binfmt_elf.c |    5 ++++-
 1 files changed, 4 insertions(+), 1 deletions(-)

diff --git a/fs/binfmt_elf.c b/fs/binfmt_elf.c
index dd0fdfc..bb11fe4 100644
--- a/fs/binfmt_elf.c
+++ b/fs/binfmt_elf.c
@@ -795,7 +795,10 @@ static int load_elf_binary(struct linux_binprm *bprm, struct pt_regs *regs)
 			 * might try to exec.  This is because the brk will
 			 * follow the loader, and is not movable.  */
 #if defined(CONFIG_X86) || defined(CONFIG_ARM)
-			load_bias = 0;
+			if (current->flags & PF_RANDOMIZE)
+				load_bias = 0;
+			else
+				load_bias = ELF_PAGESTART(ELF_ET_DYN_BASE - vaddr);
 #else
 			load_bias = ELF_PAGESTART(ELF_ET_DYN_BASE - vaddr);
 #endif

Re: [PATCH v2] binfmt_elf: Fix PIE execution with randomization disabled (was Re: [RFC PATCH] binfmt_elf: Fix PIE execution with randomization disabled)

[Josh Boyer]

On Mon, Oct 03, 2011 at 05:11:47PM +0200, Jiri Kosina wrote:
> > I was mostly just trying to adapt H.J.'s patch to account for the
> > PF_RANDOMIZE case.  Looking at it a bit more, I'm not sure why they
> > would need to be different.  H.J., do you recall why you made that
> > change originally?
> 
> How about the patch below instead? It survives my testing, and I believe 
> it handles both cases properly.
> 
> Confirmation from the original bug reporter would obviously be a nice 
> bonus too :)

I built an F15 kernel with this patch included.  The testcase included
in the original bug report seems to run with and without randomization
enabled.  Looking at the ldd output on the binary shows that both cases
are working appropriately as well.

I'm happy to add my Acked-by below, but it would be nice if H.J.
confirmed as well.

> From: Jiri Kosina <jkosina@suse.cz>
> Subject: [PATCH] binfmt_elf: fix PIE execution with randomization disabled
> 
> The case of address space randomization being disabled in runtime through
> randomize_va_space sysctl is not treated properly in load_elf_binary(),
> resulting in SIGKILL coming at exec() time for certain PIE-linked binaries
> in case the randomization has been disabled at runtime prior to calling
> exec().
> 
> Handle the randomize_va_space == 0 case the same way as if we were not
> supporting .text randomization at all.
> 
> Based on original patch by H.J. Lu <hongjiu.lu@intel.com> and
> Josh Boyer <jwboyer@redhat.com>
> 
> Cc: Ingo Molnar <mingo@elte.hu>
> Cc: Jiri Kosina <jkosina@suse.cz>
> Cc: Nicolas Pitre <nicolas.pitre@linaro.org>
> Cc: Russell King <rmk@arm.linux.org.uk>
> Signed-off-by: Jiri Kosina <jkosina@suse.cz>

Acked-by: Josh Boyer <jwboyer@redhat.com>

> ---
>  fs/binfmt_elf.c |    5 ++++-
>  1 files changed, 4 insertions(+), 1 deletions(-)
> 
> diff --git a/fs/binfmt_elf.c b/fs/binfmt_elf.c
> index dd0fdfc..bb11fe4 100644
> --- a/fs/binfmt_elf.c
> +++ b/fs/binfmt_elf.c
> @@ -795,7 +795,10 @@ static int load_elf_binary(struct linux_binprm *bprm, struct pt_regs *regs)
>  			 * might try to exec.  This is because the brk will
>  			 * follow the loader, and is not movable.  */
>  #if defined(CONFIG_X86) || defined(CONFIG_ARM)
> -			load_bias = 0;
> +			if (current->flags & PF_RANDOMIZE)
> +				load_bias = 0;
> +			else
> +				load_bias = ELF_PAGESTART(ELF_ET_DYN_BASE - vaddr);
>  #else
>  			load_bias = ELF_PAGESTART(ELF_ET_DYN_BASE - vaddr);
>  #endif
> 

Re: [PATCH v2] binfmt_elf: Fix PIE execution with randomization disabled (was Re: [RFC PATCH] binfmt_elf: Fix PIE execution with randomization disabled)

[Nicolas Pitre]

On Mon, 3 Oct 2011, Jiri Kosina wrote:

> How about the patch below instead? It survives my testing, and I believe 
> it handles both cases properly.
> 
> Confirmation from the original bug reporter would obviously be a nice 
> bonus too :)
> 
> 
> 
> 
> From: Jiri Kosina <jkosina@suse.cz>
> Subject: [PATCH] binfmt_elf: fix PIE execution with randomization disabled
> 
> The case of address space randomization being disabled in runtime through
> randomize_va_space sysctl is not treated properly in load_elf_binary(),
> resulting in SIGKILL coming at exec() time for certain PIE-linked binaries
> in case the randomization has been disabled at runtime prior to calling
> exec().
> 
> Handle the randomize_va_space == 0 case the same way as if we were not
> supporting .text randomization at all.
> 
> Based on original patch by H.J. Lu <hongjiu.lu@intel.com> and
> Josh Boyer <jwboyer@redhat.com>
> 
> Cc: Ingo Molnar <mingo@elte.hu>
> Cc: Jiri Kosina <jkosina@suse.cz>
> Cc: Nicolas Pitre <nicolas.pitre@linaro.org>
> Cc: Russell King <rmk@arm.linux.org.uk>
> Signed-off-by: Jiri Kosina <jkosina@suse.cz>

Acked-by: Nicolas Pitre <nicolas.pitre@linaro.org>

However I'd suggest a change to deal with style issues:

> diff --git a/fs/binfmt_elf.c b/fs/binfmt_elf.c
> index dd0fdfc..bb11fe4 100644
> --- a/fs/binfmt_elf.c
> +++ b/fs/binfmt_elf.c
> @@ -795,7 +795,10 @@ static int load_elf_binary(struct linux_binprm *bprm, struct pt_regs *regs)
>  			 * might try to exec.  This is because the brk will
>  			 * follow the loader, and is not movable.  */
>  #if defined(CONFIG_X86) || defined(CONFIG_ARM)
> -			load_bias = 0;
> +			if (current->flags & PF_RANDOMIZE)
> +				load_bias = 0;
> +			else
> +				load_bias = ELF_PAGESTART(ELF_ET_DYN_BASE - vaddr);
>  #else
>  			load_bias = ELF_PAGESTART(ELF_ET_DYN_BASE - vaddr);
>  #endif

What about this instead, saving 3 lines out of 8:

			load_bias = ELF_PAGESTART(ELF_ET_DYN_BASE - vaddr);
#if defined(CONFIG_X86) || defined(CONFIG_ARM)
			if (current->flags & PF_RANDOMIZE)
				load_bias = 0;
#endif


?


Nicolas

RE: [PATCH v2] binfmt_elf: Fix PIE execution with randomization disabled (was Re: [RFC PATCH] binfmt_elf: Fix PIE execution with randomization disabled)

[Lu, Hongjiu]

Why not

#if defined(CONFIG_X86) || defined(CONFIG_ARM)
#define CHECK_PF_RANDOMIZE 1
#else
#define CHECK_PF_RANDOMIZE 0
#endif
			if (CHECK_PF_RANDOMIZE && (current->flags & PF_RANDOMIZE))
				load_bias = 0;
			else
				load_bias = ELF_PAGESTART(ELF_ET_DYN_BASE - vaddr);

H.J.

> -----Original Message-----
> From: Nicolas Pitre [mailto:nicolas.pitre@linaro.org]
> Sent: Monday, October 03, 2011 8:56 AM
> To: Jiri Kosina
> Cc: Josh Boyer; Andrew Morton; Ingo Molnar; Lu, Hongjiu; linux-
> kernel@vger.kernel.org; Andrew Morton; Russell King
> Subject: Re: [PATCH v2] binfmt_elf: Fix PIE execution with randomization
> disabled (was Re: [RFC PATCH] binfmt_elf: Fix PIE execution with randomization
> disabled)
> 
> On Mon, 3 Oct 2011, Jiri Kosina wrote:
> 
> > How about the patch below instead? It survives my testing, and I believe
> > it handles both cases properly.
> >
> > Confirmation from the original bug reporter would obviously be a nice
> > bonus too :)
> >
> >
> >
> >
> > From: Jiri Kosina <jkosina@suse.cz>
> > Subject: [PATCH] binfmt_elf: fix PIE execution with randomization disabled
> >
> > The case of address space randomization being disabled in runtime through
> > randomize_va_space sysctl is not treated properly in load_elf_binary(),
> > resulting in SIGKILL coming at exec() time for certain PIE-linked binaries
> > in case the randomization has been disabled at runtime prior to calling
> > exec().
> >
> > Handle the randomize_va_space == 0 case the same way as if we were not
> > supporting .text randomization at all.
> >
> > Based on original patch by H.J. Lu <hongjiu.lu@intel.com> and
> > Josh Boyer <jwboyer@redhat.com>
> >
> > Cc: Ingo Molnar <mingo@elte.hu>
> > Cc: Jiri Kosina <jkosina@suse.cz>
> > Cc: Nicolas Pitre <nicolas.pitre@linaro.org>
> > Cc: Russell King <rmk@arm.linux.org.uk>
> > Signed-off-by: Jiri Kosina <jkosina@suse.cz>
> 
> Acked-by: Nicolas Pitre <nicolas.pitre@linaro.org>
> 
> However I'd suggest a change to deal with style issues:
> 
> > diff --git a/fs/binfmt_elf.c b/fs/binfmt_elf.c
> > index dd0fdfc..bb11fe4 100644
> > --- a/fs/binfmt_elf.c
> > +++ b/fs/binfmt_elf.c
> > @@ -795,7 +795,10 @@ static int load_elf_binary(struct linux_binprm *bprm,
> struct pt_regs *regs)
> >  			 * might try to exec.  This is because the brk will
> >  			 * follow the loader, and is not movable.  */
> >  #if defined(CONFIG_X86) || defined(CONFIG_ARM)
> > -			load_bias = 0;
> > +			if (current->flags & PF_RANDOMIZE)
> > +				load_bias = 0;
> > +			else
> > +				load_bias = ELF_PAGESTART(ELF_ET_DYN_BASE - vaddr);
> >  #else
> >  			load_bias = ELF_PAGESTART(ELF_ET_DYN_BASE - vaddr);
> >  #endif
> 
> What about this instead, saving 3 lines out of 8:
> 
> 			load_bias = ELF_PAGESTART(ELF_ET_DYN_BASE - vaddr);
> #if defined(CONFIG_X86) || defined(CONFIG_ARM)
> 			if (current->flags & PF_RANDOMIZE)
> 				load_bias = 0;
> #endif
> 
> 
> ?
> 
> 
> Nicolas

RE: [PATCH v2] binfmt_elf: Fix PIE execution with randomization disabled (was Re: [RFC PATCH] binfmt_elf: Fix PIE execution with randomization disabled)

[Nicolas Pitre]

On Mon, 3 Oct 2011, Lu, Hongjiu wrote:

> Why not
> 
> #if defined(CONFIG_X86) || defined(CONFIG_ARM)
> #define CHECK_PF_RANDOMIZE 1
> #else
> #define CHECK_PF_RANDOMIZE 0
> #endif
> 			if (CHECK_PF_RANDOMIZE && (current->flags & PF_RANDOMIZE))
> 				load_bias = 0;
> 			else
> 				load_bias = ELF_PAGESTART(ELF_ET_DYN_BASE - vaddr);
> 

Whatever.  I don't have a strong opinion on the shed color.


> > -----Original Message-----
> > From: Nicolas Pitre [mailto:nicolas.pitre@linaro.org]
> > Sent: Monday, October 03, 2011 8:56 AM
> > To: Jiri Kosina
> > Cc: Josh Boyer; Andrew Morton; Ingo Molnar; Lu, Hongjiu; linux-
> > kernel@vger.kernel.org; Andrew Morton; Russell King
> > Subject: Re: [PATCH v2] binfmt_elf: Fix PIE execution with randomization
> > disabled (was Re: [RFC PATCH] binfmt_elf: Fix PIE execution with randomization
> > disabled)
> > 
> > On Mon, 3 Oct 2011, Jiri Kosina wrote:
> > 
> > > How about the patch below instead? It survives my testing, and I believe
> > > it handles both cases properly.
> > >
> > > Confirmation from the original bug reporter would obviously be a nice
> > > bonus too :)
> > >
> > >
> > >
> > >
> > > From: Jiri Kosina <jkosina@suse.cz>
> > > Subject: [PATCH] binfmt_elf: fix PIE execution with randomization disabled
> > >
> > > The case of address space randomization being disabled in runtime through
> > > randomize_va_space sysctl is not treated properly in load_elf_binary(),
> > > resulting in SIGKILL coming at exec() time for certain PIE-linked binaries
> > > in case the randomization has been disabled at runtime prior to calling
> > > exec().
> > >
> > > Handle the randomize_va_space == 0 case the same way as if we were not
> > > supporting .text randomization at all.
> > >
> > > Based on original patch by H.J. Lu <hongjiu.lu@intel.com> and
> > > Josh Boyer <jwboyer@redhat.com>
> > >
> > > Cc: Ingo Molnar <mingo@elte.hu>
> > > Cc: Jiri Kosina <jkosina@suse.cz>
> > > Cc: Nicolas Pitre <nicolas.pitre@linaro.org>
> > > Cc: Russell King <rmk@arm.linux.org.uk>
> > > Signed-off-by: Jiri Kosina <jkosina@suse.cz>
> > 
> > Acked-by: Nicolas Pitre <nicolas.pitre@linaro.org>
> > 
> > However I'd suggest a change to deal with style issues:
> > 
> > > diff --git a/fs/binfmt_elf.c b/fs/binfmt_elf.c
> > > index dd0fdfc..bb11fe4 100644
> > > --- a/fs/binfmt_elf.c
> > > +++ b/fs/binfmt_elf.c
> > > @@ -795,7 +795,10 @@ static int load_elf_binary(struct linux_binprm *bprm,
> > struct pt_regs *regs)
> > >  			 * might try to exec.  This is because the brk will
> > >  			 * follow the loader, and is not movable.  */
> > >  #if defined(CONFIG_X86) || defined(CONFIG_ARM)
> > > -			load_bias = 0;
> > > +			if (current->flags & PF_RANDOMIZE)
> > > +				load_bias = 0;
> > > +			else
> > > +				load_bias = ELF_PAGESTART(ELF_ET_DYN_BASE - vaddr);
> > >  #else
> > >  			load_bias = ELF_PAGESTART(ELF_ET_DYN_BASE - vaddr);
> > >  #endif
> > 
> > What about this instead, saving 3 lines out of 8:
> > 
> > 			load_bias = ELF_PAGESTART(ELF_ET_DYN_BASE - vaddr);
> > #if defined(CONFIG_X86) || defined(CONFIG_ARM)
> > 			if (current->flags & PF_RANDOMIZE)
> > 				load_bias = 0;
> > #endif
> > 
> > 
> > ?
> > 
> > 
> > Nicolas
> --
> 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/
> 

RE: [PATCH v2] binfmt_elf: Fix PIE execution with randomization disabled (was Re: [RFC PATCH] binfmt_elf: Fix PIE execution with randomization disabled)

[Jiri Kosina]

On Mon, 3 Oct 2011, Lu, Hongjiu wrote:

> Why not
> 
> #if defined(CONFIG_X86) || defined(CONFIG_ARM)
> #define CHECK_PF_RANDOMIZE 1
> #else
> #define CHECK_PF_RANDOMIZE 0
> #endif
> 			if (CHECK_PF_RANDOMIZE && (current->flags & PF_RANDOMIZE))
> 				load_bias = 0;
> 			else
> 				load_bias = ELF_PAGESTART(ELF_ET_DYN_BASE - vaddr);

If anything, that is not particularly good name for that macro (as we are 
checking current->flags for PF_RANDOMIZE at other places indepenedntly on 
this).

Personally, I find my original patch the most readable one, but I don't 
care much, it's really bike-shedding.

-- 
Jiri Kosina
SUSE Labs

Re: [PATCH v2] binfmt_elf: Fix PIE execution with randomization disabled (was Re: [RFC PATCH] binfmt_elf: Fix PIE execution with randomization disabled)

[Andrew Morton]

On Mon, 3 Oct 2011 17:11:47 +0200 (CEST)
Jiri Kosina <jkosina@suse.cz> wrote:

> 
> From: Jiri Kosina <jkosina@suse.cz>
> Subject: [PATCH] binfmt_elf: fix PIE execution with randomization disabled
> 
> The case of address space randomization being disabled in runtime through
> randomize_va_space sysctl is not treated properly in load_elf_binary(),
> resulting in SIGKILL coming at exec() time for certain PIE-linked binaries
> in case the randomization has been disabled at runtime prior to calling
> exec().
> 
> Handle the randomize_va_space == 0 case the same way as if we were not
> supporting .text randomization at all.
> 
> Based on original patch by H.J. Lu <hongjiu.lu@intel.com> and
> Josh Boyer <jwboyer@redhat.com>
> 
> Cc: Ingo Molnar <mingo@elte.hu>
> Cc: Jiri Kosina <jkosina@suse.cz>
> Cc: Nicolas Pitre <nicolas.pitre@linaro.org>
> Cc: Russell King <rmk@arm.linux.org.uk>
> Signed-off-by: Jiri Kosina <jkosina@suse.cz>
> ---
>  fs/binfmt_elf.c |    5 ++++-
>  1 files changed, 4 insertions(+), 1 deletions(-)
> 
> diff --git a/fs/binfmt_elf.c b/fs/binfmt_elf.c
> index dd0fdfc..bb11fe4 100644
> --- a/fs/binfmt_elf.c
> +++ b/fs/binfmt_elf.c
> @@ -795,7 +795,10 @@ static int load_elf_binary(struct linux_binprm *bprm, struct pt_regs *regs)
>  			 * might try to exec.  This is because the brk will
>  			 * follow the loader, and is not movable.  */
>  #if defined(CONFIG_X86) || defined(CONFIG_ARM)
> -			load_bias = 0;
> +			if (current->flags & PF_RANDOMIZE)
> +				load_bias = 0;
> +			else
> +				load_bias = ELF_PAGESTART(ELF_ET_DYN_BASE - vaddr);
>  #else
>  			load_bias = ELF_PAGESTART(ELF_ET_DYN_BASE - vaddr);
>  #endif

Guys, it took several people several days and 10+ emails to work out
what's happening in there, and the first attempt to fix it was buggy. 
This is all a huuuuge signal that the code is unobvious, hard to
understand, hard to maintain.

Please, let's get a good code comment in there while it's fresh in your
minds.  So the next person who comes along doesn't have the same amount
of difficulty?

Re: [PATCH v2] binfmt_elf: Fix PIE execution with randomization disabled (was Re: [RFC PATCH] binfmt_elf: Fix PIE execution with randomization disabled)

[Jiri Kosina]

On Mon, 3 Oct 2011, Andrew Morton wrote:

> > From: Jiri Kosina <jkosina@suse.cz>
> > Subject: [PATCH] binfmt_elf: fix PIE execution with randomization disabled
> > 
> > The case of address space randomization being disabled in runtime through
> > randomize_va_space sysctl is not treated properly in load_elf_binary(),
> > resulting in SIGKILL coming at exec() time for certain PIE-linked binaries
> > in case the randomization has been disabled at runtime prior to calling
> > exec().
> > 
> > Handle the randomize_va_space == 0 case the same way as if we were not
> > supporting .text randomization at all.
> > 
> > Based on original patch by H.J. Lu <hongjiu.lu@intel.com> and
> > Josh Boyer <jwboyer@redhat.com>
> > 
> > Cc: Ingo Molnar <mingo@elte.hu>
> > Cc: Jiri Kosina <jkosina@suse.cz>
> > Cc: Nicolas Pitre <nicolas.pitre@linaro.org>
> > Cc: Russell King <rmk@arm.linux.org.uk>
> > Signed-off-by: Jiri Kosina <jkosina@suse.cz>
> > ---
> >  fs/binfmt_elf.c |    5 ++++-
> >  1 files changed, 4 insertions(+), 1 deletions(-)
> > 
> > diff --git a/fs/binfmt_elf.c b/fs/binfmt_elf.c
> > index dd0fdfc..bb11fe4 100644
> > --- a/fs/binfmt_elf.c
> > +++ b/fs/binfmt_elf.c
> > @@ -795,7 +795,10 @@ static int load_elf_binary(struct linux_binprm *bprm, struct pt_regs *regs)
> >  			 * might try to exec.  This is because the brk will
> >  			 * follow the loader, and is not movable.  */
> >  #if defined(CONFIG_X86) || defined(CONFIG_ARM)
> > -			load_bias = 0;
> > +			if (current->flags & PF_RANDOMIZE)
> > +				load_bias = 0;
> > +			else
> > +				load_bias = ELF_PAGESTART(ELF_ET_DYN_BASE - vaddr);
> >  #else
> >  			load_bias = ELF_PAGESTART(ELF_ET_DYN_BASE - vaddr);
> >  #endif
> 
> Guys, it took several people several days and 10+ emails to work out
> what's happening in there, and the first attempt to fix it was buggy. 
> This is all a huuuuge signal that the code is unobvious, hard to
> understand, hard to maintain.

It's unfortunately true. Rewriting the ELF binary loader is on my list of 
very-long-term TODOs. It has became quite a monster over decades.

> Please, let's get a good code comment in there while it's fresh in your 
> minds.  So the next person who comes along doesn't have the same amount 
> of difficulty?

I have to run now, I will resend the patch to you with an explanatory 
comment added in about 8 hours from now.

Thanks,

-- 
Jiri Kosina
SUSE Labs

Re: [PATCH v3] binfmt_elf: Fix PIE execution with randomization disabled (was Re: [RFC PATCH] binfmt_elf: Fix PIE execution with randomization disabled)

[Jiri Kosina]

On Tue, 4 Oct 2011, Jiri Kosina wrote:

> > Guys, it took several people several days and 10+ emails to work out
> > what's happening in there, and the first attempt to fix it was buggy. 
> > This is all a huuuuge signal that the code is unobvious, hard to
> > understand, hard to maintain.
> 
> It's unfortunately true. Rewriting the ELF binary loader is on my list of 
> very-long-term TODOs. It has became quite a monster over decades.
> 
> > Please, let's get a good code comment in there while it's fresh in your 
> > minds.  So the next person who comes along doesn't have the same amount 
> > of difficulty?

Andrew,

find the updated patch below. Please consider applying.

I believe Cc to stable is appropriate as well.

Thanks a lot Josh for verification of the fix.



From: Jiri Kosina <jkosina@suse.cz>
Subject: [PATCH] binfmt_elf: fix PIE execution with randomization disabled

The case of address space randomization being disabled in runtime through
randomize_va_space sysctl is not treated properly in load_elf_binary(),
resulting in SIGKILL coming at exec() time for certain PIE-linked binaries
in case the randomization has been disabled at runtime prior to calling
exec().

Handle the randomize_va_space == 0 case the same way as if we were not
supporting .text randomization at all.

Based on original patch by H.J. Lu <hongjiu.lu@intel.com> and.
Josh Boyer <jwboyer@redhat.com>

Cc: Ingo Molnar <mingo@elte.hu>
Cc: Russell King <rmk@arm.linux.org.uk>
Cc: stable@kernel.org
Tested-by: Josh Boyer <jwboyer@redhat.com>
Acked-by: Nicolas Pitre <nicolas.pitre@linaro.org>
Signed-off-by: Jiri Kosina <jkosina@suse.cz>
---
 fs/binfmt_elf.c |   11 ++++++++++-
 1 files changed, 10 insertions(+), 1 deletions(-)

diff --git a/fs/binfmt_elf.c b/fs/binfmt_elf.c
index dd0fdfc..21ac5ee 100644
--- a/fs/binfmt_elf.c
+++ b/fs/binfmt_elf.c
@@ -795,7 +795,16 @@ static int load_elf_binary(struct linux_binprm *bprm, struct pt_regs *regs)
 			 * might try to exec.  This is because the brk will
 			 * follow the loader, and is not movable.  */
 #if defined(CONFIG_X86) || defined(CONFIG_ARM)
-			load_bias = 0;
+			/* Memory randomization might have been switched off
+			 * in runtime via sysctl.
+			 * If that is the case, retain the original non-zero
+			 * load_bias value in order to establish proper
+			 * non-randomized mappings.
+			 */
+			if (current->flags & PF_RANDOMIZE)
+				load_bias = 0;
+			else
+				load_bias = ELF_PAGESTART(ELF_ET_DYN_BASE - vaddr);
 #else
 			load_bias = ELF_PAGESTART(ELF_ET_DYN_BASE - vaddr);
 #endif

-- 
Jiri Kosina
SUSE Labs

Re: [PATCH v2] binfmt_elf: Fix PIE execution with randomization disabled (was Re: [RFC PATCH] binfmt_elf: Fix PIE execution with randomization disabled)

[Jiri Kosina]

On Mon, 3 Oct 2011, Andrew Morton wrote:

> > Signed-off-by: Jiri Kosina <jkosina@suse.cz>
> > ---
> >  fs/binfmt_elf.c |    5 ++++-
> >  1 files changed, 4 insertions(+), 1 deletions(-)
> > 
> > diff --git a/fs/binfmt_elf.c b/fs/binfmt_elf.c
> > index dd0fdfc..bb11fe4 100644
> > --- a/fs/binfmt_elf.c
> > +++ b/fs/binfmt_elf.c
> > @@ -795,7 +795,10 @@ static int load_elf_binary(struct linux_binprm *bprm, struct pt_regs *regs)
> >  			 * might try to exec.  This is because the brk will
> >  			 * follow the loader, and is not movable.  */
> >  #if defined(CONFIG_X86) || defined(CONFIG_ARM)
> > -			load_bias = 0;
> > +			if (current->flags & PF_RANDOMIZE)
> > +				load_bias = 0;
> > +			else
> > +				load_bias = ELF_PAGESTART(ELF_ET_DYN_BASE - vaddr);
> >  #else
> >  			load_bias = ELF_PAGESTART(ELF_ET_DYN_BASE - vaddr);
> >  #endif
> 
> Guys, it took several people several days and 10+ emails to work out
> what's happening in there, and the first attempt to fix it was buggy. 
> This is all a huuuuge signal that the code is unobvious, hard to
> understand, hard to maintain.
> 
> Please, let's get a good code comment in there while it's fresh in your
> minds.  So the next person who comes along doesn't have the same amount
> of difficulty?

BTW, after thinking about this a little bit more, maybe the patch below 
would be worthwile and could save some mis-routed e-mails and bugreports 
in the future ...


From: Jiri Kosina <jkosina@suse.cz>
Subject: [PATCH] MAINTAINERS: add ASLR maintainer

Since achieving the full ASLR by merging the PIE randomization in commit 
cc503c1b43 ("x86: PIE executable randomization"), I have been dealing with 
most (if not all) of the bugreports reported against userspace address 
space randomization, so it might be a good idea to provide a decent 
contact point in MAINTAINERS.

Signed-off-by: Jiri Kosina <jkosina@suse.cz>
---
 MAINTAINERS |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)

diff --git a/MAINTAINERS b/MAINTAINERS
index ace8f9c..c362845 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -317,6 +317,10 @@ W:	http://wiki.analog.com/AD7879
 S:	Supported
 F:	drivers/input/touchscreen/ad7879.c
 
+ADDRESS SPACE LAYOUT RANDOMIZATION (ASLR)
+M:	Jiri Kosina <jkosina@suse.cz>
+S:	Maintained
+
 ADM1025 HARDWARE MONITOR DRIVER
 M:	Jean Delvare <khali@linux-fr.org>
 L:	lm-sensors@lm-sensors.org
-- 
1.7.3.1