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(-)
diff --git a/fs/binfmt_elf.c b/fs/binfmt_elf.c
index d8fc060..3ba08ca 100644
--- a/fs/binfmt_elf.c
+++ b/fs/binfmt_elf.c
@@ -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:
--
1.9.1
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.
> @@ -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...
> + 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;
Hello, Andrew.
2014-11-26 6:38 GMT+09:00 Andrew Morton <[email protected]>:
> 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?
>
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
> 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.
>> @@ -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.
>> + 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;
>
Thanks,
2014-11-26 15:51 GMT+09:00 Jungseung Lee <[email protected]>:
>>> @@ -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.
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]>:
>
> >>> @@ -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.
Solution to what? I was merely suggesting the use of kmalloc_array()
instead of open-coded kmalloc(n * m).
2014-11-26 21:43 GMT+09:00 Andrew Morton <[email protected]>:
> 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]>:
>>
>> >>> @@ -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.
>
> 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.