fs/binfmt_elf.c: fix inconsistent vma dump size

RFC PATCH Superseded Jungseung Lee 2 participants 6 messages Nov 22, 2014 – Nov 26, 2014

Subsystems (2)

VFS exec and binfmt

lore

Share & tools

lore

Jungseung Lee

author

2014-11-22 19:16 UTC #0 lore raw

RFC PATCH 1/1 fs/binfmt_elf.c: fix inconsistent vma dump size

vma_dump_size() has been used several times on actual dumper and it is supposed to be same values for same vma.
But vma_dump_size() could be different, while coredump is procceeded. (e.g. remove shared memory)

In that case, header info and vma size could be inconsistent and it cause wrong coredump file creation.

Fix the problem by always using the same vma dump size which is stored in vma_filsz.

Signed-off-by Jungseung Lee <[email protected]>

---
 fs/binfmt_elf.c | 40 ++++++++++++++++++++++------------------
 1 file changed, 22 insertions(+), 18 deletions(-)

fs/binfmt_elf.c +22 ~~-18~~

index d8fc060..3ba08ca 100644
@@ -1994,18 +1994,6 @@ static void fill_extnum_info(struct elfhdr *elf, struct elf_shdr *shdr4extnum,
 	shdr4extnum->sh_info = segs;
 }
 
-static size_t elf_core_vma_data_size(struct vm_area_struct *gate_vma,
-				     unsigned long mm_flags)
-{
-	struct vm_area_struct *vma;
-	size_t size = 0;
-
-	for (vma = first_vma(current, gate_vma); vma != NULL;
-	     vma = next_vma(vma, gate_vma))
-		size += vma_dump_size(vma, mm_flags);
-	return size;
-}
-
 /*
  * Actual dumper
  *

@@ -2017,7 +2005,8 @@ static int elf_core_dump(struct coredump_params *cprm)
 {
 	int has_dumped = 0;
 	mm_segment_t fs;
-	int segs;
+	int segs, i;
+	size_t vma_data_size = 0;
 	struct vm_area_struct *vma, *gate_vma;
 	struct elfhdr *elf = NULL;
 	loff_t offset = 0, dataoff;

@@ -2026,6 +2015,7 @@ static int elf_core_dump(struct coredump_params *cprm)
 	struct elf_shdr *shdr4extnum = NULL;
 	Elf_Half e_phnum;
 	elf_addr_t e_shoff;
+	elf_addr_t *vma_filesz = NULL;
 
 	/*
 	 * We no longer stop all VM operations.

@@ -2093,7 +2083,20 @@ static int elf_core_dump(struct coredump_params *cprm)
 
 	dataoff = offset = roundup(offset, ELF_EXEC_PAGESIZE);
 
-	offset += elf_core_vma_data_size(gate_vma, cprm->mm_flags);
+	vma_filesz = kmalloc(sizeof(*vma_filesz) * (segs - 1), GFP_KERNEL);
+	if (!vma_filesz)
+		goto end_coredump;
+
+	for (i = 0, vma = first_vma(current, gate_vma); vma != NULL;
+			vma = next_vma(vma, gate_vma)) {
+		unsigned long dump_size;
+
+		dump_size = vma_dump_size(vma, cprm->mm_flags);
+		vma_filesz[i++] = dump_size;
+		vma_data_size += dump_size;
+	}
+
+	offset += vma_data_size;
 	offset += elf_core_extra_data_size();
 	e_shoff = offset;

@@ -2113,7 +2116,7 @@ static int elf_core_dump(struct coredump_params *cprm)
 		goto end_coredump;
 
 	/* Write program headers for segments dump */
-	for (vma = first_vma(current, gate_vma); vma != NULL;
+	for (i = 0, vma = first_vma(current, gate_vma); vma != NULL;
 			vma = next_vma(vma, gate_vma)) {
 		struct elf_phdr phdr;

@@ -2121,7 +2124,7 @@ static int elf_core_dump(struct coredump_params *cprm)
 		phdr.p_offset = offset;
 		phdr.p_vaddr = vma->vm_start;
 		phdr.p_paddr = 0;
-		phdr.p_filesz = vma_dump_size(vma, cprm->mm_flags);
+		phdr.p_filesz = vma_filesz[i++];
 		phdr.p_memsz = vma->vm_end - vma->vm_start;
 		offset += phdr.p_filesz;
 		phdr.p_flags = vma->vm_flags & VM_READ ? PF_R : 0;

@@ -2149,12 +2152,12 @@ static int elf_core_dump(struct coredump_params *cprm)
 	if (!dump_skip(cprm, dataoff - cprm->written))
 		goto end_coredump;
 
-	for (vma = first_vma(current, gate_vma); vma != NULL;
+	for (i = 0, vma = first_vma(current, gate_vma); vma != NULL;
 			vma = next_vma(vma, gate_vma)) {
 		unsigned long addr;
 		unsigned long end;
 
-		end = vma->vm_start + vma_dump_size(vma, cprm->mm_flags);
+		end = vma->vm_start + vma_filesz[i++];
 
 		for (addr = vma->vm_start; addr < end; addr += PAGE_SIZE) {
 			struct page *page;

@@ -2187,6 +2190,7 @@ end_coredump:
 cleanup:
 	free_note_info(&info);
 	kfree(shdr4extnum);
+	kfree(vma_filesz);
 	kfree(phdr4note);
 	kfree(elf);
 out:

Signature · 1 line

1.9.1

Andrew Morton

2014-11-25 21:38 UTC #1 lore raw

Quoted 10 lines · On Sun, 23 Nov 2014 04:16:39 +0900 Jungseung Lee <[email protected]> wrote:

vma_dump_size() has been used several times on actual dumper and it is supposed to be same values for same vma.
But vma_dump_size() could be different, while coredump is procceeded. (e.g. remove shared memory)
In that case, header info and vma size could be inconsistent and it cause wrong coredump file creation.
Fix the problem by always using the same vma dump size which is stored in vma_filsz.

So concurrent shared memory removal causes inconsistencies.

But concurrent shared memory removal can still cause inconsistency after this patch, can't it? If it happens while vma_filesz[] is being populated or if it happens between vma_filesz[] population and vma_filesz[] usage. This will result in a dump file which is internally consistent, but is inconsistent with reality?

If my understanding is correct then please fully describe this scenario within the changelog and explain why it is tolerable, if it is tolerable.

fs/binfmt_elf.c static int elf_core_dump(struct coredump_params *cprm) · line 2086

Quoted 6 lines

fs/binfmt_elf.c +1 -1

@@ -2093,7 +2083,20 @@ static int elf_core_dump(struct coredump_params *cprm)
 
 	dataoff = offset = roundup(offset, ELF_EXEC_PAGESIZE);
 
-	offset += elf_core_vma_data_size(gate_vma, cprm->mm_flags);
+	vma_filesz = kmalloc(sizeof(*vma_filesz) * (segs - 1), GFP_KERNEL);

Use kmalloc_array() here, in case a disaster has occurred...

Quoted 13 lines

fs/binfmt_elf.c +13 -0

+	if (!vma_filesz)
+		goto end_coredump;
+
+	for (i = 0, vma = first_vma(current, gate_vma); vma != NULL;
+			vma = next_vma(vma, gate_vma)) {
+		unsigned long dump_size;
+
+		dump_size = vma_dump_size(vma, cprm->mm_flags);
+		vma_filesz[i++] = dump_size;
+		vma_data_size += dump_size;
+	}
+
+	offset += vma_data_size;

Jungseung Lee

author

2014-11-26 06:51 UTC #2 lore raw

Hello, Andrew.

2014-11-26 6:38 GMT+09:00 Andrew Morton <[email protected]>:

Quoted 13 lines · On Sun, 23 Nov 2014 04:16:39 +0900 Jungseung Lee <[email protected]> wrote:

Quoted 11 lines
vma_dump_size() has been used several times on actual dumper and it is supposed to be same values for same vma.
But vma_dump_size() could be different, while coredump is procceeded. (e.g. remove shared memory)
In that case, header info and vma size could be inconsistent and it cause wrong coredump file creation.
Fix the problem by always using the same vma dump size which is stored in vma_filsz.

Quoted 8 lines

So concurrent shared memory removal causes inconsistencies.
But concurrent shared memory removal can still cause inconsistency after this patch, can't it? If it happens while vma_filesz[] is being populated or if it happens between vma_filesz[] population and vma_filesz[] usage. This will result in a dump file which is internally consistent, but is inconsistent with reality?

Yes, you are right.

The problem is internally inconsistent between header and dump data in core file that
cause wrong vma detection in gdb.
In my opinion, if the factor whether to dump or not is changed while coredump is working it is
reasonable to choose anyone.
(bit 1) anonymous shared memory vs (bit 3) file-backed shared memory

Quoted 4 lines

If my understanding is correct then please fully describe this scenario within the changelog and explain why it is tolerable, if it is tolerable.

OK, I'll.

Quoted 9 lines

fs/binfmt_elf.c static int elf_core_dump(struct coredump_params *cprm) · line 2086
Quoted 6 lines
fs/binfmt_elf.c +1 -1
@@ -2093,7 +2083,20 @@ static int elf_core_dump(struct coredump_params *cprm)
 
      dataoff = offset = roundup(offset, ELF_EXEC_PAGESIZE);
 
-     offset += elf_core_vma_data_size(gate_vma, cprm->mm_flags);
+     vma_filesz = kmalloc(sizeof(*vma_filesz) * (segs - 1), GFP_KERNEL);
Use kmalloc_array() here, in case a disaster has occurred...

I'll try to find another way how to pass or save vma size with consistency.

Quoted 5 lines

fs/binfmt_elf.c +5 -0

+     if (!vma_filesz)
+             goto end_coredump;
+
+     for (i = 0, vma = first_vma(current, gate_vma); vma != NULL;
+                     vma = next_vma(vma, gate_vma)) {

Quoted 9 lines

fs/binfmt_elf.c static int elf_core_dump(struct coredump_params *cprm) · line 2099

Quoted 8 lines

fs/binfmt_elf.c +8 -0

+             unsigned long dump_size;
+
+             dump_size = vma_dump_size(vma, cprm->mm_flags);
+             vma_filesz[i++] = dump_size;
+             vma_data_size += dump_size;
+     }
+
+     offset += vma_data_size;

Thanks,

Jungseung Lee

author

2014-11-26 12:37 UTC #3 lore raw

2014-11-26 15:51 GMT+09:00 Jungseung Lee <[email protected]>:

Quoted 11 lines

Quoted 9 lines
fs/binfmt_elf.c static int elf_core_dump(struct coredump_params *cprm) · line 2086
Quoted 6 lines
fs/binfmt_elf.c +1 -1
@@ -2093,7 +2083,20 @@ static int elf_core_dump(struct coredump_params *cprm)
 
      dataoff = offset = roundup(offset, ELF_EXEC_PAGESIZE);
 
-     offset += elf_core_vma_data_size(gate_vma, cprm->mm_flags);
+     vma_filesz = kmalloc(sizeof(*vma_filesz) * (segs - 1), GFP_KERNEL);
Use kmalloc_array() here, in case a disaster has occurred...
I'll try to find another way how to pass or save vma size with consistency.

it looks like just to replace kmalloc to vmalloc could be solution. I'll send patch, thanks.

Andrew Morton

2014-11-26 12:42 UTC #4 lore raw

Quoted 11 lines · On Wed, 26 Nov 2014 21:37:54 +0900 Jungseung Lee <[email protected]> wrote:

2014-11-26 15:51 GMT+09:00 Jungseung Lee <[email protected]>:
Quoted 9 lines
Quoted 9 lines
fs/binfmt_elf.c static int elf_core_dump(struct coredump_params *cprm) · line 2086
Quoted 6 lines
fs/binfmt_elf.c +1 -1
@@ -2093,7 +2083,20 @@ static int elf_core_dump(struct coredump_params *cprm)
 
      dataoff = offset = roundup(offset, ELF_EXEC_PAGESIZE);
 
-     offset += elf_core_vma_data_size(gate_vma, cprm->mm_flags);
+     vma_filesz = kmalloc(sizeof(*vma_filesz) * (segs - 1), GFP_KERNEL);
Use kmalloc_array() here, in case a disaster has occurred...

Quoted 4 lines

Quoted 2 lines
I'll try to find another way how to pass or save vma size with consistency.
it looks like just to replace kmalloc to vmalloc could be solution. I'll send patch, thanks.

Solution to what? I was merely suggesting the use of kmalloc_array() instead of open-coded kmalloc(n * m).

Jungseung Lee

author

2014-11-26 13:13 UTC #5 lore raw

2014-11-26 21:43 GMT+09:00 Andrew Morton <[email protected]>:

Quoted 13 lines · On Wed, 26 Nov 2014 21:37:54 +0900 Jungseung Lee <[email protected]> wrote:

Quoted 12 lines
2014-11-26 15:51 GMT+09:00 Jungseung Lee <[email protected]>:
Quoted 9 lines
Quoted 9 lines
fs/binfmt_elf.c static int elf_core_dump(struct coredump_params *cprm) · line 2086
Quoted 6 lines
fs/binfmt_elf.c +1 -1
@@ -2093,7 +2083,20 @@ static int elf_core_dump(struct coredump_params *cprm)
 
      dataoff = offset = roundup(offset, ELF_EXEC_PAGESIZE);
 
-     offset += elf_core_vma_data_size(gate_vma, cprm->mm_flags);
+     vma_filesz = kmalloc(sizeof(*vma_filesz) * (segs - 1), GFP_KERNEL);
Use kmalloc_array() here, in case a disaster has occurred...

Quoted 8 lines

Quoted 4 lines
Quoted 2 lines
I'll try to find another way how to pass or save vma size with consistency.
it looks like just to replace kmalloc to vmalloc could be solution. I'll send patch, thanks.
Solution to what? I was merely suggesting the use of kmalloc_array() instead of open-coded kmalloc(n * m).

Ah.. I misunderstood with my poor english. Thanks for the notice to me.