* [PATCH v3 1/4] binfmt_elf: Support segments with 0 filesz and misaligned starts
2023-09-27 3:42 [PATCH v3 0/4] binfmt_elf: Support segments with 0 filesz and misaligned starts Kees Cook
@ 2023-09-27 3:42 ` Kees Cook
2023-09-27 3:42 ` [PATCH v3 2/4] binfmt_elf: elf_bss no longer used by load_elf_binary() Kees Cook
` (4 subsequent siblings)
5 siblings, 0 replies; 9+ messages in thread
From: Kees Cook @ 2023-09-27 3:42 UTC (permalink / raw)
To: Eric Biederman
Cc: Kees Cook, Sebastian Ott, Thomas Weißschuh, Al Viro,
Christian Brauner, Pedro Falcato, linux-kernel, linux-fsdevel,
linux-mm, linux-hardening
From: "Eric W. Biederman" <ebiederm@xmission.com>
Implement a helper elf_load() that wraps elf_map() and performs all
of the necessary work to ensure that when "memsz > filesz" the bytes
described by "memsz > filesz" are zeroed.
An outstanding issue is if the first segment has filesz 0, and has a
randomized location. But that is the same as today.
In this change I replaced an open coded padzero() that did not clear
all of the way to the end of the page, with padzero() that does.
I also stopped checking the return of padzero() as there is at least
one known case where testing for failure is the wrong thing to do.
It looks like binfmt_elf_fdpic may have the proper set of tests
for when error handling can be safely completed.
I found a couple of commits in the old history
https://git.kernel.org/pub/scm/linux/kernel/git/tglx/history.git,
that look very interesting in understanding this code.
commit 39b56d902bf3 ("[PATCH] binfmt_elf: clearing bss may fail")
commit c6e2227e4a3e ("[SPARC64]: Missing user access return value checks in fs/binfmt_elf.c and fs/compat.c")
commit 5bf3be033f50 ("v2.4.10.1 -> v2.4.10.2")
Looking at commit 39b56d902bf3 ("[PATCH] binfmt_elf: clearing bss may fail"):
> commit 39b56d902bf35241e7cba6cc30b828ed937175ad
> Author: Pavel Machek <pavel@ucw.cz>
> Date: Wed Feb 9 22:40:30 2005 -0800
>
> [PATCH] binfmt_elf: clearing bss may fail
>
> So we discover that Borland's Kylix application builder emits weird elf
> files which describe a non-writeable bss segment.
>
> So remove the clear_user() check at the place where we zero out the bss. I
> don't _think_ there are any security implications here (plus we've never
> checked that clear_user() return value, so whoops if it is a problem).
>
> Signed-off-by: Pavel Machek <pavel@suse.cz>
> Signed-off-by: Andrew Morton <akpm@osdl.org>
> Signed-off-by: Linus Torvalds <torvalds@osdl.org>
It seems pretty clear that binfmt_elf_fdpic with skipping clear_user() for
non-writable segments and otherwise calling clear_user(), aka padzero(),
and checking it's return code is the right thing to do.
I just skipped the error checking as that avoids breaking things.
And notably, it looks like Borland's Kylix died in 2005 so it might be
safe to just consider read-only segments with memsz > filesz an error.
Reported-by: Sebastian Ott <sebott@redhat.com>
Reported-by: Thomas Weißschuh <linux@weissschuh.net>
Closes: https://lkml.kernel.org/r/20230914-bss-alloc-v1-1-78de67d2c6dd@weissschuh.net
Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com>
Link: https://lore.kernel.org/r/87sf71f123.fsf@email.froward.int.ebiederm.org
Signed-off-by: Kees Cook <keescook@chromium.org>
---
fs/binfmt_elf.c | 111 +++++++++++++++++++++---------------------------
1 file changed, 48 insertions(+), 63 deletions(-)
diff --git a/fs/binfmt_elf.c b/fs/binfmt_elf.c
index 7b3d2d491407..2a615f476e44 100644
--- a/fs/binfmt_elf.c
+++ b/fs/binfmt_elf.c
@@ -110,25 +110,6 @@ static struct linux_binfmt elf_format = {
#define BAD_ADDR(x) (unlikely((unsigned long)(x) >= TASK_SIZE))
-static int set_brk(unsigned long start, unsigned long end, int prot)
-{
- start = ELF_PAGEALIGN(start);
- end = ELF_PAGEALIGN(end);
- if (end > start) {
- /*
- * Map the last of the bss segment.
- * If the header is requesting these pages to be
- * executable, honour that (ppc32 needs this).
- */
- int error = vm_brk_flags(start, end - start,
- prot & PROT_EXEC ? VM_EXEC : 0);
- if (error)
- return error;
- }
- current->mm->start_brk = current->mm->brk = end;
- return 0;
-}
-
/* We need to explicitly zero any fractional pages
after the data section (i.e. bss). This would
contain the junk from the file that should not
@@ -406,6 +387,51 @@ static unsigned long elf_map(struct file *filep, unsigned long addr,
return(map_addr);
}
+static unsigned long elf_load(struct file *filep, unsigned long addr,
+ const struct elf_phdr *eppnt, int prot, int type,
+ unsigned long total_size)
+{
+ unsigned long zero_start, zero_end;
+ unsigned long map_addr;
+
+ if (eppnt->p_filesz) {
+ map_addr = elf_map(filep, addr, eppnt, prot, type, total_size);
+ if (BAD_ADDR(map_addr))
+ return map_addr;
+ if (eppnt->p_memsz > eppnt->p_filesz) {
+ zero_start = map_addr + ELF_PAGEOFFSET(eppnt->p_vaddr) +
+ eppnt->p_filesz;
+ zero_end = map_addr + ELF_PAGEOFFSET(eppnt->p_vaddr) +
+ eppnt->p_memsz;
+
+ /* Zero the end of the last mapped page */
+ padzero(zero_start);
+ }
+ } else {
+ map_addr = zero_start = ELF_PAGESTART(addr);
+ zero_end = zero_start + ELF_PAGEOFFSET(eppnt->p_vaddr) +
+ eppnt->p_memsz;
+ }
+ if (eppnt->p_memsz > eppnt->p_filesz) {
+ /*
+ * Map the last of the segment.
+ * If the header is requesting these pages to be
+ * executable, honour that (ppc32 needs this).
+ */
+ int error;
+
+ zero_start = ELF_PAGEALIGN(zero_start);
+ zero_end = ELF_PAGEALIGN(zero_end);
+
+ error = vm_brk_flags(zero_start, zero_end - zero_start,
+ prot & PROT_EXEC ? VM_EXEC : 0);
+ if (error)
+ map_addr = error;
+ }
+ return map_addr;
+}
+
+
static unsigned long total_mapping_size(const struct elf_phdr *phdr, int nr)
{
elf_addr_t min_addr = -1;
@@ -829,7 +855,6 @@ static int load_elf_binary(struct linux_binprm *bprm)
struct elf_phdr *elf_ppnt, *elf_phdata, *interp_elf_phdata = NULL;
struct elf_phdr *elf_property_phdata = NULL;
unsigned long elf_bss, elf_brk;
- int bss_prot = 0;
int retval, i;
unsigned long elf_entry;
unsigned long e_entry;
@@ -1040,33 +1065,6 @@ static int load_elf_binary(struct linux_binprm *bprm)
if (elf_ppnt->p_type != PT_LOAD)
continue;
- if (unlikely (elf_brk > elf_bss)) {
- unsigned long nbyte;
-
- /* There was a PT_LOAD segment with p_memsz > p_filesz
- before this one. Map anonymous pages, if needed,
- and clear the area. */
- retval = set_brk(elf_bss + load_bias,
- elf_brk + load_bias,
- bss_prot);
- if (retval)
- goto out_free_dentry;
- nbyte = ELF_PAGEOFFSET(elf_bss);
- if (nbyte) {
- nbyte = ELF_MIN_ALIGN - nbyte;
- if (nbyte > elf_brk - elf_bss)
- nbyte = elf_brk - elf_bss;
- if (clear_user((void __user *)elf_bss +
- load_bias, nbyte)) {
- /*
- * This bss-zeroing can fail if the ELF
- * file specifies odd protections. So
- * we don't check the return value
- */
- }
- }
- }
-
elf_prot = make_prot(elf_ppnt->p_flags, &arch_state,
!!interpreter, false);
@@ -1162,7 +1160,7 @@ static int load_elf_binary(struct linux_binprm *bprm)
}
}
- error = elf_map(bprm->file, load_bias + vaddr, elf_ppnt,
+ error = elf_load(bprm->file, load_bias + vaddr, elf_ppnt,
elf_prot, elf_flags, total_size);
if (BAD_ADDR(error)) {
retval = IS_ERR_VALUE(error) ?
@@ -1217,10 +1215,8 @@ static int load_elf_binary(struct linux_binprm *bprm)
if (end_data < k)
end_data = k;
k = elf_ppnt->p_vaddr + elf_ppnt->p_memsz;
- if (k > elf_brk) {
- bss_prot = elf_prot;
+ if (k > elf_brk)
elf_brk = k;
- }
}
e_entry = elf_ex->e_entry + load_bias;
@@ -1232,18 +1228,7 @@ static int load_elf_binary(struct linux_binprm *bprm)
start_data += load_bias;
end_data += load_bias;
- /* Calling set_brk effectively mmaps the pages that we need
- * for the bss and break sections. We must do this before
- * mapping in the interpreter, to make sure it doesn't wind
- * up getting placed where the bss needs to go.
- */
- retval = set_brk(elf_bss, elf_brk, bss_prot);
- if (retval)
- goto out_free_dentry;
- if (likely(elf_bss != elf_brk) && unlikely(padzero(elf_bss))) {
- retval = -EFAULT; /* Nobody gets to see this, but.. */
- goto out_free_dentry;
- }
+ current->mm->start_brk = current->mm->brk = ELF_PAGEALIGN(elf_brk);
if (interpreter) {
elf_entry = load_elf_interp(interp_elf_ex,
--
2.34.1
^ permalink raw reply related [flat|nested] 9+ messages in thread* [PATCH v3 2/4] binfmt_elf: elf_bss no longer used by load_elf_binary()
2023-09-27 3:42 [PATCH v3 0/4] binfmt_elf: Support segments with 0 filesz and misaligned starts Kees Cook
2023-09-27 3:42 ` [PATCH v3 1/4] " Kees Cook
@ 2023-09-27 3:42 ` Kees Cook
2023-09-27 3:42 ` [PATCH v3 3/4] binfmt_elf: Provide prot bits as context for padzero() errors Kees Cook
` (3 subsequent siblings)
5 siblings, 0 replies; 9+ messages in thread
From: Kees Cook @ 2023-09-27 3:42 UTC (permalink / raw)
To: Eric Biederman
Cc: Kees Cook, Alexander Viro, Christian Brauner, linux-fsdevel,
linux-mm, Sebastian Ott, Thomas Weißschuh, Pedro Falcato,
linux-kernel, linux-hardening
With the BSS handled generically via the new filesz/memsz mismatch
handling logic in elf_load(), elf_bss no longer needs to be tracked.
Drop the variable.
Cc: Eric Biederman <ebiederm@xmission.com>
Cc: Alexander Viro <viro@zeniv.linux.org.uk>
Cc: Christian Brauner <brauner@kernel.org>
Cc: linux-fsdevel@vger.kernel.org
Cc: linux-mm@kvack.org
Suggested-by: Eric Biederman <ebiederm@xmission.com>
Signed-off-by: Kees Cook <keescook@chromium.org>
---
fs/binfmt_elf.c | 6 +-----
1 file changed, 1 insertion(+), 5 deletions(-)
diff --git a/fs/binfmt_elf.c b/fs/binfmt_elf.c
index 2a615f476e44..0214d5a949fc 100644
--- a/fs/binfmt_elf.c
+++ b/fs/binfmt_elf.c
@@ -854,7 +854,7 @@ static int load_elf_binary(struct linux_binprm *bprm)
unsigned long error;
struct elf_phdr *elf_ppnt, *elf_phdata, *interp_elf_phdata = NULL;
struct elf_phdr *elf_property_phdata = NULL;
- unsigned long elf_bss, elf_brk;
+ unsigned long elf_brk;
int retval, i;
unsigned long elf_entry;
unsigned long e_entry;
@@ -1045,7 +1045,6 @@ static int load_elf_binary(struct linux_binprm *bprm)
if (retval < 0)
goto out_free_dentry;
- elf_bss = 0;
elf_brk = 0;
start_code = ~0UL;
@@ -1208,8 +1207,6 @@ static int load_elf_binary(struct linux_binprm *bprm)
k = elf_ppnt->p_vaddr + elf_ppnt->p_filesz;
- if (k > elf_bss)
- elf_bss = k;
if ((elf_ppnt->p_flags & PF_X) && end_code < k)
end_code = k;
if (end_data < k)
@@ -1221,7 +1218,6 @@ static int load_elf_binary(struct linux_binprm *bprm)
e_entry = elf_ex->e_entry + load_bias;
phdr_addr += load_bias;
- elf_bss += load_bias;
elf_brk += load_bias;
start_code += load_bias;
end_code += load_bias;
--
2.34.1
^ permalink raw reply related [flat|nested] 9+ messages in thread* [PATCH v3 3/4] binfmt_elf: Provide prot bits as context for padzero() errors
2023-09-27 3:42 [PATCH v3 0/4] binfmt_elf: Support segments with 0 filesz and misaligned starts Kees Cook
2023-09-27 3:42 ` [PATCH v3 1/4] " Kees Cook
2023-09-27 3:42 ` [PATCH v3 2/4] binfmt_elf: elf_bss no longer used by load_elf_binary() Kees Cook
@ 2023-09-27 3:42 ` Kees Cook
2023-09-27 20:18 ` Eric W. Biederman
2023-09-27 3:42 ` [PATCH v3 4/4] binfmt_elf: Use elf_load() for interpreter Kees Cook
` (2 subsequent siblings)
5 siblings, 1 reply; 9+ messages in thread
From: Kees Cook @ 2023-09-27 3:42 UTC (permalink / raw)
To: Eric Biederman
Cc: Kees Cook, Alexander Viro, Christian Brauner, linux-fsdevel,
linux-mm, Sebastian Ott, Thomas Weißschuh, Pedro Falcato,
linux-kernel, linux-hardening
Errors with padzero() should be caught unless we're expecting a
pathological (non-writable) segment. Report -EFAULT only when PROT_WRITE
is present.
Additionally add some more documentation to padzero(), elf_map(), and
elf_load().
Cc: Eric Biederman <ebiederm@xmission.com>
Cc: Alexander Viro <viro@zeniv.linux.org.uk>
Cc: Christian Brauner <brauner@kernel.org>
Cc: linux-fsdevel@vger.kernel.org
Cc: linux-mm@kvack.org
Suggested-by: Eric Biederman <ebiederm@xmission.com>
Signed-off-by: Kees Cook <keescook@chromium.org>
---
fs/binfmt_elf.c | 33 +++++++++++++++++++++++----------
1 file changed, 23 insertions(+), 10 deletions(-)
diff --git a/fs/binfmt_elf.c b/fs/binfmt_elf.c
index 0214d5a949fc..b939cfe3215c 100644
--- a/fs/binfmt_elf.c
+++ b/fs/binfmt_elf.c
@@ -110,19 +110,21 @@ static struct linux_binfmt elf_format = {
#define BAD_ADDR(x) (unlikely((unsigned long)(x) >= TASK_SIZE))
-/* We need to explicitly zero any fractional pages
- after the data section (i.e. bss). This would
- contain the junk from the file that should not
- be in memory
+/*
+ * We need to explicitly zero any trailing portion of the page that follows
+ * p_filesz when it ends before the page ends (e.g. bss), otherwise this
+ * memory will contain the junk from the file that should not be present.
*/
-static int padzero(unsigned long elf_bss)
+static int padzero(unsigned long address, int prot)
{
unsigned long nbyte;
- nbyte = ELF_PAGEOFFSET(elf_bss);
+ nbyte = ELF_PAGEOFFSET(address);
if (nbyte) {
nbyte = ELF_MIN_ALIGN - nbyte;
- if (clear_user((void __user *) elf_bss, nbyte))
+ /* Only report errors when the segment is writable. */
+ if (clear_user((void __user *)address, nbyte) &&
+ prot & PROT_WRITE)
return -EFAULT;
}
return 0;
@@ -348,6 +350,11 @@ create_elf_tables(struct linux_binprm *bprm, const struct elfhdr *exec,
return 0;
}
+/*
+ * Map "eppnt->p_filesz" bytes from "filep" offset "eppnt->p_offset"
+ * into memory at "addr". (Note that p_filesz is rounded up to the
+ * next page, so any extra bytes from the file must be wiped.)
+ */
static unsigned long elf_map(struct file *filep, unsigned long addr,
const struct elf_phdr *eppnt, int prot, int type,
unsigned long total_size)
@@ -387,6 +394,11 @@ static unsigned long elf_map(struct file *filep, unsigned long addr,
return(map_addr);
}
+/*
+ * Map "eppnt->p_filesz" bytes from "filep" offset "eppnt->p_offset"
+ * into memory at "addr". Memory from "p_filesz" through "p_memsz"
+ * rounded up to the next page is zeroed.
+ */
static unsigned long elf_load(struct file *filep, unsigned long addr,
const struct elf_phdr *eppnt, int prot, int type,
unsigned long total_size)
@@ -405,7 +417,8 @@ static unsigned long elf_load(struct file *filep, unsigned long addr,
eppnt->p_memsz;
/* Zero the end of the last mapped page */
- padzero(zero_start);
+ if (padzero(zero_start, prot))
+ return -EFAULT;
}
} else {
map_addr = zero_start = ELF_PAGESTART(addr);
@@ -712,7 +725,7 @@ static unsigned long load_elf_interp(struct elfhdr *interp_elf_ex,
* the file up to the page boundary, and zero it from elf_bss
* up to the end of the page.
*/
- if (padzero(elf_bss)) {
+ if (padzero(elf_bss, bss_prot)) {
error = -EFAULT;
goto out;
}
@@ -1407,7 +1420,7 @@ static int load_elf_library(struct file *file)
goto out_free_ph;
elf_bss = eppnt->p_vaddr + eppnt->p_filesz;
- if (padzero(elf_bss)) {
+ if (padzero(elf_bss, PROT_WRITE)) {
error = -EFAULT;
goto out_free_ph;
}
--
2.34.1
^ permalink raw reply related [flat|nested] 9+ messages in thread* Re: [PATCH v3 3/4] binfmt_elf: Provide prot bits as context for padzero() errors
2023-09-27 3:42 ` [PATCH v3 3/4] binfmt_elf: Provide prot bits as context for padzero() errors Kees Cook
@ 2023-09-27 20:18 ` Eric W. Biederman
2023-09-29 0:51 ` Kees Cook
0 siblings, 1 reply; 9+ messages in thread
From: Eric W. Biederman @ 2023-09-27 20:18 UTC (permalink / raw)
To: Kees Cook
Cc: Alexander Viro, Christian Brauner, linux-fsdevel, linux-mm,
Sebastian Ott, Thomas Weißschuh, Pedro Falcato, linux-kernel,
linux-hardening
Kees Cook <keescook@chromium.org> writes:
> Errors with padzero() should be caught unless we're expecting a
> pathological (non-writable) segment. Report -EFAULT only when PROT_WRITE
> is present.
>
> Additionally add some more documentation to padzero(), elf_map(), and
> elf_load().
I wonder if this might be easier to just perform the PROT_WRITE
test in elf_load, and to completely skip padzero of PROT_WRITE
is not present.
Eric
> Cc: Eric Biederman <ebiederm@xmission.com>
> Cc: Alexander Viro <viro@zeniv.linux.org.uk>
> Cc: Christian Brauner <brauner@kernel.org>
> Cc: linux-fsdevel@vger.kernel.org
> Cc: linux-mm@kvack.org
> Suggested-by: Eric Biederman <ebiederm@xmission.com>
> Signed-off-by: Kees Cook <keescook@chromium.org>
> ---
> fs/binfmt_elf.c | 33 +++++++++++++++++++++++----------
> 1 file changed, 23 insertions(+), 10 deletions(-)
>
> diff --git a/fs/binfmt_elf.c b/fs/binfmt_elf.c
> index 0214d5a949fc..b939cfe3215c 100644
> --- a/fs/binfmt_elf.c
> +++ b/fs/binfmt_elf.c
> @@ -110,19 +110,21 @@ static struct linux_binfmt elf_format = {
>
> #define BAD_ADDR(x) (unlikely((unsigned long)(x) >= TASK_SIZE))
>
> -/* We need to explicitly zero any fractional pages
> - after the data section (i.e. bss). This would
> - contain the junk from the file that should not
> - be in memory
> +/*
> + * We need to explicitly zero any trailing portion of the page that follows
> + * p_filesz when it ends before the page ends (e.g. bss), otherwise this
> + * memory will contain the junk from the file that should not be present.
> */
> -static int padzero(unsigned long elf_bss)
> +static int padzero(unsigned long address, int prot)
> {
> unsigned long nbyte;
>
> - nbyte = ELF_PAGEOFFSET(elf_bss);
> + nbyte = ELF_PAGEOFFSET(address);
> if (nbyte) {
> nbyte = ELF_MIN_ALIGN - nbyte;
> - if (clear_user((void __user *) elf_bss, nbyte))
> + /* Only report errors when the segment is writable. */
> + if (clear_user((void __user *)address, nbyte) &&
> + prot & PROT_WRITE)
> return -EFAULT;
> }
> return 0;
> @@ -348,6 +350,11 @@ create_elf_tables(struct linux_binprm *bprm, const struct elfhdr *exec,
> return 0;
> }
>
> +/*
> + * Map "eppnt->p_filesz" bytes from "filep" offset "eppnt->p_offset"
> + * into memory at "addr". (Note that p_filesz is rounded up to the
> + * next page, so any extra bytes from the file must be wiped.)
> + */
> static unsigned long elf_map(struct file *filep, unsigned long addr,
> const struct elf_phdr *eppnt, int prot, int type,
> unsigned long total_size)
> @@ -387,6 +394,11 @@ static unsigned long elf_map(struct file *filep, unsigned long addr,
> return(map_addr);
> }
>
> +/*
> + * Map "eppnt->p_filesz" bytes from "filep" offset "eppnt->p_offset"
> + * into memory at "addr". Memory from "p_filesz" through "p_memsz"
> + * rounded up to the next page is zeroed.
> + */
> static unsigned long elf_load(struct file *filep, unsigned long addr,
> const struct elf_phdr *eppnt, int prot, int type,
> unsigned long total_size)
> @@ -405,7 +417,8 @@ static unsigned long elf_load(struct file *filep, unsigned long addr,
> eppnt->p_memsz;
>
> /* Zero the end of the last mapped page */
> - padzero(zero_start);
> + if (padzero(zero_start, prot))
> + return -EFAULT;
> }
> } else {
> map_addr = zero_start = ELF_PAGESTART(addr);
> @@ -712,7 +725,7 @@ static unsigned long load_elf_interp(struct elfhdr *interp_elf_ex,
> * the file up to the page boundary, and zero it from elf_bss
> * up to the end of the page.
> */
> - if (padzero(elf_bss)) {
> + if (padzero(elf_bss, bss_prot)) {
> error = -EFAULT;
> goto out;
> }
> @@ -1407,7 +1420,7 @@ static int load_elf_library(struct file *file)
> goto out_free_ph;
>
> elf_bss = eppnt->p_vaddr + eppnt->p_filesz;
> - if (padzero(elf_bss)) {
> + if (padzero(elf_bss, PROT_WRITE)) {
> error = -EFAULT;
> goto out_free_ph;
> }
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH v3 3/4] binfmt_elf: Provide prot bits as context for padzero() errors
2023-09-27 20:18 ` Eric W. Biederman
@ 2023-09-29 0:51 ` Kees Cook
0 siblings, 0 replies; 9+ messages in thread
From: Kees Cook @ 2023-09-29 0:51 UTC (permalink / raw)
To: Eric W. Biederman
Cc: Alexander Viro, Christian Brauner, linux-fsdevel, linux-mm,
Sebastian Ott, Thomas Weißschuh, Pedro Falcato, linux-kernel,
linux-hardening
On Wed, Sep 27, 2023 at 03:18:34PM -0500, Eric W. Biederman wrote:
> Kees Cook <keescook@chromium.org> writes:
>
> > Errors with padzero() should be caught unless we're expecting a
> > pathological (non-writable) segment. Report -EFAULT only when PROT_WRITE
> > is present.
> >
> > Additionally add some more documentation to padzero(), elf_map(), and
> > elf_load().
>
> I wonder if this might be easier to just perform the PROT_WRITE
> test in elf_load, and to completely skip padzero of PROT_WRITE
> is not present.
Yeah, actually, after moving load_elf_library() to elf_load(), there's
only 1 caller of padzero... :P
I'll work on that.
-Kees
>
> Eric
>
> > Cc: Eric Biederman <ebiederm@xmission.com>
> > Cc: Alexander Viro <viro@zeniv.linux.org.uk>
> > Cc: Christian Brauner <brauner@kernel.org>
> > Cc: linux-fsdevel@vger.kernel.org
> > Cc: linux-mm@kvack.org
> > Suggested-by: Eric Biederman <ebiederm@xmission.com>
> > Signed-off-by: Kees Cook <keescook@chromium.org>
> > ---
> > fs/binfmt_elf.c | 33 +++++++++++++++++++++++----------
> > 1 file changed, 23 insertions(+), 10 deletions(-)
> >
> > diff --git a/fs/binfmt_elf.c b/fs/binfmt_elf.c
> > index 0214d5a949fc..b939cfe3215c 100644
> > --- a/fs/binfmt_elf.c
> > +++ b/fs/binfmt_elf.c
> > @@ -110,19 +110,21 @@ static struct linux_binfmt elf_format = {
> >
> > #define BAD_ADDR(x) (unlikely((unsigned long)(x) >= TASK_SIZE))
> >
> > -/* We need to explicitly zero any fractional pages
> > - after the data section (i.e. bss). This would
> > - contain the junk from the file that should not
> > - be in memory
> > +/*
> > + * We need to explicitly zero any trailing portion of the page that follows
> > + * p_filesz when it ends before the page ends (e.g. bss), otherwise this
> > + * memory will contain the junk from the file that should not be present.
> > */
> > -static int padzero(unsigned long elf_bss)
> > +static int padzero(unsigned long address, int prot)
> > {
> > unsigned long nbyte;
> >
> > - nbyte = ELF_PAGEOFFSET(elf_bss);
> > + nbyte = ELF_PAGEOFFSET(address);
> > if (nbyte) {
> > nbyte = ELF_MIN_ALIGN - nbyte;
> > - if (clear_user((void __user *) elf_bss, nbyte))
> > + /* Only report errors when the segment is writable. */
> > + if (clear_user((void __user *)address, nbyte) &&
> > + prot & PROT_WRITE)
> > return -EFAULT;
> > }
> > return 0;
> > @@ -348,6 +350,11 @@ create_elf_tables(struct linux_binprm *bprm, const struct elfhdr *exec,
> > return 0;
> > }
> >
> > +/*
> > + * Map "eppnt->p_filesz" bytes from "filep" offset "eppnt->p_offset"
> > + * into memory at "addr". (Note that p_filesz is rounded up to the
> > + * next page, so any extra bytes from the file must be wiped.)
> > + */
> > static unsigned long elf_map(struct file *filep, unsigned long addr,
> > const struct elf_phdr *eppnt, int prot, int type,
> > unsigned long total_size)
> > @@ -387,6 +394,11 @@ static unsigned long elf_map(struct file *filep, unsigned long addr,
> > return(map_addr);
> > }
> >
> > +/*
> > + * Map "eppnt->p_filesz" bytes from "filep" offset "eppnt->p_offset"
> > + * into memory at "addr". Memory from "p_filesz" through "p_memsz"
> > + * rounded up to the next page is zeroed.
> > + */
> > static unsigned long elf_load(struct file *filep, unsigned long addr,
> > const struct elf_phdr *eppnt, int prot, int type,
> > unsigned long total_size)
> > @@ -405,7 +417,8 @@ static unsigned long elf_load(struct file *filep, unsigned long addr,
> > eppnt->p_memsz;
> >
> > /* Zero the end of the last mapped page */
> > - padzero(zero_start);
> > + if (padzero(zero_start, prot))
> > + return -EFAULT;
> > }
> > } else {
> > map_addr = zero_start = ELF_PAGESTART(addr);
> > @@ -712,7 +725,7 @@ static unsigned long load_elf_interp(struct elfhdr *interp_elf_ex,
> > * the file up to the page boundary, and zero it from elf_bss
> > * up to the end of the page.
> > */
> > - if (padzero(elf_bss)) {
> > + if (padzero(elf_bss, bss_prot)) {
> > error = -EFAULT;
> > goto out;
> > }
> > @@ -1407,7 +1420,7 @@ static int load_elf_library(struct file *file)
> > goto out_free_ph;
> >
> > elf_bss = eppnt->p_vaddr + eppnt->p_filesz;
> > - if (padzero(elf_bss)) {
> > + if (padzero(elf_bss, PROT_WRITE)) {
> > error = -EFAULT;
> > goto out_free_ph;
> > }
--
Kees Cook
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH v3 4/4] binfmt_elf: Use elf_load() for interpreter
2023-09-27 3:42 [PATCH v3 0/4] binfmt_elf: Support segments with 0 filesz and misaligned starts Kees Cook
` (2 preceding siblings ...)
2023-09-27 3:42 ` [PATCH v3 3/4] binfmt_elf: Provide prot bits as context for padzero() errors Kees Cook
@ 2023-09-27 3:42 ` Kees Cook
2023-09-27 20:25 ` [PATCH v3 0/4] binfmt_elf: Support segments with 0 filesz and misaligned starts Eric W. Biederman
2023-09-28 12:55 ` Sebastian Ott
5 siblings, 0 replies; 9+ messages in thread
From: Kees Cook @ 2023-09-27 3:42 UTC (permalink / raw)
To: Eric Biederman
Cc: Kees Cook, Alexander Viro, Christian Brauner, linux-fsdevel,
linux-mm, Pedro Falcato, Sebastian Ott, Thomas Weißschuh,
linux-kernel, linux-hardening
Handle arbitrary memsz>filesz in interpreter ELF segments, instead of
only supporting it in the last segment (which is expected to be the
BSS).
Cc: Eric Biederman <ebiederm@xmission.com>
Cc: Alexander Viro <viro@zeniv.linux.org.uk>
Cc: Christian Brauner <brauner@kernel.org>
Cc: linux-fsdevel@vger.kernel.org
Cc: linux-mm@kvack.org
Reported-by: Pedro Falcato <pedro.falcato@gmail.com>
Closes: https://lore.kernel.org/lkml/20221106021657.1145519-1-pedro.falcato@gmail.com/
Signed-off-by: Kees Cook <keescook@chromium.org>
---
fs/binfmt_elf.c | 46 +---------------------------------------------
1 file changed, 1 insertion(+), 45 deletions(-)
diff --git a/fs/binfmt_elf.c b/fs/binfmt_elf.c
index b939cfe3215c..74af5c8319a0 100644
--- a/fs/binfmt_elf.c
+++ b/fs/binfmt_elf.c
@@ -635,8 +635,6 @@ static unsigned long load_elf_interp(struct elfhdr *interp_elf_ex,
struct elf_phdr *eppnt;
unsigned long load_addr = 0;
int load_addr_set = 0;
- unsigned long last_bss = 0, elf_bss = 0;
- int bss_prot = 0;
unsigned long error = ~0UL;
unsigned long total_size;
int i;
@@ -673,7 +671,7 @@ static unsigned long load_elf_interp(struct elfhdr *interp_elf_ex,
else if (no_base && interp_elf_ex->e_type == ET_DYN)
load_addr = -vaddr;
- map_addr = elf_map(interpreter, load_addr + vaddr,
+ map_addr = elf_load(interpreter, load_addr + vaddr,
eppnt, elf_prot, elf_type, total_size);
total_size = 0;
error = map_addr;
@@ -699,51 +697,9 @@ static unsigned long load_elf_interp(struct elfhdr *interp_elf_ex,
error = -ENOMEM;
goto out;
}
-
- /*
- * Find the end of the file mapping for this phdr, and
- * keep track of the largest address we see for this.
- */
- k = load_addr + eppnt->p_vaddr + eppnt->p_filesz;
- if (k > elf_bss)
- elf_bss = k;
-
- /*
- * Do the same thing for the memory mapping - between
- * elf_bss and last_bss is the bss section.
- */
- k = load_addr + eppnt->p_vaddr + eppnt->p_memsz;
- if (k > last_bss) {
- last_bss = k;
- bss_prot = elf_prot;
- }
}
}
- /*
- * Now fill out the bss section: first pad the last page from
- * the file up to the page boundary, and zero it from elf_bss
- * up to the end of the page.
- */
- if (padzero(elf_bss, bss_prot)) {
- error = -EFAULT;
- goto out;
- }
- /*
- * Next, align both the file and mem bss up to the page size,
- * since this is where elf_bss was just zeroed up to, and where
- * last_bss will end after the vm_brk_flags() below.
- */
- elf_bss = ELF_PAGEALIGN(elf_bss);
- last_bss = ELF_PAGEALIGN(last_bss);
- /* Finally, if there is still more bss to allocate, do it. */
- if (last_bss > elf_bss) {
- error = vm_brk_flags(elf_bss, last_bss - elf_bss,
- bss_prot & PROT_EXEC ? VM_EXEC : 0);
- if (error)
- goto out;
- }
-
error = load_addr;
out:
return error;
--
2.34.1
^ permalink raw reply related [flat|nested] 9+ messages in thread* Re: [PATCH v3 0/4] binfmt_elf: Support segments with 0 filesz and misaligned starts
2023-09-27 3:42 [PATCH v3 0/4] binfmt_elf: Support segments with 0 filesz and misaligned starts Kees Cook
` (3 preceding siblings ...)
2023-09-27 3:42 ` [PATCH v3 4/4] binfmt_elf: Use elf_load() for interpreter Kees Cook
@ 2023-09-27 20:25 ` Eric W. Biederman
2023-09-28 12:55 ` Sebastian Ott
5 siblings, 0 replies; 9+ messages in thread
From: Eric W. Biederman @ 2023-09-27 20:25 UTC (permalink / raw)
To: Kees Cook
Cc: Sebastian Ott, Thomas Weißschuh, Al Viro, Christian Brauner,
Pedro Falcato, linux-kernel, linux-fsdevel, linux-mm,
linux-hardening
Kees Cook <keescook@chromium.org> writes:
> Hi,
>
> This is the continuation of the work Eric started for handling
> "p_memsz > p_filesz" in arbitrary segments (rather than just the last,
> BSS, segment). I've added the suggested changes:
>
> - drop unused "elf_bss" variable
> - report padzero() errors when PROT_WRITE is present
> - refactor load_elf_interp() to use elf_load()
>
> This passes my quick smoke tests, but I'm still trying to construct some
> more complete tests...
Acked-by: "Eric W. Biederman" <ebiederm@xmission.com>
You might also consider using elf_load in load_elf_library.
The code in load_elf_library only supports files with a single program
header, and I think is only needed for libc5.
The advantage is that load_elf_library would be using well tested code,
vm_brk would have no callers, and padzero would only be called by
elf_load, and load_elf_library would do little more than just call
load_elf_library.
Eric
>
> -Kees
>
> Eric W. Biederman (1):
> binfmt_elf: Support segments with 0 filesz and misaligned starts
>
> Kees Cook (3):
> binfmt_elf: elf_bss no longer used by load_elf_binary()
> binfmt_elf: Provide prot bits as context for padzero() errors
> binfmt_elf: Use elf_load() for interpreter
>
> fs/binfmt_elf.c | 192 ++++++++++++++++++------------------------------
> 1 file changed, 71 insertions(+), 121 deletions(-)
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH v3 0/4] binfmt_elf: Support segments with 0 filesz and misaligned starts
2023-09-27 3:42 [PATCH v3 0/4] binfmt_elf: Support segments with 0 filesz and misaligned starts Kees Cook
` (4 preceding siblings ...)
2023-09-27 20:25 ` [PATCH v3 0/4] binfmt_elf: Support segments with 0 filesz and misaligned starts Eric W. Biederman
@ 2023-09-28 12:55 ` Sebastian Ott
5 siblings, 0 replies; 9+ messages in thread
From: Sebastian Ott @ 2023-09-28 12:55 UTC (permalink / raw)
To: Kees Cook
Cc: Eric Biederman, Thomas Weißschuh, Al Viro, Christian Brauner,
Pedro Falcato, linux-kernel, linux-fsdevel, linux-mm,
linux-hardening
On Tue, 26 Sep 2023, Kees Cook wrote:
> This is the continuation of the work Eric started for handling
> "p_memsz > p_filesz" in arbitrary segments (rather than just the last,
> BSS, segment). I've added the suggested changes:
>
> - drop unused "elf_bss" variable
> - report padzero() errors when PROT_WRITE is present
> - refactor load_elf_interp() to use elf_load()
>
> This passes my quick smoke tests, but I'm still trying to construct some
> more complete tests...
I've repeated all my tests with this one - no issues found.
Thanks,
Sebastian
^ permalink raw reply [flat|nested] 9+ messages in thread