On 03/17/17 at 12:22pm, Eric W. Biederman wrote:
> Xunlei Pang <xlpang at redhat.com> writes:
>
> > 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" or "makedumpfile"(etc) utility to parse this vmcore,
> > we probably will get "Segmentation fault" or other unexpected/confusing
> > errors.
>
> If this is a real concern and the previous discussion sounds like it is
> part of what we need to do is move the variable vmcoreinfo_note out
> of the kernel's .bss section. And modify the code to regenerate
> and keep this information in something like the control page.
I guess this is not from a real issue, just from Xunlei's worry. But
Xunlei didn't give a direct answer to this, and Petr's question. Not
very sure if this will impact other implementation. fadump will be
impacted by this or other dump? Maybe yet or maybe not.
I don't object this strongly, but please at least add code comment to
explain why vmcoreinfo need be saved twice because it does look weird.
>
> Definitely something like this needs a page all to itself, and ideally
> far away from any other kernel data structures. I clearly was not
> watching closely the data someone decided to keep this silly thing
> in the kernel's .bss section.
>
> > As vmcoreinfo is the most fundamental information for vmcore, we better
> > double check its correctness. Here we generate a signature(using crc32)
> > after it is saved, then verify it in crash_save_vmcoreinfo() to see if
> > the signature was broken, if so we have to re-save the vmcoreinfo data
> > to get the correct vmcoreinfo for kdump as possible as we can.
>
> Sigh. We already have a sha256 that is supposed to cover this sort of
> thing. The bug rather is that apparently it isn't covering this data.
> That sounds like what we should be fixing.
>
> Please let's not invent new mechanisms we have to maintain. Let's
> reorganize this so this static data is protected like all other static
> data in the kexec-on-panic path. We have good mechanims and good
> strategies for avoiding and detecting corruption we just need to use
> them.
>
> Eric
>
>
>
> > Signed-off-by: Xunlei Pang <xlpang at redhat.com>
> > ---
> > v1->v2:
> > - Keep crash_save_vmcoreinfo_init() because "makedumpfile --mem-usage"
> > uses the information.
> > - Add crc32 verification for vmcoreinfo, re-save when failure.
> >
> > arch/Kconfig | 1 +
> > kernel/kexec_core.c | 43 +++++++++++++++++++++++++++++++++++--------
> > 2 files changed, 36 insertions(+), 8 deletions(-)
> >
> > diff --git a/arch/Kconfig b/arch/Kconfig
> > index c4d6833..66eb296 100644
> > --- a/arch/Kconfig
> > +++ b/arch/Kconfig
> > @@ -4,6 +4,7 @@
> >
> > config KEXEC_CORE
> > bool
> > + select CRC32
> >
> > config HAVE_IMA_KEXEC
> > bool
> > diff --git a/kernel/kexec_core.c b/kernel/kexec_core.c
> > index bfe62d5..012acbe 100644
> > --- a/kernel/kexec_core.c
> > +++ b/kernel/kexec_core.c
> > @@ -38,6 +38,7 @@
> > #include <linux/syscore_ops.h>
> > #include <linux/compiler.h>
> > #include <linux/hugetlb.h>
> > +#include <linux/crc32.h>
> >
> > #include <asm/page.h>
> > #include <asm/sections.h>
> > @@ -53,9 +54,10 @@
> >
> > /* vmcoreinfo stuff */
> > static unsigned char vmcoreinfo_data[VMCOREINFO_BYTES];
> > -u32 vmcoreinfo_note[VMCOREINFO_NOTE_SIZE/4];
> > +static u32 vmcoreinfo_sig;
> > size_t vmcoreinfo_size;
> > size_t vmcoreinfo_max_size = sizeof(vmcoreinfo_data);
> > +u32 vmcoreinfo_note[VMCOREINFO_NOTE_SIZE/4];
> >
> > /* Flag to indicate we are going to kexec a new kernel */
> > bool kexec_in_progress = false;
> > @@ -1367,12 +1369,6 @@ static void update_vmcoreinfo_note(void)
> > final_note(buf);
> > }
> >
> > -void crash_save_vmcoreinfo(void)
> > -{
> > - vmcoreinfo_append_str("CRASHTIME=%ld\n", get_seconds());
> > - update_vmcoreinfo_note();
> > -}
> > -
> > void vmcoreinfo_append_str(const char *fmt, ...)
> > {
> > va_list args;
> > @@ -1402,7 +1398,7 @@ phys_addr_t __weak paddr_vmcoreinfo_note(void)
> > return __pa_symbol((unsigned long)(char *)&vmcoreinfo_note);
> > }
> >
> > -static int __init crash_save_vmcoreinfo_init(void)
> > +static void do_crash_save_vmcoreinfo_init(void)
> > {
> > VMCOREINFO_OSRELEASE(init_uts_ns.name.release);
> > VMCOREINFO_PAGESIZE(PAGE_SIZE);
> > @@ -1474,6 +1470,37 @@ static int __init crash_save_vmcoreinfo_init(void)
> > #endif
> >
> > arch_crash_save_vmcoreinfo();
> > +}
> > +
> > +static u32 crash_calc_vmcoreinfo_sig(void)
> > +{
> > + return crc32(~0, vmcoreinfo_data, vmcoreinfo_size);
> > +}
> > +
> > +static bool crash_verify_vmcoreinfo(void)
> > +{
> > + if (crash_calc_vmcoreinfo_sig() == vmcoreinfo_sig)
> > + return true;
> > +
> > + return false;
> > +}
> > +
> > +void crash_save_vmcoreinfo(void)
> > +{
> > + /* Re-save if verification fails */
> > + if (!crash_verify_vmcoreinfo()) {
> > + vmcoreinfo_size = 0;
> > + do_crash_save_vmcoreinfo_init();
> > + }
> > +
> > + vmcoreinfo_append_str("CRASHTIME=%ld\n", get_seconds());
> > + update_vmcoreinfo_note();
> > +}
> > +
> > +static int __init crash_save_vmcoreinfo_init(void)
> > +{
> > + do_crash_save_vmcoreinfo_init();
> > + vmcoreinfo_sig = crash_calc_vmcoreinfo_sig();
> > update_vmcoreinfo_note();
> >
> > return 0;
