Date:   Mon, 20 Mar 2017 10:06:12 +0800
From:   Xunlei Pang <xpang@...hat.com>
To:     "Eric W. Biederman" <ebiederm@...ssion.com>,
        Xunlei Pang <xlpang@...hat.com>
Cc:     Dave Young <dyoung@...hat.com>, akpm@...ux-foundation.org,
        kexec@...ts.infradead.org, linux-kernel@...r.kernel.org,
        Baoquan He <bhe@...hat.com>
Subject: Re: [PATCH v2] kexec: Introduce vmcoreinfo signature verification

On 03/18/2017 at 01:22 AM, Eric W. Biederman wrote:
> Xunlei Pang <xlpang@...hat.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.
>
> 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
>

Yes, this idea looks way better, I will follow your suggestions, thanks!

Regards,
Xunlei

>
>> Signed-off-by: Xunlei Pang <xlpang@...hat.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;
> _______________________________________________
> kexec mailing list
> kexec@...ts.infradead.org
> http://lists.infradead.org/mailman/listinfo/kexec

Powered by blists - more mailing lists