Skip to contentSkip to conversation

coredump: prevent double-free on an error path in core dumper

PATCH In review Denys Vlasenko 3 participants 4 messages Sep 12, 2012

Subsystems (2)
VFS exec and binfmt
lore
Share & tools
lore

Denys Vlasenko

author
#0 lore raw

PATCH 1/1 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 <[email protected]>
Signed-off-by Denys Vlasenko <[email protected]>
---
 fs/binfmt_elf.c |   19 ++++---------------
 1 files changed, 4 insertions(+), 15 deletions(-)
fs/binfmt_elf.c +4 -15
index 760d7f5..1b4efbc 100644
@@ -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,
Signature · 1 line
1.7.7.6
#1 lore raw
Quoted 32 lines

-----Original Message-----
From: [email protected] [mailto:linux-kernel- [email protected]] On Behalf Of Denys Vlasenko
Sent: Wednesday, September 12, 2012 5:07 PM
To: Oleg Nesterov; [email protected]; 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 <[email protected]>
Signed-off-by Denys Vlasenko <[email protected]>
---
 fs/binfmt_elf.c |   19 ++++---------------
 1 files changed, 4 insertions(+), 15 deletions(-)
fs/binfmt_elf.c +0 -0
index 760d7f5..1b4efbc 100644

fs/binfmt_elf.c static int elf_note_info_init(struct elf_note_info · line 1697

Quoted 3 lines
fs/binfmt_elf.c +0 -0
@@ -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.

Quoted 40 lines
fs/binfmt_elf.c +4 -15
 	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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

#2 lore raw
Quoted 9 lines · Venu Byravarasu

fs/binfmt_elf.c static int elf_note_info_init(struct elf_note_info · line 1697

Quoted 7 lines
fs/binfmt_elf.c +0 -0
@@ -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.

#3 lore raw
Quoted 12 lines · Oleg Nesterov
Quoted 9 lines · Venu Byravarasu

fs/binfmt_elf.c static int elf_note_info_init(struct elf_note_info · line 1697

Quoted 7 lines
fs/binfmt_elf.c +0 -0
@@ -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.

Quoted 3 lines

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.