linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] coredump: prevent double-free on an error path in core dumper
@ 2012-09-12 11:37 Denys Vlasenko
  2012-09-12 11:45 ` Venu Byravarasu
  0 siblings, 1 reply; 4+ messages in thread
From: Denys Vlasenko @ 2012-09-12 11:37 UTC (permalink / raw)
  To: Oleg Nesterov, linux-kernel, Andrew Morton, Roland McGrath; +Cc: Denys Vlasenko

In !CORE_DUMP_USE_REGSET case, if elf_note_info_init fails to allocate memory
for info->fields, it frees already allocated stuff and returns
error to its caller, fill_note_info. Which in turn returns
error to its caller, elf_core_dump. Which jumps to cleanup
label and calls free_note_info, which will happily try to
free all info->fields again. BOOM.

This is the fix.

Signed-off-by: Oleg Nesterov <oleg@redhat.com>
Signed-off-by: Denys Vlasenko <vda.linux@googlemail.com>
---
 fs/binfmt_elf.c |   19 ++++---------------
 1 files changed, 4 insertions(+), 15 deletions(-)

diff --git a/fs/binfmt_elf.c b/fs/binfmt_elf.c
index 760d7f5..1b4efbc 100644
--- a/fs/binfmt_elf.c
+++ b/fs/binfmt_elf.c
@@ -1695,30 +1695,19 @@ static int elf_note_info_init(struct elf_note_info *info)
 		return 0;
 	info->psinfo = kmalloc(sizeof(*info->psinfo), GFP_KERNEL);
 	if (!info->psinfo)
-		goto notes_free;
+		return 0;
 	info->prstatus = kmalloc(sizeof(*info->prstatus), GFP_KERNEL);
 	if (!info->prstatus)
-		goto psinfo_free;
+		return 0;
 	info->fpu = kmalloc(sizeof(*info->fpu), GFP_KERNEL);
 	if (!info->fpu)
-		goto prstatus_free;
+		return 0;
 #ifdef ELF_CORE_COPY_XFPREGS
 	info->xfpu = kmalloc(sizeof(*info->xfpu), GFP_KERNEL);
 	if (!info->xfpu)
-		goto fpu_free;
+		return 0;
 #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,
-- 
1.7.7.6


^ permalink raw reply related	[flat|nested] 4+ messages in thread

* RE: [PATCH] coredump: prevent double-free on an error path in core dumper
  2012-09-12 11:37 [PATCH] coredump: prevent double-free on an error path in core dumper Denys Vlasenko
@ 2012-09-12 11:45 ` Venu Byravarasu
  2012-09-12 13:00   ` Oleg Nesterov
  0 siblings, 1 reply; 4+ messages in thread
From: Venu Byravarasu @ 2012-09-12 11:45 UTC (permalink / raw)
  To: Denys Vlasenko, Oleg Nesterov, linux-kernel, Andrew Morton,
	Roland McGrath


> -----Original Message-----
> From: linux-kernel-owner@vger.kernel.org [mailto:linux-kernel-
> owner@vger.kernel.org] On Behalf Of Denys Vlasenko
> Sent: Wednesday, September 12, 2012 5:07 PM
> To: Oleg Nesterov; linux-kernel@vger.kernel.org; Andrew Morton; Roland
> McGrath
> Cc: Denys Vlasenko
> Subject: [PATCH] coredump: prevent double-free on an error path in core
> dumper
> 
> In !CORE_DUMP_USE_REGSET case, if elf_note_info_init fails to allocate
> memory
> for info->fields, it frees already allocated stuff and returns
> error to its caller, fill_note_info. Which in turn returns
> error to its caller, elf_core_dump. Which jumps to cleanup
> label and calls free_note_info, which will happily try to
> free all info->fields again. BOOM.
> 
> This is the fix.
> 
> Signed-off-by: Oleg Nesterov <oleg@redhat.com>
> Signed-off-by: Denys Vlasenko <vda.linux@googlemail.com>
> ---
>  fs/binfmt_elf.c |   19 ++++---------------
>  1 files changed, 4 insertions(+), 15 deletions(-)
> 
> diff --git a/fs/binfmt_elf.c b/fs/binfmt_elf.c
> index 760d7f5..1b4efbc 100644
> --- a/fs/binfmt_elf.c
> +++ b/fs/binfmt_elf.c
> @@ -1695,30 +1695,19 @@ static int elf_note_info_init(struct elf_note_info
> *info)
>  		return 0;
>  	info->psinfo = kmalloc(sizeof(*info->psinfo), GFP_KERNEL);

Why don't you change kmalloc to devm_kzalloc, so that free can be ignored altogether. 

>  	if (!info->psinfo)
> -		goto notes_free;
> +		return 0;
>  	info->prstatus = kmalloc(sizeof(*info->prstatus), GFP_KERNEL);
>  	if (!info->prstatus)
> -		goto psinfo_free;
> +		return 0;
>  	info->fpu = kmalloc(sizeof(*info->fpu), GFP_KERNEL);
>  	if (!info->fpu)
> -		goto prstatus_free;
> +		return 0;
>  #ifdef ELF_CORE_COPY_XFPREGS
>  	info->xfpu = kmalloc(sizeof(*info->xfpu), GFP_KERNEL);
>  	if (!info->xfpu)
> -		goto fpu_free;
> +		return 0;
>  #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,
> --
> 1.7.7.6
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH] coredump: prevent double-free on an error path in core dumper
  2012-09-12 11:45 ` Venu Byravarasu
@ 2012-09-12 13:00   ` Oleg Nesterov
  2012-09-12 19:00     ` Oleg Nesterov
  0 siblings, 1 reply; 4+ messages in thread
From: Oleg Nesterov @ 2012-09-12 13:00 UTC (permalink / raw)
  To: Venu Byravarasu
  Cc: Denys Vlasenko, linux-kernel, Andrew Morton, Roland McGrath

On 09/12, Venu Byravarasu wrote:
>
> > --- a/fs/binfmt_elf.c
> > +++ b/fs/binfmt_elf.c
> > @@ -1695,30 +1695,19 @@ static int elf_note_info_init(struct elf_note_info
> > *info)
> >  		return 0;
> >  	info->psinfo = kmalloc(sizeof(*info->psinfo), GFP_KERNEL);
>
> Why don't you change kmalloc to devm_kzalloc, so that free can be ignored altogether.

kzalloc(), I guess...

Still I can't understand what did you mean. If kmalloc() fails we
return "fail" and ->psinfo = NULL, the subsequent kfree(NULL) is nop.

Oleg.


^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH] coredump: prevent double-free on an error path in core dumper
  2012-09-12 13:00   ` Oleg Nesterov
@ 2012-09-12 19:00     ` Oleg Nesterov
  0 siblings, 0 replies; 4+ messages in thread
From: Oleg Nesterov @ 2012-09-12 19:00 UTC (permalink / raw)
  To: Venu Byravarasu
  Cc: Denys Vlasenko, linux-kernel, Andrew Morton, Roland McGrath

On 09/12, Oleg Nesterov wrote:
>
> On 09/12, Venu Byravarasu wrote:
> >
> > > --- a/fs/binfmt_elf.c
> > > +++ b/fs/binfmt_elf.c
> > > @@ -1695,30 +1695,19 @@ static int elf_note_info_init(struct elf_note_info
> > > *info)
> > >  		return 0;
> > >  	info->psinfo = kmalloc(sizeof(*info->psinfo), GFP_KERNEL);
> >
> > Why don't you change kmalloc to devm_kzalloc, so that free can be ignored altogether.
>
> kzalloc(), I guess...
>
> Still I can't understand what did you mean.

Denys explained me privately what did you mean.

Well. elf_core_dump() does a lot of kmalloc's. We should either
change them all or nothing. And even if we do this, I think this
particular patch makes sense anyway.

Not sure we want "struct device" for this, perhaps this devres
code should be generalized somehow.

Oleg.


^ permalink raw reply	[flat|nested] 4+ messages in thread

end of thread, other threads:[~2012-09-12 18:58 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2012-09-12 11:37 [PATCH] coredump: prevent double-free on an error path in core dumper Denys Vlasenko
2012-09-12 11:45 ` Venu Byravarasu
2012-09-12 13:00   ` Oleg Nesterov
2012-09-12 19:00     ` Oleg Nesterov

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).