DMARC-Filter: OpenDMARC Filter v1.3.2 mx1.redhat.com CA24FC054C4C
Date: Mon, 20 Mar 2017 11:55:09 +0800
From: Baoquan He <bhe@redhat.com>
To: xlpang@redhat.com
Cc: "Eric W. Biederman" <ebiederm@xmission.com>, kexec@lists.infradead.org,
        akpm@linux-foundation.org, Dave Young <dyoung@redhat.com>,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH v2] kexec: Introduce vmcoreinfo signature verification
Message-ID: <20170320035509.GB22469@x1>
References: <1489722318-13695-1-git-send-email-xlpang@redhat.com>
 <874lyrhl81.fsf@xmission.com>
 <20170320021330.GA22469@x1>
 <58CF40E5.6010104@redhat.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <58CF40E5.6010104@redhat.com>
User-Agent: Mutt/1.7.0 (2016-08-17)
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 6885
Lines: 185

On 03/20/17 at 10:39am, Xunlei Pang wrote:
> On 03/20/2017 at 10:13 AM, Baoquan He wrote:
> > On 03/17/17 at 12:22pm, Eric W. Biederman wrote:
> >> Xunlei Pang <xlpang@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
> 
> It's easy to reproduce: write a kernel module to modify part content of
> vmcoreinfo_data (we surely have many ways to acquire its VA). If it does
> exist in theory, we will met it sooner or later in real world due to billions
> of applications.
> 
> Also there are bugs like this one
> https://bugzilla.redhat.com/show_bug.cgi?id=1287097
> Not sure if it is makedumpfile issue or this one, maybe we can't know forever.

Well, kdump is not all-purpose. If you write code in module to stomp
page init_level4_pgt is pointing at, you won't get a vmcore.

And you are saying vmcoreinfo_data, it's a intermediate page, should be
vmcoreinfo_note. If the wrong code you mentioned didn't change
vmcoreinfo_note, but other kernel data which need be saved into
vmcoreinfo_note, crash_save_vmcoreinfo_init is doing better than you
re-saved one.

> 
> Regards,
> Xunlei
> 
> > 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@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;
> 
> 
> _______________________________________________
> kexec mailing list
> kexec@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/kexec