[PATCH] kill the last few warnings in binfmt_elf.c

[PATCH] kill the last few warnings in binfmt_elf.c

[Jesper Juhl]

Hi Andrew,

Sorry to keep bugging you with this directly, but I'm still not aware of a 
sepperate maintainer for this file.


I've send you 3 patches previously that kill a few warnings in 
fs/binfmt_elf.c. The patch below includes those previous 3 and also adds a 
fix for the last ones.

The patch does the following :

- kill the unused result warning in fill_psinfo - yes, I know you consider 
this a silly way to shut up gcc, so feel free to replace it with a dummy 
assignment or (void) if you please - but at least this way is safe and 
doesn't add an extra variable or anything. (this you've already recieved 
as a sepperate patch)

- kill the unused result warning in padzero and make padzero return the nr 
of bytes not cleared and update callers of padzero to catch return of != 0 
and act accordingly. (this you've already recieved as a sepperate patch)

- kill the unused result warning in load_elf_binary and do proper error 
handling. (this you've already recieved as a sepperate patch)

- kill the unused result warning in create_elf_tables and update caller to 
check for error condition. This is the new bit in this patch that you 
haven't recieved in its current form before. I'm not 100% sure that
send_sig(SIGKILL, current, 0);
goto out_free_dentry;
is the correct thing to do in the create_elf_tables caller if 
create_elf_tables fails - I think it is, but having someone confirm it 
would be nice.

Patch has been compile tested and compiles without errors or warnings.
Patch has been test booted and is in the kernel I'm currently running.
Patch is created against 2.6.10-rc1-bk12

Please review and consider for inclusion.

Signed-off-by: Jesper Juhl <juhl-lkml@dif.dk>

diff -up linux-2.6.10-rc1-bk12-orig/fs/binfmt_elf.c linux-2.6.10-rc1-bk12/fs/binfmt_elf.c
--- linux-2.6.10-rc1-bk12-orig/fs/binfmt_elf.c	2004-10-26 00:21:01.000000000 +0200
+++ linux-2.6.10-rc1-bk12/fs/binfmt_elf.c	2004-11-03 00:06:25.000000000 +0100
@@ -103,15 +103,18 @@ static int set_brk(unsigned long start, 
    be in memory */
 
 
-static void padzero(unsigned long elf_bss)
+static int padzero(unsigned long elf_bss)
 {
 	unsigned long nbyte;
+	int ret = 0;
 
 	nbyte = ELF_PAGEOFFSET(elf_bss);
 	if (nbyte) {
 		nbyte = ELF_MIN_ALIGN - nbyte;
-		clear_user((void __user *) elf_bss, nbyte);
+		ret = clear_user((void __user *) elf_bss, nbyte);
 	}
+	
+	return ret;
 }
 
 /* Let's use some macros to make this stack manipulation a litle clearer */
@@ -127,7 +130,7 @@ static void padzero(unsigned long elf_bs
 #define STACK_ALLOC(sp, len) ({ sp -= len ; sp; })
 #endif
 
-static void
+static int
 create_elf_tables(struct linux_binprm *bprm, struct elfhdr * exec,
 		int interp_aout, unsigned long load_addr,
 		unsigned long interp_load_addr)
@@ -144,6 +147,7 @@ create_elf_tables(struct linux_binprm *b
 	elf_addr_t *elf_info;
 	int ei_index = 0;
 	struct task_struct *tsk = current;
+	int retval = 0;
 
 	/*
 	 * If this architecture has a platform capability string, copy it
@@ -172,7 +176,10 @@ create_elf_tables(struct linux_binprm *b
 			STACK_ALLOC(p, ((current->pid % 64) << 7));
 #endif
 		u_platform = (elf_addr_t __user *)STACK_ALLOC(p, len);
-		__copy_to_user(u_platform, k_platform, len);
+		if (__copy_to_user(u_platform, k_platform, len)) {
+			retval = -EFAULT;
+			goto out;
+		}
 	}
 
 	/* Create the ELF interpreter info */
@@ -252,7 +259,7 @@ create_elf_tables(struct linux_binprm *b
 		__put_user((elf_addr_t)p, argv++);
 		len = strnlen_user((void __user *)p, PAGE_SIZE*MAX_ARG_PAGES);
 		if (!len || len > PAGE_SIZE*MAX_ARG_PAGES)
-			return;
+			goto out_ok;
 		p += len;
 	}
 	__put_user(0, argv);
@@ -262,7 +269,7 @@ create_elf_tables(struct linux_binprm *b
 		__put_user((elf_addr_t)p, envp++);
 		len = strnlen_user((void __user *)p, PAGE_SIZE*MAX_ARG_PAGES);
 		if (!len || len > PAGE_SIZE*MAX_ARG_PAGES)
-			return;
+			goto out_ok;
 		p += len;
 	}
 	__put_user(0, envp);
@@ -270,7 +277,15 @@ create_elf_tables(struct linux_binprm *b
 
 	/* Put the elf_info on the stack in the right place.  */
 	sp = (elf_addr_t __user *)envp + 1;
-	copy_to_user(sp, elf_info, ei_index * sizeof(elf_addr_t));
+	if (copy_to_user(sp, elf_info, ei_index * sizeof(elf_addr_t))) {
+		retval = -EFAULT;
+		goto out;
+	}
+
+out_ok:
+	retval = 0;
+out:
+	return retval;
 }
 
 #ifndef elf_map
@@ -400,7 +415,10 @@ static unsigned long load_elf_interp(str
 	 * that there are zero-mapped pages up to and including the 
 	 * last bss page.
 	 */
-	padzero(elf_bss);
+	if (padzero(elf_bss) != 0) {
+		error = -EFAULT;
+		goto out_close;
+	}
 	elf_bss = ELF_PAGESTART(elf_bss + ELF_MIN_ALIGN - 1);	/* What we have mapped so far */
 
 	/* Map the last of the bss segment */
@@ -755,7 +773,11 @@ static int load_elf_binary(struct linux_
 				nbyte = ELF_MIN_ALIGN - nbyte;
 				if (nbyte > elf_brk - elf_bss)
 					nbyte = elf_brk - elf_bss;
-				clear_user((void __user *) elf_bss + load_bias, nbyte);
+				if (clear_user((void __user *) elf_bss + load_bias, nbyte)) {
+					send_sig(SIGKILL, current, 0);
+					retval = -EFAULT;
+					goto out_free_dentry;
+				}
 			}
 		}
 
@@ -837,7 +859,11 @@ static int load_elf_binary(struct linux_
 		send_sig(SIGKILL, current, 0);
 		goto out_free_dentry;
 	}
-	padzero(elf_bss);
+	if (padzero(elf_bss) != 0) {
+		send_sig(SIGKILL, current, 0);
+		retval = -EFAULT;
+		goto out_free_dentry;
+	}
 
 	if (elf_interpreter) {
 		if (interpreter_type == INTERPRETER_AOUT)
@@ -863,6 +889,7 @@ static int load_elf_binary(struct linux_
 	}
 
 	kfree(elf_phdata);
+	elf_phdata = NULL;
 
 	if (interpreter_type != INTERPRETER_AOUT)
 		sys_close(elf_exec_fileno);
@@ -871,8 +898,11 @@ static int load_elf_binary(struct linux_
 
 	compute_creds(bprm);
 	current->flags &= ~PF_FORKNOEXEC;
-	create_elf_tables(bprm, &loc->elf_ex, (interpreter_type == INTERPRETER_AOUT),
-			load_addr, interp_load_addr);
+	if ((retval = create_elf_tables(bprm, &loc->elf_ex, (interpreter_type == INTERPRETER_AOUT),
+			load_addr, interp_load_addr)) != 0) {
+		send_sig(SIGKILL, current, 0);
+		goto out_free_dentry;
+	}
 	/* N.B. passed_fileno might not be initialized? */
 	if (interpreter_type == INTERPRETER_AOUT)
 		current->mm->arg_start += strlen(passed_fileno) + 1;
@@ -1000,7 +1030,10 @@ static int load_elf_library(struct file 
 		goto out_free_ph;
 
 	elf_bss = elf_phdata->p_vaddr + elf_phdata->p_filesz;
-	padzero(elf_bss);
+	if (padzero(elf_bss) != 0) {
+		error = -EFAULT;
+		goto out_free_ph;
+	}
 
 	len = ELF_PAGESTART(elf_phdata->p_filesz + elf_phdata->p_vaddr + ELF_MIN_ALIGN - 1);
 	bss = elf_phdata->p_memsz + elf_phdata->p_vaddr;
@@ -1215,7 +1248,7 @@ static void fill_psinfo(struct elf_prpsi
 	len = mm->arg_end - mm->arg_start;
 	if (len >= ELF_PRARGSZ)
 		len = ELF_PRARGSZ-1;
-	copy_from_user(&psinfo->pr_psargs,
+	len -= copy_from_user(&psinfo->pr_psargs,
 		       (const char __user *)mm->arg_start, len);
 	for(i = 0; i < len; i++)
 		if (psinfo->pr_psargs[i] == 0)

Re: [PATCH] kill the last few warnings in binfmt_elf.c

[Andrew Morton]

Jesper Juhl <juhl-lkml@dif.dk> wrote:
>
> Sorry to keep bugging you with this directly, but I'm still not aware of a 
>  sepperate maintainer for this file.

There isn't one.

> 
>  I've send you 3 patches previously that kill a few warnings in 
>  fs/binfmt_elf.c. The patch below includes those previous 3 and also adds a 
>  fix for the last ones.

The other patches are still queued up in my todo queue.  This is because I
took a close look at these warnings some weeks ago and ended up deciding
that the proper fixes were complex and risky.  So I need to go through your
patches with some care.  I hope you've already done so.

Re: [PATCH] kill the last few warnings in binfmt_elf.c

[Jesper Juhl]

On Tue, 2 Nov 2004, Andrew Morton wrote:

> Date: Tue, 2 Nov 2004 16:54:22 -0800
> From: Andrew Morton <akpm@osdl.org>
> To: Jesper Juhl <juhl-lkml@dif.dk>
> Cc: linux-kernel@vger.kernel.org
> Subject: Re: [PATCH] kill the last few warnings in binfmt_elf.c
> 
> Jesper Juhl <juhl-lkml@dif.dk> wrote:
> >
> > Sorry to keep bugging you with this directly, but I'm still not aware of a 
> >  sepperate maintainer for this file.
> 
> There isn't one.
> 
Ok, you + lkml it is then.

> > 
> >  I've send you 3 patches previously that kill a few warnings in 
> >  fs/binfmt_elf.c. The patch below includes those previous 3 and also adds a 
> >  fix for the last ones.
> 
> The other patches are still queued up in my todo queue.  This is because I
> took a close look at these warnings some weeks ago and ended up deciding
> that the proper fixes were complex and risky.  So I need to go through your
> patches with some care.  I hope you've already done so.
> 
I've certainly tried, but as I'm sure you are aware I have nowhere near 
the level of experience of most people on the list, but I've tried my best 
to ensure that these patches do the right thing, and I've been using them 
on my own box ever since making the first one and I've not seen any ill 
effects yet.

Now that you say the proper fixes are probably "complex and risky" I think 
I'll go back and double check my changes once more this evening. I 
certainly don't want to waste your time with silly errors I could find 
myself.


--
Jesper Juhl

Re: [PATCH] kill the last few warnings in binfmt_elf.c

[Jesper Juhl]

Hi Andrew,

On Wed, 3 Nov 2004, Jesper Juhl wrote:

> On Tue, 2 Nov 2004, Andrew Morton wrote:
> 
[...]
> > The other patches are still queued up in my todo queue.  This is because I
> > took a close look at these warnings some weeks ago and ended up deciding
> > that the proper fixes were complex and risky.  So I need to go through your
> > patches with some care.  I hope you've already done so.
> > 
> I've certainly tried, but as I'm sure you are aware I have nowhere near 
> the level of experience of most people on the list, but I've tried my best 
> to ensure that these patches do the right thing, and I've been using them 
> on my own box ever since making the first one and I've not seen any ill 
> effects yet.
> 
> Now that you say the proper fixes are probably "complex and risky" I think 
> I'll go back and double check my changes once more this evening. I 
> certainly don't want to waste your time with silly errors I could find 
> myself.
> 
I've been looking over my patches again (and still doing so), and I must 
say that I feel quite comfortable with my changes related to padzero() - I 
think at least that bit could go into -mm for now. I'm also quite certain 
my change to fill_psinfo() is safe (although I know you consider it a 
silly fix), that could probably also safely go into -mm. The rest of the 
changes have not caused me any trouble, but I don't have as warm and fuzzy 
a feeling in my belly about those as I do about the padzero() and 
fill_psinfo() changes, so the rest should probably wait a bit unless your 
reading of them tells you they are correct - I'll keep 
digging/reading/learning and see if I can either confirm the rest is OK or 
find the proper solution, but that could take a while...

--
Jesper Juhl