* [RFC PATCH] binfmt_elf: Fix PIE execution with randomization disabled @ 2011-09-29 19:53 Josh Boyer 2011-09-29 21:19 ` Andrew Morton 2011-10-03 12:10 ` [RFC PATCH] binfmt_elf: Fix PIE execution with randomization disabled Jiri Kosina 0 siblings, 2 replies; 19+ messages in thread From: Josh Boyer @ 2011-09-29 19:53 UTC (permalink / raw) To: Ingo Molnar, akpm, Jiri Kosina; +Cc: hongjiu.lu, linux-kernel 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 ^ permalink raw reply related [flat|nested] 19+ messages in thread
* Re: [RFC PATCH] binfmt_elf: Fix PIE execution with randomization disabled 2011-09-29 19:53 [RFC PATCH] binfmt_elf: Fix PIE execution with randomization disabled Josh Boyer @ 2011-09-29 21:19 ` Andrew Morton 2011-09-29 21:36 ` Lu, Hongjiu 2011-09-30 0:41 ` Nicolas Pitre 2011-10-03 12:10 ` [RFC PATCH] binfmt_elf: Fix PIE execution with randomization disabled Jiri Kosina 1 sibling, 2 replies; 19+ messages in thread From: Andrew Morton @ 2011-09-29 21:19 UTC (permalink / raw) To: Josh Boyer Cc: Ingo Molnar, Jiri Kosina, hongjiu.lu, linux-kernel, Nicolas Pitre, Nicolas Pitre, Andrew Morton, Russell King 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 ^ permalink raw reply [flat|nested] 19+ messages in thread
* RE: [RFC PATCH] binfmt_elf: Fix PIE execution with randomization disabled 2011-09-29 21:19 ` Andrew Morton @ 2011-09-29 21:36 ` Lu, Hongjiu 2011-09-30 0:41 ` Nicolas Pitre 1 sibling, 0 replies; 19+ messages in thread From: Lu, Hongjiu @ 2011-09-29 21:36 UTC (permalink / raw) To: Andrew Morton, Josh Boyer Cc: Ingo Molnar, Jiri Kosina, linux-kernel, Nicolas Pitre, Nicolas Pitre, Andrew Morton, Russell King > 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 ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [RFC PATCH] binfmt_elf: Fix PIE execution with randomization disabled 2011-09-29 21:19 ` Andrew Morton 2011-09-29 21:36 ` Lu, Hongjiu @ 2011-09-30 0:41 ` Nicolas Pitre 2011-09-30 2:16 ` Josh Boyer 1 sibling, 1 reply; 19+ messages in thread From: Nicolas Pitre @ 2011-09-30 0:41 UTC (permalink / raw) To: Andrew Morton Cc: Josh Boyer, Ingo Molnar, Jiri Kosina, hongjiu.lu, linux-kernel, Andrew Morton, Russell King 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 ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [RFC PATCH] binfmt_elf: Fix PIE execution with randomization disabled 2011-09-30 0:41 ` Nicolas Pitre @ 2011-09-30 2:16 ` Josh Boyer 2011-10-03 14:53 ` Jiri Kosina 0 siblings, 1 reply; 19+ messages in thread From: Josh Boyer @ 2011-09-30 2:16 UTC (permalink / raw) To: Nicolas Pitre Cc: Andrew Morton, Ingo Molnar, Jiri Kosina, hongjiu.lu, linux-kernel, Andrew Morton, Russell King 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 ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [RFC PATCH] binfmt_elf: Fix PIE execution with randomization disabled 2011-09-30 2:16 ` Josh Boyer @ 2011-10-03 14:53 ` Jiri Kosina 2011-10-03 15:03 ` Josh Boyer 0 siblings, 1 reply; 19+ messages in thread From: Jiri Kosina @ 2011-10-03 14:53 UTC (permalink / raw) To: Josh Boyer Cc: Nicolas Pitre, Andrew Morton, Ingo Molnar, hongjiu.lu, linux-kernel, Andrew Morton, Russell King 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 ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [RFC PATCH] binfmt_elf: Fix PIE execution with randomization disabled 2011-10-03 14:53 ` Jiri Kosina @ 2011-10-03 15:03 ` Josh Boyer 2011-10-03 15:11 ` [PATCH v2] binfmt_elf: Fix PIE execution with randomization disabled (was Re: [RFC PATCH] binfmt_elf: Fix PIE execution with randomization disabled) Jiri Kosina 0 siblings, 1 reply; 19+ messages in thread From: Josh Boyer @ 2011-10-03 15:03 UTC (permalink / raw) To: Jiri Kosina Cc: Nicolas Pitre, Andrew Morton, Ingo Molnar, hongjiu.lu, linux-kernel, Andrew Morton, Russell King 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 ^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH v2] binfmt_elf: Fix PIE execution with randomization disabled (was Re: [RFC PATCH] binfmt_elf: Fix PIE execution with randomization disabled) 2011-10-03 15:03 ` Josh Boyer @ 2011-10-03 15:11 ` Jiri Kosina 2011-10-03 15:42 ` Josh Boyer ` (2 more replies) 0 siblings, 3 replies; 19+ messages in thread From: Jiri Kosina @ 2011-10-03 15:11 UTC (permalink / raw) To: Josh Boyer Cc: Nicolas Pitre, Andrew Morton, Ingo Molnar, hongjiu.lu, linux-kernel, Andrew Morton, Russell King 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 ^ permalink raw reply related [flat|nested] 19+ messages in thread
* Re: [PATCH v2] binfmt_elf: Fix PIE execution with randomization disabled (was Re: [RFC PATCH] binfmt_elf: Fix PIE execution with randomization disabled) 2011-10-03 15:11 ` [PATCH v2] binfmt_elf: Fix PIE execution with randomization disabled (was Re: [RFC PATCH] binfmt_elf: Fix PIE execution with randomization disabled) Jiri Kosina @ 2011-10-03 15:42 ` Josh Boyer 2011-10-03 15:56 ` Nicolas Pitre 2011-10-03 22:03 ` Andrew Morton 2 siblings, 0 replies; 19+ messages in thread From: Josh Boyer @ 2011-10-03 15:42 UTC (permalink / raw) To: Jiri Kosina Cc: Nicolas Pitre, Andrew Morton, Ingo Molnar, hongjiu.lu, linux-kernel, Andrew Morton, Russell King 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 > ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v2] binfmt_elf: Fix PIE execution with randomization disabled (was Re: [RFC PATCH] binfmt_elf: Fix PIE execution with randomization disabled) 2011-10-03 15:11 ` [PATCH v2] binfmt_elf: Fix PIE execution with randomization disabled (was Re: [RFC PATCH] binfmt_elf: Fix PIE execution with randomization disabled) Jiri Kosina 2011-10-03 15:42 ` Josh Boyer @ 2011-10-03 15:56 ` Nicolas Pitre 2011-10-03 16:02 ` Lu, Hongjiu 2011-10-03 22:03 ` Andrew Morton 2 siblings, 1 reply; 19+ messages in thread From: Nicolas Pitre @ 2011-10-03 15:56 UTC (permalink / raw) To: Jiri Kosina Cc: Josh Boyer, Andrew Morton, Ingo Molnar, hongjiu.lu, linux-kernel, Andrew Morton, Russell King 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 ^ permalink raw reply [flat|nested] 19+ messages in thread
* RE: [PATCH v2] binfmt_elf: Fix PIE execution with randomization disabled (was Re: [RFC PATCH] binfmt_elf: Fix PIE execution with randomization disabled) 2011-10-03 15:56 ` Nicolas Pitre @ 2011-10-03 16:02 ` Lu, Hongjiu 2011-10-03 16:13 ` Nicolas Pitre 2011-10-03 21:14 ` Jiri Kosina 0 siblings, 2 replies; 19+ messages in thread From: Lu, Hongjiu @ 2011-10-03 16:02 UTC (permalink / raw) To: Nicolas Pitre, Jiri Kosina Cc: Josh Boyer, Andrew Morton, Ingo Molnar, linux-kernel, Andrew Morton, Russell King 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 ^ permalink raw reply [flat|nested] 19+ messages in thread
* RE: [PATCH v2] binfmt_elf: Fix PIE execution with randomization disabled (was Re: [RFC PATCH] binfmt_elf: Fix PIE execution with randomization disabled) 2011-10-03 16:02 ` Lu, Hongjiu @ 2011-10-03 16:13 ` Nicolas Pitre 2011-10-03 21:14 ` Jiri Kosina 1 sibling, 0 replies; 19+ messages in thread From: Nicolas Pitre @ 2011-10-03 16:13 UTC (permalink / raw) To: Lu, Hongjiu Cc: Jiri Kosina, Josh Boyer, Andrew Morton, Ingo Molnar, linux-kernel, Andrew Morton, Russell King 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/ > ^ permalink raw reply [flat|nested] 19+ messages in thread
* RE: [PATCH v2] binfmt_elf: Fix PIE execution with randomization disabled (was Re: [RFC PATCH] binfmt_elf: Fix PIE execution with randomization disabled) 2011-10-03 16:02 ` Lu, Hongjiu 2011-10-03 16:13 ` Nicolas Pitre @ 2011-10-03 21:14 ` Jiri Kosina 1 sibling, 0 replies; 19+ messages in thread From: Jiri Kosina @ 2011-10-03 21:14 UTC (permalink / raw) To: Lu, Hongjiu Cc: Nicolas Pitre, Josh Boyer, Andrew Morton, Ingo Molnar, linux-kernel, Andrew Morton, Russell King 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 ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v2] binfmt_elf: Fix PIE execution with randomization disabled (was Re: [RFC PATCH] binfmt_elf: Fix PIE execution with randomization disabled) 2011-10-03 15:11 ` [PATCH v2] binfmt_elf: Fix PIE execution with randomization disabled (was Re: [RFC PATCH] binfmt_elf: Fix PIE execution with randomization disabled) Jiri Kosina 2011-10-03 15:42 ` Josh Boyer 2011-10-03 15:56 ` Nicolas Pitre @ 2011-10-03 22:03 ` Andrew Morton 2011-10-03 22:06 ` Jiri Kosina 2011-10-08 22:35 ` [PATCH v2] " Jiri Kosina 2 siblings, 2 replies; 19+ messages in thread From: Andrew Morton @ 2011-10-03 22:03 UTC (permalink / raw) To: Jiri Kosina Cc: Josh Boyer, Nicolas Pitre, Andrew Morton, Ingo Molnar, hongjiu.lu, linux-kernel, Russell King 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? ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v2] binfmt_elf: Fix PIE execution with randomization disabled (was Re: [RFC PATCH] binfmt_elf: Fix PIE execution with randomization disabled) 2011-10-03 22:03 ` Andrew Morton @ 2011-10-03 22:06 ` Jiri Kosina 2011-10-03 22:56 ` [PATCH v3] " Jiri Kosina 2011-10-08 22:35 ` [PATCH v2] " Jiri Kosina 1 sibling, 1 reply; 19+ messages in thread From: Jiri Kosina @ 2011-10-03 22:06 UTC (permalink / raw) To: Andrew Morton Cc: Josh Boyer, Nicolas Pitre, Ingo Molnar, hongjiu.lu, linux-kernel, Russell King 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 ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v3] binfmt_elf: Fix PIE execution with randomization disabled (was Re: [RFC PATCH] binfmt_elf: Fix PIE execution with randomization disabled) 2011-10-03 22:06 ` Jiri Kosina @ 2011-10-03 22:56 ` Jiri Kosina 0 siblings, 0 replies; 19+ messages in thread From: Jiri Kosina @ 2011-10-03 22:56 UTC (permalink / raw) To: Andrew Morton Cc: Josh Boyer, Nicolas Pitre, Ingo Molnar, hongjiu.lu, linux-kernel, Russell King 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 ^ permalink raw reply related [flat|nested] 19+ messages in thread
* Re: [PATCH v2] binfmt_elf: Fix PIE execution with randomization disabled (was Re: [RFC PATCH] binfmt_elf: Fix PIE execution with randomization disabled) 2011-10-03 22:03 ` Andrew Morton 2011-10-03 22:06 ` Jiri Kosina @ 2011-10-08 22:35 ` Jiri Kosina 1 sibling, 0 replies; 19+ messages in thread From: Jiri Kosina @ 2011-10-08 22:35 UTC (permalink / raw) To: Andrew Morton Cc: Josh Boyer, Nicolas Pitre, Ingo Molnar, hongjiu.lu, linux-kernel, Russell King 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 ^ permalink raw reply related [flat|nested] 19+ messages in thread
* Re: [RFC PATCH] binfmt_elf: Fix PIE execution with randomization disabled 2011-09-29 19:53 [RFC PATCH] binfmt_elf: Fix PIE execution with randomization disabled Josh Boyer 2011-09-29 21:19 ` Andrew Morton @ 2011-10-03 12:10 ` Jiri Kosina 2011-10-03 12:59 ` Josh Boyer 1 sibling, 1 reply; 19+ messages in thread From: Jiri Kosina @ 2011-10-03 12:10 UTC (permalink / raw) To: Josh Boyer; +Cc: Ingo Molnar, akpm, hongjiu.lu, linux-kernel 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 ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [RFC PATCH] binfmt_elf: Fix PIE execution with randomization disabled 2011-10-03 12:10 ` [RFC PATCH] binfmt_elf: Fix PIE execution with randomization disabled Jiri Kosina @ 2011-10-03 12:59 ` Josh Boyer 0 siblings, 0 replies; 19+ messages in thread From: Josh Boyer @ 2011-10-03 12:59 UTC (permalink / raw) To: Jiri Kosina; +Cc: Ingo Molnar, akpm, hongjiu.lu, linux-kernel 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 ^ permalink raw reply [flat|nested] 19+ messages in thread
end of thread, other threads:[~2011-10-08 22:35 UTC | newest] Thread overview: 19+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2011-09-29 19:53 [RFC PATCH] binfmt_elf: Fix PIE execution with randomization disabled Josh Boyer 2011-09-29 21:19 ` Andrew Morton 2011-09-29 21:36 ` Lu, Hongjiu 2011-09-30 0:41 ` Nicolas Pitre 2011-09-30 2:16 ` Josh Boyer 2011-10-03 14:53 ` Jiri Kosina 2011-10-03 15:03 ` Josh Boyer 2011-10-03 15:11 ` [PATCH v2] binfmt_elf: Fix PIE execution with randomization disabled (was Re: [RFC PATCH] binfmt_elf: Fix PIE execution with randomization disabled) Jiri Kosina 2011-10-03 15:42 ` Josh Boyer 2011-10-03 15:56 ` Nicolas Pitre 2011-10-03 16:02 ` Lu, Hongjiu 2011-10-03 16:13 ` Nicolas Pitre 2011-10-03 21:14 ` Jiri Kosina 2011-10-03 22:03 ` Andrew Morton 2011-10-03 22:06 ` Jiri Kosina 2011-10-03 22:56 ` [PATCH v3] " Jiri Kosina 2011-10-08 22:35 ` [PATCH v2] " Jiri Kosina 2011-10-03 12:10 ` [RFC PATCH] binfmt_elf: Fix PIE execution with randomization disabled Jiri Kosina 2011-10-03 12:59 ` Josh Boyer
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.