In fs/binfmt_elf.c, routine load_elf_interp() calls padzero() for .bss
even if the PT_LOAD has no PROT_WRITE and no .bss. This generates EFAULT.
One easy way to avoid trouble is that a PT_LOAD with no PROT_WRITE
should skip the .bss calculation entirely.
Here is a small test case. (Yes, there are other, useful PT_INTERP
which have only .text and no .data/.bss.)
----- ptinterp.S
_start: .globl _start
nop
int3
-----
$ gcc -m32 -nostartfiles -nostdlib -o ptinterp ptinterp.S
$ gcc -m32 -Wl,--dynamic-linker=ptinterp -o hello hello.c
$ ./hello
Segmentation fault # during execve() itself
After applying the patch:
$ ./hello
Trace trap # user-mode execution after execve() finishes
Signed-off-by: John Reiser <[email protected]>
diff --git a/fs/binfmt_elf.c b/fs/binfmt_elf.c
index b7c1603..3b9a097 100644
--- a/fs/binfmt_elf.c
+++ b/fs/binfmt_elf.c
@@ -488,7 +488,7 @@ static unsigned long load_elf_interp(struct elfhdr *interp_elf_ex,
* keep track of the largest address we see for this.
*/
k = load_addr + eppnt->p_vaddr + eppnt->p_filesz;
- if (k > elf_bss)
+ if (k > elf_bss && PROT_WRITE & elf_prot)
elf_bss = k;
/*
@@ -496,7 +496,7 @@ static unsigned long load_elf_interp(struct elfhdr *interp_elf_ex,
* elf_bss and last_bss is the bss section.
*/
k = load_addr + eppnt->p_memsz + eppnt->p_vaddr;
- if (k > last_bss)
+ if (k > last_bss && PROT_WRITE & elf_prot)
last_bss = k;
}
}
--
Good catch, John. But I prefer a different fix.
I've reproduced the bug with the test case John included,
and verified that my change fixes it too.
Thanks,
Roland
---
[PATCH] binfmt_elf: fix PT_INTERP bss handling
In fs/binfmt_elf.c, load_elf_interp() calls padzero() for .bss even if
the PT_LOAD has no PROT_WRITE and no .bss. This generates EFAULT.
Here is a small test case. (Yes, there are other, useful PT_INTERP
which have only .text and no .data/.bss.)
----- ptinterp.S
_start: .globl _start
nop
int3
-----
$ gcc -m32 -nostartfiles -nostdlib -o ptinterp ptinterp.S
$ gcc -m32 -Wl,--dynamic-linker=ptinterp -o hello hello.c
$ ./hello
Segmentation fault # during execve() itself
After applying the patch:
$ ./hello
Trace trap # user-mode execution after execve() finishes
If the ELF headers are actually self-inconsistent, then dying is fine.
But having no PROT_WRITE segment is perfectly normal and correct if
there is no segment with p_memsz > p_filesz (i.e. bss). John Reiser
suggested checking for PROT_WRITE in the bss logic. I think it makes
most sense to simply apply the bss logic only when there is bss.
This patch looks less trivial than it is due to some reindentation.
It just moves the "if (last_bss > elf_bss) {" test up to include the
partial-page bss logic as well as the more-pages bss logic.
Reported-by: John Reiser <[email protected]>
Signed-off-by: Roland McGrath <[email protected]>
---
fs/binfmt_elf.c | 28 ++++++++++++++--------------
1 files changed, 14 insertions(+), 14 deletions(-)
diff --git a/fs/binfmt_elf.c b/fs/binfmt_elf.c
index b7c1603..7c1e65d 100644
--- a/fs/binfmt_elf.c
+++ b/fs/binfmt_elf.c
@@ -501,22 +501,22 @@ static unsigned long load_elf_interp(struct elfhdr *interp_elf_ex,
}
}
- /*
- * Now fill out the bss section. First pad the last page up
- * to the page boundary, and then perform a mmap to make sure
- * that there are zero-mapped pages up to and including the
- * last bss page.
- */
- if (padzero(elf_bss)) {
- error = -EFAULT;
- goto out_close;
- }
+ if (last_bss > elf_bss) {
+ /*
+ * Now fill out the bss section. First pad the last page up
+ * to the page boundary, and then perform a mmap to make sure
+ * that there are zero-mapped pages up to and including the
+ * last bss page.
+ */
+ if (padzero(elf_bss)) {
+ error = -EFAULT;
+ goto out_close;
+ }
- /* What we have mapped so far */
- elf_bss = ELF_PAGESTART(elf_bss + ELF_MIN_ALIGN - 1);
+ /* What we have mapped so far */
+ elf_bss = ELF_PAGESTART(elf_bss + ELF_MIN_ALIGN - 1);
- /* Map the last of the bss segment */
- if (last_bss > elf_bss) {
+ /* Map the last of the bss segment */
down_write(¤t->mm->mmap_sem);
error = do_brk(elf_bss, last_bss - elf_bss);
up_write(¤t->mm->mmap_sem);
On Tue, 8 Sep 2009, Roland McGrath wrote:
> Good catch, John. But I prefer a different fix.
>
> I've reproduced the bug with the test case John included,
> and verified that my change fixes it too.
Applied to
git://git.kernel.org/pub/scm/linux/kernel/git/jmorris/security-testing-2.6#next
(cc stable).
>
>
> Thanks,
> Roland
>
> ---
> [PATCH] binfmt_elf: fix PT_INTERP bss handling
>
> In fs/binfmt_elf.c, load_elf_interp() calls padzero() for .bss even if
> the PT_LOAD has no PROT_WRITE and no .bss. This generates EFAULT.
>
> Here is a small test case. (Yes, there are other, useful PT_INTERP
> which have only .text and no .data/.bss.)
>
> ----- ptinterp.S
> _start: .globl _start
> nop
> int3
> -----
> $ gcc -m32 -nostartfiles -nostdlib -o ptinterp ptinterp.S
> $ gcc -m32 -Wl,--dynamic-linker=ptinterp -o hello hello.c
> $ ./hello
> Segmentation fault # during execve() itself
>
> After applying the patch:
> $ ./hello
> Trace trap # user-mode execution after execve() finishes
>
> If the ELF headers are actually self-inconsistent, then dying is fine.
> But having no PROT_WRITE segment is perfectly normal and correct if
> there is no segment with p_memsz > p_filesz (i.e. bss). John Reiser
> suggested checking for PROT_WRITE in the bss logic. I think it makes
> most sense to simply apply the bss logic only when there is bss.
>
> This patch looks less trivial than it is due to some reindentation.
> It just moves the "if (last_bss > elf_bss) {" test up to include the
> partial-page bss logic as well as the more-pages bss logic.
>
> Reported-by: John Reiser <[email protected]>
> Signed-off-by: Roland McGrath <[email protected]>
> ---
> fs/binfmt_elf.c | 28 ++++++++++++++--------------
> 1 files changed, 14 insertions(+), 14 deletions(-)
>
> diff --git a/fs/binfmt_elf.c b/fs/binfmt_elf.c
> index b7c1603..7c1e65d 100644
> --- a/fs/binfmt_elf.c
> +++ b/fs/binfmt_elf.c
> @@ -501,22 +501,22 @@ static unsigned long load_elf_interp(struct elfhdr *interp_elf_ex,
> }
> }
>
> - /*
> - * Now fill out the bss section. First pad the last page up
> - * to the page boundary, and then perform a mmap to make sure
> - * that there are zero-mapped pages up to and including the
> - * last bss page.
> - */
> - if (padzero(elf_bss)) {
> - error = -EFAULT;
> - goto out_close;
> - }
> + if (last_bss > elf_bss) {
> + /*
> + * Now fill out the bss section. First pad the last page up
> + * to the page boundary, and then perform a mmap to make sure
> + * that there are zero-mapped pages up to and including the
> + * last bss page.
> + */
> + if (padzero(elf_bss)) {
> + error = -EFAULT;
> + goto out_close;
> + }
>
> - /* What we have mapped so far */
> - elf_bss = ELF_PAGESTART(elf_bss + ELF_MIN_ALIGN - 1);
> + /* What we have mapped so far */
> + elf_bss = ELF_PAGESTART(elf_bss + ELF_MIN_ALIGN - 1);
>
> - /* Map the last of the bss segment */
> - if (last_bss > elf_bss) {
> + /* Map the last of the bss segment */
> down_write(¤t->mm->mmap_sem);
> error = do_brk(elf_bss, last_bss - elf_bss);
> up_write(¤t->mm->mmap_sem);
>
--
James Morris
<[email protected]>