* Is it legal to return positive value when do_execve() succeeds?
@ 2010-09-30 13:07 Tetsuo Handa
2010-10-01 2:29 ` Tetsuo Handa
2010-10-01 12:44 ` Tetsuo Handa
0 siblings, 2 replies; 13+ messages in thread
From: Tetsuo Handa @ 2010-09-30 13:07 UTC (permalink / raw)
To: linux-fsdevel; +Cc: linux-kernel
Hello.
I was browsing do_execve() and noticed that do_execve() will return positive
return value if "struct linux_binfmt"->load_binary() returned positive return
value. So far, all in-tree "struct linux_binfmt" users seem to return 0 on
success and negative return value on failure. But search_binary_handler()
itself is designed to allow positive return value on successful execve().
--- a/fs/exec.c
+++ b/fs/exec.c
@@ -1413,6 +1413,9 @@ int do_execve(const char * filename,
free_bprm(bprm);
if (displaced)
put_files_struct(displaced);
+ /* Just for testing. */
+ if (!retval)
+ retval = 1;
return retval;
out:
With a patch above (on x86 CentOS 5.5 with 2.6.36-rc6 kernel),
a few programs failed to work properly.
udevd-event[$PID]: run_program: '$PROGRAM' abnormal exit
Is it legal to return positive value when do_execve() succeeds?
Regards.
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: Is it legal to return positive value when do_execve() succeeds?
2010-09-30 13:07 Is it legal to return positive value when do_execve() succeeds? Tetsuo Handa
@ 2010-10-01 2:29 ` Tetsuo Handa
2010-10-01 5:19 ` Tetsuo Handa
2010-10-01 12:44 ` Tetsuo Handa
1 sibling, 1 reply; 13+ messages in thread
From: Tetsuo Handa @ 2010-10-01 2:29 UTC (permalink / raw)
To: linux-fsdevel; +Cc: linux-kernel
Tetsuo Handa wrote:
> Is it legal to return positive value when do_execve() succeeds?
It seems to me that do_execve() should not return positive return code because
some callers are expecting do_execve() to return 0 on success.
linux-2.6.36-rc6/arch/x86/kernel/process.c:
304 long sys_execve(const char __user *name,
305 const char __user *const __user *argv,
306 const char __user *const __user *envp, struct pt_regs *regs)
307 {
308 long error;
309 char *filename;
310
311 filename = getname(name);
312 error = PTR_ERR(filename);
313 if (IS_ERR(filename))
314 return error;
315 error = do_execve(filename, argv, envp, regs);
316
317 #ifdef CONFIG_X86_32
318 if (error == 0) {
319 /* Make sure we don't return using sysenter.. */
320 set_thread_flag(TIF_IRET);
321 }
322 #endif
323
324 putname(filename);
325 return error;
326 }
But there are several "struct linux_binfmt"->load_binary() users who return
positive return code for error paths.
load_elf_binary() will return positive return code if set_brk() returned
positive return code.
load_aout_binary() will return positive return code if do_brk() or
do_mmap() returned unwanted result.
Are we using positive return code for telling unrecoverable errors?
send_sig(SIGKILL, current, 0) is used when execve() failed after reaching
"the point of no return", isn't it? But set_thread_flag(TIF_IRET) will not be
called if execve() failed after "the point of no return".
Is this no problem as the current task will be SIGKILL'ed as soon as returning
using sysenter()?
Regards.
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: Is it legal to return positive value when do_execve() succeeds?
2010-10-01 2:29 ` Tetsuo Handa
@ 2010-10-01 5:19 ` Tetsuo Handa
2010-10-04 4:59 ` KOSAKI Motohiro
0 siblings, 1 reply; 13+ messages in thread
From: Tetsuo Handa @ 2010-10-01 5:19 UTC (permalink / raw)
To: linux-fsdevel; +Cc: linux-kernel
> But there are several "struct linux_binfmt"->load_binary() users who return
> positive return code for error paths.
>
> load_elf_binary() will return positive return code if set_brk() returned
> positive return code.
>
It seems to me that this causes undefined behavior.
arch/x86/include/asm/page_32_types.h:
16 #define __PAGE_OFFSET _AC(CONFIG_PAGE_OFFSET, UL)
In my .config , CONFIG_PAGE_OFFSET is 0xC0000000.
arch/x86/include/asm/page_types.h:
29 #define PAGE_OFFSET ((unsigned long)__PAGE_OFFSET)
Thus, PAGE_OFFSET == 0xC0000000.
arch/x86/include/asm/processor.h:
882 #ifdef CONFIG_X86_32
883 /*
884 * User space process size: 3GB (default).
885 */
886 #define TASK_SIZE PAGE_OFFSET
Thus, TASK_SIZE == 0xC0000000.
fs/binfmt_elf.c:
77 #define BAD_ADDR(x) ((unsigned long)(x) >= TASK_SIZE)
78
79 static int set_brk(unsigned long start, unsigned long end)
80 {
81 start = ELF_PAGEALIGN(start);
82 end = ELF_PAGEALIGN(end);
83 if (end > start) {
84 unsigned long addr;
85 down_write(¤t->mm->mmap_sem);
86 addr = do_brk(start, end - start);
87 up_write(¤t->mm->mmap_sem);
88 if (BAD_ADDR(addr))
89 return addr;
90 }
91 current->mm->start_brk = current->mm->brk = end;
92 return 0;
93 }
Thus, BAD_ADDR(x) is ((unsigned long)(x) >= 0xC0000000).
fs/binfmt_elf.c:
564 static int load_elf_binary(struct linux_binprm *bprm, struct pt_regs *regs)
(...snipped...)
573 int retval, i;
(...snipped...)
875 retval = set_brk(elf_bss, elf_brk);
876 if (retval) {
877 send_sig(SIGKILL, current, 0);
878 goto out_free_dentry;
879 }
(...snipped...)
981 out:
982 kfree(loc);
983 out_ret:
984 return retval;
985
986 /* error cleanup */
987 out_free_dentry:
988 allow_write_access(interpreter);
989 if (interpreter)
990 fput(interpreter);
991 out_free_interp:
992 kfree(elf_interpreter);
993 out_free_ph:
994 kfree(elf_phdata);
995 goto out;
Here retval can take any integer between -1073741824 and 0.
1255 int search_binary_handler(struct linux_binprm *bprm,struct pt_regs *regs)
1256 {
1257 unsigned int depth = bprm->recursion_depth;
1258 int try,retval;
(...snipped...)
1277 int (*fn)(struct linux_binprm *, struct pt_regs *) = fmt->load_binary;
1278 if (!fn)
1279 continue;
1280 if (!try_module_get(fmt->module))
1281 continue;
1282 read_unlock(&binfmt_lock);
1283 retval = fn(bprm, regs);
1284 /*
1285 * Restore the depth counter to its starting value
1286 * in this call, so we don't have to rely on every
1287 * load_binary function to restore it on return.
1288 */
1289 bprm->recursion_depth = depth;
1290 if (retval >= 0) {
1291 if (depth == 0)
1292 tracehook_report_exec(fmt, bprm, regs);
1293 put_binfmt(fmt);
1294 allow_write_access(bprm->file);
1295 if (bprm->file)
1296 fput(bprm->file);
1297 bprm->file = NULL;
1298 current->did_exec = 1;
1299 proc_exec_connector(current);
1300 return retval;
1301 }
1302 read_lock(&binfmt_lock);
1303 put_binfmt(fmt);
1304 if (retval != -ENOEXEC || bprm->mm == NULL)
1305 break;
load_elf_binary() can return any integer between -1073741824 and 0.
Thus, retval can take between -1073741824 and 0.
If retval == -ENOEXEC, search_binary_handler() would continue loop even after
load_elf_binary() recognized the image.
If retval == -EPERM, search_binary_handler() would return and do_execve()
behaves as if execve() was failed due to -EPERM (although execve() has already
reached "the point of no return" and previous image was already gone).
If retval == (e.g.) -1048576, the caller of do_execve() will get undefined
error code.
If retval > 0 (not true for my case but could be true for architectures where
TASK_SIZE is smaller than INT_MAX), the caller of do_execve() may or may not
assume that do_execve() reached "the point of no return".
In any cases, the current task will be killed by send_sig(SIGKILL, current, 0)
but I'm not sure that resources are correctly handled.
Regards.
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: Is it legal to return positive value when do_execve() succeeds?
2010-09-30 13:07 Is it legal to return positive value when do_execve() succeeds? Tetsuo Handa
2010-10-01 2:29 ` Tetsuo Handa
@ 2010-10-01 12:44 ` Tetsuo Handa
1 sibling, 0 replies; 13+ messages in thread
From: Tetsuo Handa @ 2010-10-01 12:44 UTC (permalink / raw)
To: linux-fsdevel; +Cc: linux-kernel
> --- a/fs/exec.c
> +++ b/fs/exec.c
> @@ -1413,6 +1413,9 @@ int do_execve(const char * filename,
> free_bprm(bprm);
> if (displaced)
> put_files_struct(displaced);
> + /* Just for testing. */
> + if (!retval)
> + retval = 1;
> return retval;
>
> out:
>
> With a patch above (on x86 CentOS 5.5 with 2.6.36-rc6 kernel),
> a few programs failed to work properly.
>
> udevd-event[$PID]: run_program: '$PROGRAM' abnormal exit
>
I found below messages in dmesg when above error message is printed.
[ 16.082683] usb_id[2097]: segfault at dfa68ce8 ip 0806ffb7 sp bf892b30 error 5 in usb_id[8048000+cd000]
[ 16.732016] scsi_id[2133]: segfault at dfa37e78 ip 08071157 sp bf85d640 error 5 in scsi_id[8048000+cf000]
[ 16.754163] scsi_id[2138]: segfault at e002ca08 ip 08071157 sp bfe521d0 error 4 in scsi_id[8048000+cf000]
[ 16.910293] scsi_id[2162]: segfault at e008def8 ip 08071157 sp bfeb36c0 error 4 in scsi_id[8048000+cf000]
[ 16.971028] scsi_id[2167]: segfault at dff9f908 ip 08071157 sp bfdc50d0 error 5 in scsi_id[8048000+cf000]
[ 17.435741] vol_id[2177]: segfault at e017d258 ip 08072bb7 sp bff9c0a0 error 4 in vol_id[8048000+d0000]
[ 17.497793] vol_id[2179]: segfault at dfc82ab8 ip 08072bb7 sp bfaa1900 error 5 in vol_id[8048000+d0000]
[ 17.676152] edd_id[2197]: segfault at dff0de58 ip 0806f097 sp bfd3b920 error 5 in edd_id[8048000+ca000]
[ 17.678064] edd_id[2196]: segfault at dff55a38 ip 0806f097 sp bfd83500 error 5 in edd_id[8048000+ca000]
[ 18.317277] vol_id[2210]: segfault at e00c8a48 ip 08072bb7 sp bfee7890 error 4 in vol_id[8048000+d0000]
And I confirmed that applying below patch in addition to above patch solves
these segfault failures.
--- a/arch/x86/kernel/process.c
+++ b/arch/x86/kernel/process.c
@@ -315,7 +315,7 @@ long sys_execve(const char __user *name,
error = do_execve(filename, argv, envp, regs);
#ifdef CONFIG_X86_32
- if (error == 0) {
+ if (error >= 0) {
/* Make sure we don't return using sysenter.. */
set_thread_flag(TIF_IRET);
}
This result indicates that sys_execve() expected that do_execve() returns 0
if do_execve() successfully replaced the current process's image.
Now, the question is "when is do_execve() > 0 allowed"?
If do_execve() > 0 is allowed when the current process's image was successfully
replaced, we need to either "modify do_execve() callers not to assume 0 on
success" or "modify do_execve() return 0 on success".
Regards.
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: Is it legal to return positive value when do_execve() succeeds?
2010-10-01 5:19 ` Tetsuo Handa
@ 2010-10-04 4:59 ` KOSAKI Motohiro
2010-10-04 13:31 ` Tetsuo Handa
0 siblings, 1 reply; 13+ messages in thread
From: KOSAKI Motohiro @ 2010-10-04 4:59 UTC (permalink / raw)
To: Tetsuo Handa; +Cc: kosaki.motohiro, linux-fsdevel, linux-kernel
> Thus, TASK_SIZE == 0xC0000000.
>
> fs/binfmt_elf.c:
> 77 #define BAD_ADDR(x) ((unsigned long)(x) >= TASK_SIZE)
> 78
> 79 static int set_brk(unsigned long start, unsigned long end)
> 80 {
> 81 start = ELF_PAGEALIGN(start);
> 82 end = ELF_PAGEALIGN(end);
> 83 if (end > start) {
> 84 unsigned long addr;
> 85 down_write(¤t->mm->mmap_sem);
> 86 addr = do_brk(start, end - start);
> 87 up_write(¤t->mm->mmap_sem);
> 88 if (BAD_ADDR(addr))
> 89 return addr;
> 90 }
> 91 current->mm->start_brk = current->mm->brk = end;
> 92 return 0;
> 93 }
>
> Thus, BAD_ADDR(x) is ((unsigned long)(x) >= 0xC0000000).
Can do_brk() return BAD_ADDR() _and_ !IS_ERR_VALUE() value? when?
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: Is it legal to return positive value when do_execve() succeeds?
2010-10-04 4:59 ` KOSAKI Motohiro
@ 2010-10-04 13:31 ` Tetsuo Handa
2010-10-05 1:28 ` KOSAKI Motohiro
0 siblings, 1 reply; 13+ messages in thread
From: Tetsuo Handa @ 2010-10-04 13:31 UTC (permalink / raw)
To: kosaki.motohiro; +Cc: linux-fsdevel, linux-kernel
KOSAKI Motohiro wrote:
> > fs/binfmt_elf.c:
> > 77 #define BAD_ADDR(x) ((unsigned long)(x) >= TASK_SIZE)
> Can do_brk() return BAD_ADDR() _and_ !IS_ERR_VALUE() value? when?
For example, arch/mips/include/asm/processor.h has below definitions
which is smaller than INT_MAX
47 #ifdef CONFIG_32BIT
(...snipped...)
52 #define TASK_SIZE 0x7fff8000UL
(...snipped...)
63 #endif
64
65 #ifdef CONFIG_64BIT
(...snipped...)
74 #define TASK_SIZE 0x10000000000UL
(...snipped...)
91 #endif
Also, several architectures define TASK_SIZE as
#define TASK_SIZE PAGE_OFFSET
and PAGE_OFFSET could be 0 if CONFIG_KERNEL_RAM_BASE_ADDRESS is not defined.
include/asm-generic/page.h
68 #ifdef CONFIG_KERNEL_RAM_BASE_ADDRESS
69 #define PAGE_OFFSET (CONFIG_KERNEL_RAM_BASE_ADDRESS)
70 #else
71 #define PAGE_OFFSET (0)
72 #endif
If TASK_SIZE == 0, BAD_ADDR(x) is always true and !IS_ERR_VALUE(x) can be true.
Although I don't know which combination makes such environment,
I think "BAD_ADDR() _and_ !IS_ERR_VALUE()" can happen.
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: Is it legal to return positive value when do_execve() succeeds?
2010-10-04 13:31 ` Tetsuo Handa
@ 2010-10-05 1:28 ` KOSAKI Motohiro
2010-10-05 2:24 ` Tetsuo Handa
2010-10-05 2:37 ` KOSAKI Motohiro
0 siblings, 2 replies; 13+ messages in thread
From: KOSAKI Motohiro @ 2010-10-05 1:28 UTC (permalink / raw)
To: Tetsuo Handa; +Cc: kosaki.motohiro, linux-fsdevel, linux-kernel
> KOSAKI Motohiro wrote:
> > > fs/binfmt_elf.c:
> > > 77 #define BAD_ADDR(x) ((unsigned long)(x) >= TASK_SIZE)
> > Can do_brk() return BAD_ADDR() _and_ !IS_ERR_VALUE() value? when?
>
> For example, arch/mips/include/asm/processor.h has below definitions
> which is smaller than INT_MAX
>
> 47 #ifdef CONFIG_32BIT
> (...snipped...)
> 52 #define TASK_SIZE 0x7fff8000UL
> (...snipped...)
> 63 #endif
> 64
> 65 #ifdef CONFIG_64BIT
> (...snipped...)
> 74 #define TASK_SIZE 0x10000000000UL
> (...snipped...)
> 91 #endif
>
> Also, several architectures define TASK_SIZE as
>
> #define TASK_SIZE PAGE_OFFSET
>
> and PAGE_OFFSET could be 0 if CONFIG_KERNEL_RAM_BASE_ADDRESS is not defined.
>
> include/asm-generic/page.h
> 68 #ifdef CONFIG_KERNEL_RAM_BASE_ADDRESS
> 69 #define PAGE_OFFSET (CONFIG_KERNEL_RAM_BASE_ADDRESS)
> 70 #else
> 71 #define PAGE_OFFSET (0)
> 72 #endif
>
> If TASK_SIZE == 0, BAD_ADDR(x) is always true and !IS_ERR_VALUE(x) can be true.
> Although I don't know which combination makes such environment,
> I think "BAD_ADDR() _and_ !IS_ERR_VALUE()" can happen.
I think you should read do_brk() itself. the spec is
success case:
return addr argument
error case:
return error code
When does it return invalid address?
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: Is it legal to return positive value when do_execve() succeeds?
2010-10-05 1:28 ` KOSAKI Motohiro
@ 2010-10-05 2:24 ` Tetsuo Handa
2010-10-05 5:21 ` KOSAKI Motohiro
2010-10-05 2:37 ` KOSAKI Motohiro
1 sibling, 1 reply; 13+ messages in thread
From: Tetsuo Handa @ 2010-10-05 2:24 UTC (permalink / raw)
To: kosaki.motohiro; +Cc: linux-fsdevel, linux-kernel
KOSAKI Motohiro wrote:
> I think you should read do_brk() itself. the spec is
>
> success case:
> return addr argument
>
> error case:
> return error code
>
> When does it return invalid address?
If addr argument is bogus.
load_elf_binary() in fs/binfmt_elf.c
758 if (unlikely (elf_brk > elf_bss)) {
759 unsigned long nbyte;
760
761 /* There was a PT_LOAD segment with p_memsz > p_filesz
762 before this one. Map anonymous pages, if needed,
763 and clear the area. */
764 retval = set_brk (elf_bss + load_bias,
765 elf_brk + load_bias);
766 if (retval) {
767 send_sig(SIGKILL, current, 0);
I think this function is using address information from untrusted source
(executable file). Thus, I think retval could be
ELF_PAGEALIGN(elf_bss + load_bias) if elf_bss + load_bias is bogus.
I'm OK with this if it is guaranteed that elf_bss + load_bias is always valid
and set_brk() never returns ELF_PAGEALIGN(elf_bss + load_bias).
768 goto out_free_dentry;
769 }
(...snipped...)
835 /*
836 * Check to see if the section's size will overflow the
837 * allowed task size. Note that p_filesz must always be
838 * <= p_memsz so it is only necessary to check p_memsz.
839 */
840 if (BAD_ADDR(k) || elf_ppnt->p_filesz > elf_ppnt->p_memsz ||
841 elf_ppnt->p_memsz > TASK_SIZE ||
842 TASK_SIZE - elf_ppnt->p_memsz < k) {
843 /* set_brk can never work. Avoid overflows. */
844 send_sig(SIGKILL, current, 0);
845 retval = -EINVAL;
Here setting -EINVAL. So why not to set a valid error code for set_brk() above?
846 goto out_free_dentry;
847 }
848
849 k = elf_ppnt->p_vaddr + elf_ppnt->p_filesz;
850
851 if (k > elf_bss)
852 elf_bss = k;
853 if ((elf_ppnt->p_flags & PF_X) && end_code < k)
854 end_code = k;
855 if (end_data < k)
856 end_data = k;
857 k = elf_ppnt->p_vaddr + elf_ppnt->p_memsz;
858 if (k > elf_brk)
859 elf_brk = k;
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: Is it legal to return positive value when do_execve() succeeds?
2010-10-05 1:28 ` KOSAKI Motohiro
2010-10-05 2:24 ` Tetsuo Handa
@ 2010-10-05 2:37 ` KOSAKI Motohiro
2010-10-05 2:55 ` KOSAKI Motohiro
1 sibling, 1 reply; 13+ messages in thread
From: KOSAKI Motohiro @ 2010-10-05 2:37 UTC (permalink / raw)
To: Tetsuo Handa; +Cc: kosaki.motohiro, linux-fsdevel, linux-kernel
> > KOSAKI Motohiro wrote:
> > > > fs/binfmt_elf.c:
> > > > 77 #define BAD_ADDR(x) ((unsigned long)(x) >= TASK_SIZE)
> > > Can do_brk() return BAD_ADDR() _and_ !IS_ERR_VALUE() value? when?
> >
> > For example, arch/mips/include/asm/processor.h has below definitions
> > which is smaller than INT_MAX
> >
> > 47 #ifdef CONFIG_32BIT
> > (...snipped...)
> > 52 #define TASK_SIZE 0x7fff8000UL
> > (...snipped...)
> > 63 #endif
> > 64
> > 65 #ifdef CONFIG_64BIT
> > (...snipped...)
> > 74 #define TASK_SIZE 0x10000000000UL
> > (...snipped...)
> > 91 #endif
> >
> > Also, several architectures define TASK_SIZE as
> >
> > #define TASK_SIZE PAGE_OFFSET
> >
> > and PAGE_OFFSET could be 0 if CONFIG_KERNEL_RAM_BASE_ADDRESS is not defined.
> >
> > include/asm-generic/page.h
> > 68 #ifdef CONFIG_KERNEL_RAM_BASE_ADDRESS
> > 69 #define PAGE_OFFSET (CONFIG_KERNEL_RAM_BASE_ADDRESS)
> > 70 #else
> > 71 #define PAGE_OFFSET (0)
> > 72 #endif
> >
> > If TASK_SIZE == 0, BAD_ADDR(x) is always true and !IS_ERR_VALUE(x) can be true.
> > Although I don't know which combination makes such environment,
> > I think "BAD_ADDR() _and_ !IS_ERR_VALUE()" can happen.
>
> I think you should read do_brk() itself. the spec is
>
> success case:
> return addr argument
>
> error case:
> return error code
>
> When does it return invalid address?
Does this makes a bit cleanups?
>From 5f5556d30ac1876ec2211a2eae77e8372183a9b1 Mon Sep 17 00:00:00 2001
From: KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com>
Date: Fri, 15 Oct 2010 05:13:40 +0900
Subject: [cleanup][PATCH] elf: kill BAD_ADDR() macro
BAD_ADDR() macro is useless because 1) do_brk() and do_mmap() return
only either valid pointer or error code 2) when kernel and userland have
perfectly different address space (such as old 4G:4G separation), to
compare TASK_SIZE has no good meaning.
Then, this patch change it to use IS_ERR_VALUE instead (as other a lot
of places).
But, this is theorical issue. this patch doesn't have functional change.
Signed-off-by: KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com>
Cc: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
---
fs/binfmt_elf.c | 6 ++----
1 files changed, 2 insertions(+), 4 deletions(-)
diff --git a/fs/binfmt_elf.c b/fs/binfmt_elf.c
index 535e763..ee235dd 100644
--- a/fs/binfmt_elf.c
+++ b/fs/binfmt_elf.c
@@ -74,8 +74,6 @@ static struct linux_binfmt elf_format = {
.hasvdso = 1
};
-#define BAD_ADDR(x) ((unsigned long)(x) >= TASK_SIZE)
-
static int set_brk(unsigned long start, unsigned long end)
{
start = ELF_PAGEALIGN(start);
@@ -85,7 +83,7 @@ static int set_brk(unsigned long start, unsigned long end)
down_write(¤t->mm->mmap_sem);
addr = do_brk(start, end - start);
up_write(¤t->mm->mmap_sem);
- if (BAD_ADDR(addr))
+ if (IS_ERR_VALUE(addr))
return addr;
}
current->mm->start_brk = current->mm->brk = end;
@@ -345,7 +343,7 @@ static unsigned long elf_map(struct file *filep, unsigned long addr,
if (total_size) {
total_size = ELF_PAGEALIGN(total_size);
map_addr = do_mmap(filep, addr, total_size, prot, type, off);
- if (!BAD_ADDR(map_addr))
+ if (!IS_ERR_VALUE(map_addr))
do_munmap(current->mm, map_addr+size, total_size-size);
} else
map_addr = do_mmap(filep, addr, size, prot, type, off);
--
1.6.5.2
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: Is it legal to return positive value when do_execve() succeeds?
2010-10-05 2:37 ` KOSAKI Motohiro
@ 2010-10-05 2:55 ` KOSAKI Motohiro
2010-10-05 4:40 ` Tetsuo Handa
0 siblings, 1 reply; 13+ messages in thread
From: KOSAKI Motohiro @ 2010-10-05 2:55 UTC (permalink / raw)
To: KOSAKI Motohiro
Cc: kosaki.motohiro, Tetsuo Handa, linux-fsdevel, linux-kernel
> > > KOSAKI Motohiro wrote:
> > > > > fs/binfmt_elf.c:
> > > > > 77 #define BAD_ADDR(x) ((unsigned long)(x) >= TASK_SIZE)
> > > > Can do_brk() return BAD_ADDR() _and_ !IS_ERR_VALUE() value? when?
> > >
> > > For example, arch/mips/include/asm/processor.h has below definitions
> > > which is smaller than INT_MAX
> > >
> > > 47 #ifdef CONFIG_32BIT
> > > (...snipped...)
> > > 52 #define TASK_SIZE 0x7fff8000UL
> > > (...snipped...)
> > > 63 #endif
> > > 64
> > > 65 #ifdef CONFIG_64BIT
> > > (...snipped...)
> > > 74 #define TASK_SIZE 0x10000000000UL
> > > (...snipped...)
> > > 91 #endif
> > >
> > > Also, several architectures define TASK_SIZE as
> > >
> > > #define TASK_SIZE PAGE_OFFSET
> > >
> > > and PAGE_OFFSET could be 0 if CONFIG_KERNEL_RAM_BASE_ADDRESS is not defined.
> > >
> > > include/asm-generic/page.h
> > > 68 #ifdef CONFIG_KERNEL_RAM_BASE_ADDRESS
> > > 69 #define PAGE_OFFSET (CONFIG_KERNEL_RAM_BASE_ADDRESS)
> > > 70 #else
> > > 71 #define PAGE_OFFSET (0)
> > > 72 #endif
> > >
> > > If TASK_SIZE == 0, BAD_ADDR(x) is always true and !IS_ERR_VALUE(x) can be true.
> > > Although I don't know which combination makes such environment,
> > > I think "BAD_ADDR() _and_ !IS_ERR_VALUE()" can happen.
> >
> > I think you should read do_brk() itself. the spec is
> >
> > success case:
> > return addr argument
> >
> > error case:
> > return error code
> >
> > When does it return invalid address?
>
> Does this makes a bit cleanups?
>
>
>
> From 5f5556d30ac1876ec2211a2eae77e8372183a9b1 Mon Sep 17 00:00:00 2001
> From: KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com>
> Date: Fri, 15 Oct 2010 05:13:40 +0900
> Subject: [cleanup][PATCH] elf: kill BAD_ADDR() macro
>
> BAD_ADDR() macro is useless because 1) do_brk() and do_mmap() return
> only either valid pointer or error code 2) when kernel and userland have
> perfectly different address space (such as old 4G:4G separation), to
> compare TASK_SIZE has no good meaning.
>
> Then, this patch change it to use IS_ERR_VALUE instead (as other a lot
> of places).
> But, this is theorical issue. this patch doesn't have functional change.
Ouch, this patch is completely corrupted. please ignore it.
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: Is it legal to return positive value when do_execve() succeeds?
2010-10-05 2:55 ` KOSAKI Motohiro
@ 2010-10-05 4:40 ` Tetsuo Handa
0 siblings, 0 replies; 13+ messages in thread
From: Tetsuo Handa @ 2010-10-05 4:40 UTC (permalink / raw)
To: kosaki.motohiro; +Cc: linux-fsdevel, linux-kernel
KOSAKI Motohiro wrote:
> > BAD_ADDR() macro is useless because 1) do_brk() and do_mmap() return
> > only either valid pointer or error code 2) when kernel and userland have
> > perfectly different address space (such as old 4G:4G separation), to
> > compare TASK_SIZE has no good meaning.
> >
> > Then, this patch change it to use IS_ERR_VALUE instead (as other a lot
> > of places).
Using IS_ERR_VALUE() makes sense for me.
> Ouch, this patch is completely corrupted. please ignore it.
No problem.
I just browsed fs/binfmt_*.c for similar cases.
Line number is as of 2.6.36-rc6 .
fs/binfmt_aout.c
46 #define BAD_ADDR(x) ((unsigned long)(x) >= TASK_SIZE)
47
48 static int set_brk(unsigned long start, unsigned long end)
49 {
50 start = PAGE_ALIGN(start);
51 end = PAGE_ALIGN(end);
52 if (end > start) {
53 unsigned long addr;
54 down_write(¤t->mm->mmap_sem);
55 addr = do_brk(start, end - start);
56 up_write(¤t->mm->mmap_sem);
57 if (BAD_ADDR(addr))
58 return addr;
59 }
60 return 0;
61 }
Should be "if (IS_ERR_VALUE(error))" as well?
282 down_write(¤t->mm->mmap_sem);
283 error = do_brk(text_addr & PAGE_MASK, map_size);
284 up_write(¤t->mm->mmap_sem);
285 if (error != (text_addr & PAGE_MASK)) {
Should be "if (IS_ERR_VALUE(error))" as well?
286 send_sig(SIGKILL, current, 0);
287 return error;
288 }
327 down_write(¤t->mm->mmap_sem);
328 error = do_mmap(bprm->file, N_TXTADDR(ex), ex.a_text,
329 PROT_READ | PROT_EXEC,
330 MAP_FIXED | MAP_PRIVATE | MAP_DENYWRITE | MAP_EXECUTABLE,
331 fd_offset);
332 up_write(¤t->mm->mmap_sem);
333
334 if (error != N_TXTADDR(ex)) {
Should be "if (IS_ERR_VALUE(error))" as well?
335 send_sig(SIGKILL, current, 0);
336 return error;
337 }
338
339 down_write(¤t->mm->mmap_sem);
340 error = do_mmap(bprm->file, N_DATADDR(ex), ex.a_data,
341 PROT_READ | PROT_WRITE | PROT_EXEC,
342 MAP_FIXED | MAP_PRIVATE | MAP_DENYWRITE | MAP_EXECUTABLE,
343 fd_offset + ex.a_text);
344 up_write(¤t->mm->mmap_sem);
345 if (error != N_DATADDR(ex)) {
Should be "if (IS_ERR_VALUE(error))" as well?
346 send_sig(SIGKILL, current, 0);
347 return error;
348 }
fs/binfmt_elf_fdpic.c
416 install_exec_creds(bprm);
417 current->flags &= ~PF_FORKNOEXEC;
418 if (create_elf_fdpic_tables(bprm, current->mm,
419 &exec_params, &interp_params) < 0)
420 goto error_kill;
421
Why retval == 0 here and retval < 0 elsewhere in this function when
the current process is to be killed?
459
460 /* unrecoverable error - kill the process */
461 error_kill:
462 send_sig(SIGSEGV, current, 0);
Why not to SIGKILL rather than SIGSEGV?
463 goto error;
464
465 }
fs/binfmt_flat.c
280 while ((ret = zlib_inflate(&strm, Z_NO_FLUSH)) == Z_OK) {
281 ret = bprm->file->f_op->read(bprm->file, buf, LBUFSIZE, &fpos);
282 if (ret <= 0)
283 break;
284 len -= ret;
285
286 strm.next_in = buf;
287 strm.avail_in = ret;
288 strm.total_in = 0;
289 }
290
291 if (ret < 0) {
292 DBG_FLT("binfmt_flat: decompression failed (%d), %s\n",
293 ret, strm.msg);
294 goto out_zlib;
295 }
296
297 retval = 0;
zlib_inflate() may return positive return code.
Is it OK to ignore partial inflation?
372
373 failed:
374 printk(", killing %s!\n", current->comm);
375 send_sig(SIGSEGV, current, 0);
Why not to SIGKILL rather than SIGSEGV?
376
377 return RELOC_FAILED;
378 }
582 #ifdef CONFIG_BINFMT_ZFLAT
583 if (flags & FLAT_FLAG_GZDATA) {
584 result = decompress_exec(bprm, fpos, (char *) datapos,
585 data_len + (relocs * sizeof(unsigned long)), 0);
586 } else
587 #endif
588 {
589 result = bprm->file->f_op->read(bprm->file, (char *) datapos,
590 data_len + (relocs * sizeof(unsigned long)), &fpos);
591 }
592 if (IS_ERR_VALUE(result)) {
Is it OK to ignore partial read/inflation?
634 if (flags & FLAT_FLAG_GZIP) {
635 result = decompress_exec(bprm, sizeof (struct flat_hdr),
636 (((char *) textpos) + sizeof (struct flat_hdr)),
637 (text_len + data_len + (relocs * sizeof(unsigned long))
638 - sizeof (struct flat_hdr)),
639 0);
Why not to check inflation failure before memmove()?
640 memmove((void *) datapos, (void *) realdatastart,
641 data_len + (relocs * sizeof(unsigned long)));
642 } else if (flags & FLAT_FLAG_GZDATA) {
643 fpos = 0;
644 result = bprm->file->f_op->read(bprm->file,
645 (char *) textpos, text_len, &fpos);
Why not to check partial read?
646 if (!IS_ERR_VALUE(result))
647 result = decompress_exec(bprm, text_len, (char *) datapos,
648 data_len + (relocs * sizeof(unsigned long)), 0);
649 }
650 else
651 #endif
652 {
653 fpos = 0;
654 result = bprm->file->f_op->read(bprm->file,
655 (char *) textpos, text_len, &fpos);
Why not to check partial read?
656 if (!IS_ERR_VALUE(result)) {
657 fpos = ntohl(hdr->data_start);
658 result = bprm->file->f_op->read(bprm->file, (char *) datapos,
659 data_len + (relocs * sizeof(unsigned long)), &fpos);
660 }
661 }
Why not to check partial read/inflation?
662 if (IS_ERR_VALUE(result)) {
663 printk("Unable to read code+data+bss, errno %d\n",(int)-result);
664 do_munmap(current->mm, textpos, text_len + data_len + extra +
665 MAX_SHARED_LIBS * sizeof(unsigned long));
666 ret = result;
667 goto err;
668 }
fs/binfmt_som.c
150 down_write(¤t->mm->mmap_sem);
151 retval = do_mmap(file, code_start, code_size, prot,
152 flags, SOM_PAGESTART(hpuxhdr->exec_tfile));
153 up_write(¤t->mm->mmap_sem);
154 if (retval < 0 && retval > -1024)
155 goto out;
What is 1024? Why not to IS_ERR_VALUE()?
By the way, I though it would be nice to have
do_mmap_current()
do_brk_current()
because there are a lot of
down_write(¤t->mm->mmap_sem);
error = do_mmap();
up_write(¤t->mm->mmap_sem);
down_write(¤t->mm->mmap_sem);
error = do_brk();
up_write(¤t->mm->mmap_sem);
repetition.
Also, it would be nice to have
bool kill_self_if_error(int error)
{
if (!IS_ERR_VALUE(error))
return false;
send_sig(SIGKILL, current, 0);
return true;
}
for avoiding
send_sig(SIGKILL, current, 0);
many times.
Regards.
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: Is it legal to return positive value when do_execve() succeeds?
2010-10-05 2:24 ` Tetsuo Handa
@ 2010-10-05 5:21 ` KOSAKI Motohiro
2010-10-05 6:48 ` Tetsuo Handa
0 siblings, 1 reply; 13+ messages in thread
From: KOSAKI Motohiro @ 2010-10-05 5:21 UTC (permalink / raw)
To: Tetsuo Handa; +Cc: kosaki.motohiro, linux-fsdevel, linux-kernel
> KOSAKI Motohiro wrote:
> > I think you should read do_brk() itself. the spec is
> >
> > success case:
> > return addr argument
> >
> > error case:
> > return error code
> >
> > When does it return invalid address?
>
> If addr argument is bogus.
>
>
>
> load_elf_binary() in fs/binfmt_elf.c
> 758 if (unlikely (elf_brk > elf_bss)) {
> 759 unsigned long nbyte;
> 760
> 761 /* There was a PT_LOAD segment with p_memsz > p_filesz
> 762 before this one. Map anonymous pages, if needed,
> 763 and clear the area. */
> 764 retval = set_brk (elf_bss + load_bias,
> 765 elf_brk + load_bias);
> 766 if (retval) {
> 767 send_sig(SIGKILL, current, 0);
>
> I think this function is using address information from untrusted source
> (executable file).
True.
> Thus, I think retval could be
> ELF_PAGEALIGN(elf_bss + load_bias) if elf_bss + load_bias is bogus.
What's mean bogus?
do_brk() call get_unmapped_area() and it check an argument is correctly
unmapped and userland address. If elf_bss + load_bias point to invalid
address, set_brk doesn't return elf_bss+load_bias.
> I'm OK with this if it is guaranteed that elf_bss + load_bias is always valid
> and set_brk() never returns ELF_PAGEALIGN(elf_bss + load_bias).
I think elf_bss + load_bias could be invalid (i.e. >TASK_SIZE).
but set_brk can detect it.
-------------------------------------------------------------------------
unsigned long
get_unmapped_area(struct file *file, unsigned long addr, unsigned long len,
unsigned long pgoff, unsigned long flags)
{
unsigned long (*get_area)(struct file *, unsigned long,
unsigned long, unsigned long, unsigned long);
unsigned long error = arch_mmap_check(addr, len, flags);
if (error)
return error;
/* Careful about overflows.. */
if (len > TASK_SIZE)
return -ENOMEM;
get_area = current->mm->get_unmapped_area;
if (file && file->f_op && file->f_op->get_unmapped_area)
get_area = file->f_op->get_unmapped_area;
addr = get_area(file, addr, len, pgoff, flags);
if (IS_ERR_VALUE(addr))
return addr;
if (addr > TASK_SIZE - len) // HERE
return -ENOMEM;
if (addr & ~PAGE_MASK)
return -EINVAL;
return arch_rebalance_pgtables(addr, len);
}
-------------------------------------------------------------------------
>
> 768 goto out_free_dentry;
> 769 }
> (...snipped...)
> 835 /*
> 836 * Check to see if the section's size will overflow the
> 837 * allowed task size. Note that p_filesz must always be
> 838 * <= p_memsz so it is only necessary to check p_memsz.
> 839 */
> 840 if (BAD_ADDR(k) || elf_ppnt->p_filesz > elf_ppnt->p_memsz ||
> 841 elf_ppnt->p_memsz > TASK_SIZE ||
> 842 TASK_SIZE - elf_ppnt->p_memsz < k) {
> 843 /* set_brk can never work. Avoid overflows. */
> 844 send_sig(SIGKILL, current, 0);
> 845 retval = -EINVAL;
>
> Here setting -EINVAL. So why not to set a valid error code for set_brk() above?
I don't know. perhaps I am missing anything :)
Please double check.
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: Is it legal to return positive value when do_execve() succeeds?
2010-10-05 5:21 ` KOSAKI Motohiro
@ 2010-10-05 6:48 ` Tetsuo Handa
0 siblings, 0 replies; 13+ messages in thread
From: Tetsuo Handa @ 2010-10-05 6:48 UTC (permalink / raw)
To: kosaki.motohiro; +Cc: linux-fsdevel, linux-kernel
KOSAKI Motohiro wrote:
> > Thus, I think retval could be
> > ELF_PAGEALIGN(elf_bss + load_bias) if elf_bss + load_bias is bogus.
>
> What's mean bogus?
> do_brk() call get_unmapped_area() and it check an argument is correctly
> unmapped and userland address. If elf_bss + load_bias point to invalid
> address, set_brk doesn't return elf_bss+load_bias.
>
>
> > I'm OK with this if it is guaranteed that elf_bss + load_bias is always valid
> > and set_brk() never returns ELF_PAGEALIGN(elf_bss + load_bias).
>
> I think elf_bss + load_bias could be invalid (i.e. >TASK_SIZE).
> but set_brk can detect it.
>
Indeed. Then, we can replace BAD_ADDR() with IS_ERR_VALUE() as you proposed.
Well... who returns positive value when do_execve() succeeds? Nobody?
Then, I wonder why search_binary_handler() is designed to return positive value...
^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2010-10-05 6:48 UTC | newest]
Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2010-09-30 13:07 Is it legal to return positive value when do_execve() succeeds? Tetsuo Handa
2010-10-01 2:29 ` Tetsuo Handa
2010-10-01 5:19 ` Tetsuo Handa
2010-10-04 4:59 ` KOSAKI Motohiro
2010-10-04 13:31 ` Tetsuo Handa
2010-10-05 1:28 ` KOSAKI Motohiro
2010-10-05 2:24 ` Tetsuo Handa
2010-10-05 5:21 ` KOSAKI Motohiro
2010-10-05 6:48 ` Tetsuo Handa
2010-10-05 2:37 ` KOSAKI Motohiro
2010-10-05 2:55 ` KOSAKI Motohiro
2010-10-05 4:40 ` Tetsuo Handa
2010-10-01 12:44 ` Tetsuo Handa
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox
Powered by JetHome