Is it legal to return positive value when do_execve() succeeds?

Is it legal to return positive value when do_execve() succeeds?

[Tetsuo Handa]

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.

Re: Is it legal to return positive value when do_execve() succeeds?

[Tetsuo Handa]

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.

Re: Is it legal to return positive value when do_execve() succeeds?

[Tetsuo Handa]

> 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(&current->mm->mmap_sem);
 86                 addr = do_brk(start, end - start);
 87                 up_write(&current->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.

Re: Is it legal to return positive value when do_execve() succeeds?

[KOSAKI Motohiro]

> 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(&current->mm->mmap_sem);
>  86                 addr = do_brk(start, end - start);
>  87                 up_write(&current->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?

Re: Is it legal to return positive value when do_execve() succeeds?

[Tetsuo Handa]

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.

Re: Is it legal to return positive value when do_execve() succeeds?

[KOSAKI Motohiro]

> 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?

Re: Is it legal to return positive value when do_execve() succeeds?

[Tetsuo Handa]

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;

Re: Is it legal to return positive value when do_execve() succeeds?

[KOSAKI Motohiro]

> 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.

Re: Is it legal to return positive value when do_execve() succeeds?

[Tetsuo Handa]

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...

Re: Is it legal to return positive value when do_execve() succeeds?

[KOSAKI Motohiro]

> > 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(&current->mm->mmap_sem);
 		addr = do_brk(start, end - start);
 		up_write(&current->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

Re: Is it legal to return positive value when do_execve() succeeds?

[KOSAKI Motohiro]

> > > 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.

Re: Is it legal to return positive value when do_execve() succeeds?

[Tetsuo Handa]

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(&current->mm->mmap_sem);
 55                 addr = do_brk(start, end - start);
 56                 up_write(&current->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(&current->mm->mmap_sem);
283                 error = do_brk(text_addr & PAGE_MASK, map_size);
284                 up_write(&current->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(&current->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(&current->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(&current->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(&current->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(&current->mm->mmap_sem);
151         retval = do_mmap(file, code_start, code_size, prot,
152                         flags, SOM_PAGESTART(hpuxhdr->exec_tfile));
153         up_write(&current->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(&current->mm->mmap_sem);
  error = do_mmap();
  up_write(&current->mm->mmap_sem);

  down_write(&current->mm->mmap_sem);
  error = do_brk();
  up_write(&current->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.

Re: Is it legal to return positive value when do_execve() succeeds?

[Tetsuo Handa]

> --- 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.