Check before use it.
Signed-off-by: WANG Cong <[email protected]>
Cc: Alexander Viro <[email protected]>
Cc: David Howells <[email protected]>
---
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.
*/
Use TASK_COMM_LEN instead of the raw number 16.
Signed-off-by: WANG Cong <[email protected]>
Cc: Alexander Viro <[email protected]>
Cc: David Howells <[email protected]>
---
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 */
};
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 <[email protected]>
Cc: Alexander Viro <[email protected]>
Cc: David Howells <[email protected]>
---
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)
> 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
> Check before use it.
Oops! I don't know how that snuck in there. Good catch.
Acked-by: Roland McGrath <[email protected]>
Thanks,
Roland
On Wed, 1 Jul 2009, Amerigo Wang wrote:
>
> Check before use it.
>
> Signed-off-by: WANG Cong <[email protected]>
> Cc: Alexander Viro <[email protected]>
> Cc: David Howells <[email protected]>
Probably needs to go into Linus' tree and stable.
Acked-by: Roland McGrath <[email protected]>
Acked-by: James Morris <[email protected]>
>
> ---
> 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
<[email protected]>
On Wed, 1 Jul 2009 01:06:36 -0400
Amerigo Wang <[email protected]> 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);
_
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.
Andrew Morton wrote:
> On Wed, 1 Jul 2009 01:06:36 -0400
> Amerigo Wang <[email protected]> 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);
> _
>
>
> 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