Re: [PATCH v4 3/3] kdump: Protect vmcoreinfo data under the crash memory
[snip] > >> index 43cdb00..a29e9ad 100644 > >> --- a/kernel/crash_core.c > >> +++ b/kernel/crash_core.c > >> @@ -15,9 +15,12 @@ > >> > >> /* vmcoreinfo stuff */ > >> static unsigned char *vmcoreinfo_data; > >> -size_t vmcoreinfo_size; > >> +static size_t vmcoreinfo_size; > >> u32 *vmcoreinfo_note; > >> > >> +/* trusted vmcoreinfo, e.g. we can make a copy in the crash memory */ > > May make it clearer like: > > /* Trusted vmcoreinfo copy in the kdump reserved memory */ > > My thought is that it is in crash_core.c now which should be independent of > kexec/kdump, > so I used "e.g. ..." just like one use case. Ok, then it is fine. [snip] > >> static int kimage_add_entry(struct kimage *image, kimage_entry_t entry) > >> { > >>if (*image->entry != 0) > >> @@ -598,6 +632,11 @@ void kimage_free(struct kimage *image) > >>if (image->file_mode) > >>kimage_file_post_load_cleanup(image); > >> > >> + if (image->vmcoreinfo_data_copy) { > >> + crash_update_vmcoreinfo_safecopy(NULL); > >> + vunmap(image->vmcoreinfo_data_copy); > >> + } > >> + > > Should move above chunk before the freeing of the actual page? > > It should be fine, because it is allocated from the reserved memory, it > doesn't > need to be freed. Anyway I can move it above to avoid confusion. Thanks! > Yes, it looks better, thanks for explanation. Thanks Dave ___ kexec mailing list kexec@lists.infradead.org http://lists.infradead.org/mailman/listinfo/kexec
Re: [PATCH v4 3/3] kdump: Protect vmcoreinfo data under the crash memory
On 04/26/2017 at 03:09 PM, Dave Young wrote: > On 04/20/17 at 07:39pm, Xunlei Pang wrote: >> Currently vmcoreinfo data is updated at boot time subsys_initcall(), >> it has the risk of being modified by some wrong code during system >> is running. >> >> As a result, vmcore dumped may contain the wrong vmcoreinfo. Later on, >> when using "crash", "makedumpfile", etc utility to parse this vmcore, >> we probably will get "Segmentation fault" or other unexpected errors. >> >> E.g. 1) wrong code overwrites vmcoreinfo_data; 2) further crashes the >> system; 3) trigger kdump, then we obviously will fail to recognize the >> crash context correctly due to the corrupted vmcoreinfo. >> >> Now except for vmcoreinfo, all the crash data is well protected(including >> the cpu note which is fully updated in the crash path, thus its correctness >> is guaranteed). Given that vmcoreinfo data is a large chunk prepared for >> kdump, we better protect it as well. >> >> To solve this, we relocate and copy vmcoreinfo_data to the crash memory >> when kdump is loading via kexec syscalls. Because the whole crash memory >> will be protected by existing arch_kexec_protect_crashkres() mechanism, >> we naturally protect vmcoreinfo_data from write(even read) access under >> kernel direct mapping after kdump is loaded. >> >> Since kdump is usually loaded at the very early stage after boot, we can >> trust the correctness of the vmcoreinfo data copied. >> >> On the other hand, we still need to operate the vmcoreinfo safe copy when >> crash happens to generate vmcoreinfo_note again, we rely on vmap() to map >> out a new kernel virtual address and update to use this new one instead in >> the following crash_save_vmcoreinfo(). >> >> BTW, we do not touch vmcoreinfo_note, because it will be fully updated >> using the protected vmcoreinfo_data after crash which is surely correct >> just like the cpu crash note. >> >> Cc: Michael Holzheu>> Signed-off-by: Xunlei Pang >> --- >> v3->v4: >> -Rebased on the latest linux-next >> -Copy vmcoreinfo after machine_kexec_prepare() >> >> include/linux/crash_core.h | 2 +- >> include/linux/kexec.h | 2 ++ >> kernel/crash_core.c| 17 - >> kernel/kexec.c | 8 >> kernel/kexec_core.c| 39 +++ >> kernel/kexec_file.c| 8 >> 6 files changed, 74 insertions(+), 2 deletions(-) >> >> diff --git a/include/linux/crash_core.h b/include/linux/crash_core.h >> index 7d6bc7b..5469adb 100644 >> --- a/include/linux/crash_core.h >> +++ b/include/linux/crash_core.h >> @@ -23,6 +23,7 @@ >> >> typedef u32 note_buf_t[CRASH_CORE_NOTE_BYTES/4]; >> >> +void crash_update_vmcoreinfo_safecopy(void *ptr); >> void crash_save_vmcoreinfo(void); >> void arch_crash_save_vmcoreinfo(void); >> __printf(1, 2) >> @@ -54,7 +55,6 @@ >> vmcoreinfo_append_str("PHYS_BASE=%lx\n", (unsigned long)value) >> >> extern u32 *vmcoreinfo_note; >> -extern size_t vmcoreinfo_size; >> >> Elf_Word *append_elf_note(Elf_Word *buf, char *name, unsigned int type, >>void *data, size_t data_len); >> diff --git a/include/linux/kexec.h b/include/linux/kexec.h >> index c9481eb..3ea8275 100644 >> --- a/include/linux/kexec.h >> +++ b/include/linux/kexec.h >> @@ -181,6 +181,7 @@ struct kimage { >> unsigned long start; >> struct page *control_code_page; >> struct page *swap_page; >> +void *vmcoreinfo_data_copy; /* locates in the crash memory */ >> >> unsigned long nr_segments; >> struct kexec_segment segment[KEXEC_SEGMENT_MAX]; >> @@ -250,6 +251,7 @@ extern void *kexec_purgatory_get_symbol_addr(struct >> kimage *image, >> int kexec_should_crash(struct task_struct *); >> int kexec_crash_loaded(void); >> void crash_save_cpu(struct pt_regs *regs, int cpu); >> +extern int kimage_crash_copy_vmcoreinfo(struct kimage *image); >> >> extern struct kimage *kexec_image; >> extern struct kimage *kexec_crash_image; >> diff --git a/kernel/crash_core.c b/kernel/crash_core.c >> index 43cdb00..a29e9ad 100644 >> --- a/kernel/crash_core.c >> +++ b/kernel/crash_core.c >> @@ -15,9 +15,12 @@ >> >> /* vmcoreinfo stuff */ >> static unsigned char *vmcoreinfo_data; >> -size_t vmcoreinfo_size; >> +static size_t vmcoreinfo_size; >> u32 *vmcoreinfo_note; >> >> +/* trusted vmcoreinfo, e.g. we can make a copy in the crash memory */ > May make it clearer like: > /* Trusted vmcoreinfo copy in the kdump reserved memory */ My thought is that it is in crash_core.c now which should be independent of kexec/kdump, so I used "e.g. ..." just like one use case. > >> +static unsigned char *vmcoreinfo_data_safecopy; >> + >> /* >> * parsing the "crashkernel" commandline >> * >> @@ -323,11 +326,23 @@ static void update_vmcoreinfo_note(void) >> final_note(buf); >> } >> >> +void crash_update_vmcoreinfo_safecopy(void *ptr) >> +{ >> +if (ptr) >> +
Re: [PATCH v4 3/3] kdump: Protect vmcoreinfo data under the crash memory
On 04/20/17 at 07:39pm, Xunlei Pang wrote: > Currently vmcoreinfo data is updated at boot time subsys_initcall(), > it has the risk of being modified by some wrong code during system > is running. > > As a result, vmcore dumped may contain the wrong vmcoreinfo. Later on, > when using "crash", "makedumpfile", etc utility to parse this vmcore, > we probably will get "Segmentation fault" or other unexpected errors. > > E.g. 1) wrong code overwrites vmcoreinfo_data; 2) further crashes the > system; 3) trigger kdump, then we obviously will fail to recognize the > crash context correctly due to the corrupted vmcoreinfo. > > Now except for vmcoreinfo, all the crash data is well protected(including > the cpu note which is fully updated in the crash path, thus its correctness > is guaranteed). Given that vmcoreinfo data is a large chunk prepared for > kdump, we better protect it as well. > > To solve this, we relocate and copy vmcoreinfo_data to the crash memory > when kdump is loading via kexec syscalls. Because the whole crash memory > will be protected by existing arch_kexec_protect_crashkres() mechanism, > we naturally protect vmcoreinfo_data from write(even read) access under > kernel direct mapping after kdump is loaded. > > Since kdump is usually loaded at the very early stage after boot, we can > trust the correctness of the vmcoreinfo data copied. > > On the other hand, we still need to operate the vmcoreinfo safe copy when > crash happens to generate vmcoreinfo_note again, we rely on vmap() to map > out a new kernel virtual address and update to use this new one instead in > the following crash_save_vmcoreinfo(). > > BTW, we do not touch vmcoreinfo_note, because it will be fully updated > using the protected vmcoreinfo_data after crash which is surely correct > just like the cpu crash note. > > Cc: Michael Holzheu> Signed-off-by: Xunlei Pang > --- > v3->v4: > -Rebased on the latest linux-next > -Copy vmcoreinfo after machine_kexec_prepare() > > include/linux/crash_core.h | 2 +- > include/linux/kexec.h | 2 ++ > kernel/crash_core.c| 17 - > kernel/kexec.c | 8 > kernel/kexec_core.c| 39 +++ > kernel/kexec_file.c| 8 > 6 files changed, 74 insertions(+), 2 deletions(-) > > diff --git a/include/linux/crash_core.h b/include/linux/crash_core.h > index 7d6bc7b..5469adb 100644 > --- a/include/linux/crash_core.h > +++ b/include/linux/crash_core.h > @@ -23,6 +23,7 @@ > > typedef u32 note_buf_t[CRASH_CORE_NOTE_BYTES/4]; > > +void crash_update_vmcoreinfo_safecopy(void *ptr); > void crash_save_vmcoreinfo(void); > void arch_crash_save_vmcoreinfo(void); > __printf(1, 2) > @@ -54,7 +55,6 @@ > vmcoreinfo_append_str("PHYS_BASE=%lx\n", (unsigned long)value) > > extern u32 *vmcoreinfo_note; > -extern size_t vmcoreinfo_size; > > Elf_Word *append_elf_note(Elf_Word *buf, char *name, unsigned int type, > void *data, size_t data_len); > diff --git a/include/linux/kexec.h b/include/linux/kexec.h > index c9481eb..3ea8275 100644 > --- a/include/linux/kexec.h > +++ b/include/linux/kexec.h > @@ -181,6 +181,7 @@ struct kimage { > unsigned long start; > struct page *control_code_page; > struct page *swap_page; > + void *vmcoreinfo_data_copy; /* locates in the crash memory */ > > unsigned long nr_segments; > struct kexec_segment segment[KEXEC_SEGMENT_MAX]; > @@ -250,6 +251,7 @@ extern void *kexec_purgatory_get_symbol_addr(struct > kimage *image, > int kexec_should_crash(struct task_struct *); > int kexec_crash_loaded(void); > void crash_save_cpu(struct pt_regs *regs, int cpu); > +extern int kimage_crash_copy_vmcoreinfo(struct kimage *image); > > extern struct kimage *kexec_image; > extern struct kimage *kexec_crash_image; > diff --git a/kernel/crash_core.c b/kernel/crash_core.c > index 43cdb00..a29e9ad 100644 > --- a/kernel/crash_core.c > +++ b/kernel/crash_core.c > @@ -15,9 +15,12 @@ > > /* vmcoreinfo stuff */ > static unsigned char *vmcoreinfo_data; > -size_t vmcoreinfo_size; > +static size_t vmcoreinfo_size; > u32 *vmcoreinfo_note; > > +/* trusted vmcoreinfo, e.g. we can make a copy in the crash memory */ May make it clearer like: /* Trusted vmcoreinfo copy in the kdump reserved memory */ > +static unsigned char *vmcoreinfo_data_safecopy; > + > /* > * parsing the "crashkernel" commandline > * > @@ -323,11 +326,23 @@ static void update_vmcoreinfo_note(void) > final_note(buf); > } > > +void crash_update_vmcoreinfo_safecopy(void *ptr) > +{ > + if (ptr) > + memcpy(ptr, vmcoreinfo_data, vmcoreinfo_size); > + > + vmcoreinfo_data_safecopy = ptr; > +} > + > void crash_save_vmcoreinfo(void) > { > if (!vmcoreinfo_note) > return; > > + /* Use the safe copy to generate vmcoreinfo note if have */ > + if