[Patch 1/3] elf: fix one check-after-use

[Patch 1/3] elf: fix one check-after-use

[Amerigo Wang]

Check before use it.

Signed-off-by: WANG Cong <amwang@redhat.com>
Cc: Alexander Viro <viro@zeniv.linux.org.uk>
Cc: David Howells <dhowells@redhat.com>

---
Index: linux-2.6/fs/binfmt_elf.c
===================================================================
--- linux-2.6.orig/fs/binfmt_elf.c
+++ linux-2.6/fs/binfmt_elf.c
@@ -1522,11 +1522,11 @@ static int fill_note_info(struct elfhdr 
 	info->thread = NULL;
 
 	psinfo = kmalloc(sizeof(*psinfo), GFP_KERNEL);
-	fill_note(&info->psinfo, "CORE", NT_PRPSINFO, sizeof(*psinfo), psinfo);
-
 	if (psinfo == NULL)
 		return 0;
 
+	fill_note(&info->psinfo, "CORE", NT_PRPSINFO, sizeof(*psinfo), psinfo);
+
 	/*
 	 * Figure out how many notes we're going to need for each thread.
 	 */

[Patch 2/3] elf: clean up fill_note_info()

[Amerigo Wang]

cleanups

Introduce a helper function elf_note_info_init() to help
fill_note_info() to do initializations, also fix the potential
memory leaks.

Signed-off-by: WANG Cong <amwang@redhat.com>
Cc: Alexander Viro <viro@zeniv.linux.org.uk>
Cc: David Howells <dhowells@redhat.com>

---
Index: linux-2.6/fs/binfmt_elf.c
===================================================================
--- linux-2.6.orig/fs/binfmt_elf.c
+++ linux-2.6/fs/binfmt_elf.c
@@ -1714,42 +1714,54 @@ struct elf_note_info {
 	int numnote;
 };
 
-static int fill_note_info(struct elfhdr *elf, int phdrs,
-			  struct elf_note_info *info,
-			  long signr, struct pt_regs *regs)
+static int elf_note_info_init(struct elf_note_info *info)
 {
-#define	NUM_NOTES	6
-	struct list_head *t;
-
-	info->notes = NULL;
-	info->prstatus = NULL;
-	info->psinfo = NULL;
-	info->fpu = NULL;
-#ifdef ELF_CORE_COPY_XFPREGS
-	info->xfpu = NULL;
-#endif
+	memset(info, 0, sizeof(*info));
 	INIT_LIST_HEAD(&info->thread_list);
 
+#define	NUM_NOTES	6
 	info->notes = kmalloc(NUM_NOTES * sizeof(struct memelfnote),
 			      GFP_KERNEL);
+#undef NUM_NOTES
 	if (!info->notes)
 		return 0;
 	info->psinfo = kmalloc(sizeof(*info->psinfo), GFP_KERNEL);
 	if (!info->psinfo)
-		return 0;
+		goto notes_free;
 	info->prstatus = kmalloc(sizeof(*info->prstatus), GFP_KERNEL);
 	if (!info->prstatus)
-		return 0;
+		goto psinfo_free;
 	info->fpu = kmalloc(sizeof(*info->fpu), GFP_KERNEL);
 	if (!info->fpu)
-		return 0;
+		goto prstatus_free;
 #ifdef ELF_CORE_COPY_XFPREGS
 	info->xfpu = kmalloc(sizeof(*info->xfpu), GFP_KERNEL);
 	if (!info->xfpu)
-		return 0;
+		goto fpu_free;
+#endif
+	return 1;
+#ifdef ELF_CORE_COPY_XFPREGS
+ fpu_free:
+	kfree(info->fpu);
 #endif
+ prstatus_free:
+	kfree(info->prstatus);
+ psinfo_free:
+	kfree(info->psinfo);
+ notes_free:
+	kfree(info->notes);
+	return 0;
+}
+
+static int fill_note_info(struct elfhdr *elf, int phdrs,
+			  struct elf_note_info *info,
+			  long signr, struct pt_regs *regs)
+{
+	struct list_head *t;
+
+	if (!elf_note_info_init(info))
+		return 0;
 
-	info->thread_status_size = 0;
 	if (signr) {
 		struct core_thread *ct;
 		struct elf_thread_status *ets;
@@ -1809,8 +1821,6 @@ static int fill_note_info(struct elfhdr 
 #endif
 
 	return 1;
-
-#undef NUM_NOTES
 }
 
 static size_t get_note_info_size(struct elf_note_info *info)

[Patch 3/3] elf: use a macro instead of a raw number

[Amerigo Wang]

Use TASK_COMM_LEN instead of the raw number 16.

Signed-off-by: WANG Cong <amwang@redhat.com>
Cc: Alexander Viro <viro@zeniv.linux.org.uk>
Cc: David Howells <dhowells@redhat.com>

---
Index: linux-2.6/include/linux/elfcore.h
===================================================================
--- linux-2.6.orig/include/linux/elfcore.h
+++ linux-2.6/include/linux/elfcore.h
@@ -90,7 +90,7 @@ struct elf_prpsinfo
 	__kernel_gid_t	pr_gid;
 	pid_t	pr_pid, pr_ppid, pr_pgrp, pr_sid;
 	/* Lots missing */
-	char	pr_fname[16];	/* filename of executable */
+	char	pr_fname[TASK_COMM_LEN];/* filename of executable */
 	char	pr_psargs[ELF_PRARGSZ];	/* initial part of arg list */
 };
 

Re: [Patch 3/3] elf: use a macro instead of a raw number

[Roland McGrath]

> Use TASK_COMM_LEN instead of the raw number 16.

This is probably a bad idea.  elf_prpsinfo layout is a userland ABI.
AFAIK there is no reason TASK_COMM_LEN could not be enlarged in the kernel.


Thanks,
Roland

Re: [Patch 1/3] elf: fix one check-after-use

[Roland McGrath]

> Check before use it.

Oops!  I don't know how that snuck in there.  Good catch.

Acked-by: Roland McGrath <roland@redhat.com>


Thanks,
Roland

Re: [Patch 1/3] elf: fix one check-after-use

[James Morris]

On Wed, 1 Jul 2009, Amerigo Wang wrote:

> 
> Check before use it.
> 
> Signed-off-by: WANG Cong <amwang@redhat.com>
> Cc: Alexander Viro <viro@zeniv.linux.org.uk>
> Cc: David Howells <dhowells@redhat.com>

Probably needs to go into Linus' tree and stable.


Acked-by: Roland McGrath <roland@redhat.com>
Acked-by: James Morris <jmorris@namei.org>

> 
> ---
> Index: linux-2.6/fs/binfmt_elf.c
> ===================================================================
> --- linux-2.6.orig/fs/binfmt_elf.c
> +++ linux-2.6/fs/binfmt_elf.c
> @@ -1522,11 +1522,11 @@ static int fill_note_info(struct elfhdr 
>  	info->thread = NULL;
>  
>  	psinfo = kmalloc(sizeof(*psinfo), GFP_KERNEL);
> -	fill_note(&info->psinfo, "CORE", NT_PRPSINFO, sizeof(*psinfo), psinfo);
> -
>  	if (psinfo == NULL)
>  		return 0;
>  
> +	fill_note(&info->psinfo, "CORE", NT_PRPSINFO, sizeof(*psinfo), psinfo);
> +
>  	/*
>  	 * Figure out how many notes we're going to need for each thread.
>  	 */
> 

-- 
James Morris
<jmorris@namei.org>

Re: [Patch 2/3] elf: clean up fill_note_info()

[Andrew Morton]

On Wed, 1 Jul 2009 01:06:36 -0400
Amerigo Wang <amwang@redhat.com> wrote:

> +#define	NUM_NOTES	6
>  	info->notes = kmalloc(NUM_NOTES * sizeof(struct memelfnote),
>  			      GFP_KERNEL);
> +#undef NUM_NOTES

That #define amounts to a really perverse code comment.

How about we do this?

--- a/fs/binfmt_elf.c~elf-clean-up-fill_note_info-fix
+++ a/fs/binfmt_elf.c
@@ -1719,10 +1719,8 @@ static int elf_note_info_init(struct elf
 	memset(info, 0, sizeof(*info));
 	INIT_LIST_HEAD(&info->thread_list);
 
-#define	NUM_NOTES	6
-	info->notes = kmalloc(NUM_NOTES * sizeof(struct memelfnote),
-			      GFP_KERNEL);
-#undef NUM_NOTES
+	/* Allocate space for six ELF notes */
+	info->notes = kmalloc(6 * sizeof(struct memelfnote), GFP_KERNEL);
 	if (!info->notes)
 		return 0;
 	info->psinfo = kmalloc(sizeof(*info->psinfo), GFP_KERNEL);
_

Re: [Patch 3/3] elf: use a macro instead of a raw number

[Amerigo Wang]

Roland McGrath wrote:
>> Use TASK_COMM_LEN instead of the raw number 16.
>>     
>
> This is probably a bad idea.  elf_prpsinfo layout is a userland ABI.
> AFAIK there is no reason TASK_COMM_LEN could not be enlarged in the kernel.
>   

Thanks for your reply.

But in the kernel code, pr_fname is copied from ->comm, they should
be equal, shouldn't they?

Hmm, yes, I agree that 16 is too small for ELF core dump, the longer
names get truncated and gdb will complain about this...

I can cook a patch to increase this length if you agree.

Thanks.

Re: [Patch 2/3] elf: clean up fill_note_info()

[Amerigo Wang]

Andrew Morton wrote:
> On Wed, 1 Jul 2009 01:06:36 -0400
> Amerigo Wang <amwang@redhat.com> wrote:
>
>   
>> +#define	NUM_NOTES	6
>>  	info->notes = kmalloc(NUM_NOTES * sizeof(struct memelfnote),
>>  			      GFP_KERNEL);
>> +#undef NUM_NOTES
>>     
>
> That #define amounts to a really perverse code comment.
>
> How about we do this?
>   


Ah, better! No problem for me.

Thank you!
> --- a/fs/binfmt_elf.c~elf-clean-up-fill_note_info-fix
> +++ a/fs/binfmt_elf.c
> @@ -1719,10 +1719,8 @@ static int elf_note_info_init(struct elf
>  	memset(info, 0, sizeof(*info));
>  	INIT_LIST_HEAD(&info->thread_list);
>  
> -#define	NUM_NOTES	6
> -	info->notes = kmalloc(NUM_NOTES * sizeof(struct memelfnote),
> -			      GFP_KERNEL);
> -#undef NUM_NOTES
> +	/* Allocate space for six ELF notes */
> +	info->notes = kmalloc(6 * sizeof(struct memelfnote), GFP_KERNEL);
>  	if (!info->notes)
>  		return 0;
>  	info->psinfo = kmalloc(sizeof(*info->psinfo), GFP_KERNEL);
> _
>
>   

Re: [Patch 3/3] elf: use a macro instead of a raw number

[Roland McGrath]

> But in the kernel code, pr_fname is copied from ->comm, they should
> be equal, shouldn't they?

The point is that we are not at liberty to change the size of pr_fname.
Its size and layout are known to userland and thus set in stone.  To
have a larger size, we would have to invent a new NT_* type code with a
new layout that would also be known to userland.  It's not worth the
bother.  

Nowadays a debugger can see AT_EXECFN in auxv (NT_AUXV in core files,
/proc/pid/auxv live), and look at that address in the user memory (core
file or process).  That's clobberable on the user-mode stack, but it can
be of unbounded size.


Thanks,
Roland