[Crash-utility] [PATCH v2 0/4] Generalize KASLR calculation and use it for KDUMPs

[Sergio Lopez]

On Mon, Mar 19, 2018 at 10:11:52AM -0400, Dave Anderson wrote:
> 
> 
> ----- Original Message -----
> > On Fri, Mar 16, 2018 at 04:33:41PM -0400, Dave Anderson wrote:
> > >   
> > > Hi Sergio,
> > > 
> > > A few initial comments/questions/concerns about this patch...
> > > 
> > > > diff --git a/diskdump.c b/diskdump.c
> > > > index b08a46c..1ec4bcf 100644
> > > > --- a/diskdump.c
> > > > +++ b/diskdump.c
> > > > @@ -56,6 +56,7 @@ struct diskdump_data {
> > > >          void        **nt_prstatus_percpu;
> > > >          uint        num_prstatus_notes;
> > > >          void        **nt_qemu_percpu;
> > > > +        void        **nt_qemucs_percpu;
> > > >          uint        num_qemu_notes;
> > > > 
> > > >          /* page cache */
> > > > @@ -72,6 +73,7 @@ struct diskdump_data {
> > > >          ulong  *valid_pages;
> > > >          ulong   accesses;
> > > >          ulong        snapshot_task;
> > > > +        ulong        kaslr_phys_base;
> > > >  };
> > > 
> > > Generally speaking, there already is an sd->phys_base, and you've
> > > added an nd->phys_base, but I don't understand why you also added
> > > a new dd->kaslr_phys_base member and new diskdump_kaslr_phys_base()
> > > function?  Is there any reason that you can't continue to use the
> > > currently-existing dd->sub_header_kdump->phys_base member and the
> > > diskdump_phys_base() function?  I just find the concept of a
> > > "kaslr_phys_base" confusing, i.e., as if there are two different
> > > types of phys_base in the kernel.  Can you please try to utilize
> > > the existing member and function?
> > > 
> > > Also related, your diskdump_kaslr_phys_base() and kdump_phys_base()
> > > functions
> > > don't account for (return FALSE) with a legitimate phys_base value of 0.
> > > In fact I have a sample RHEL7 ELF vmcore generated by virsh dump which
> > > has a phys_base of 0.  More on that below...
> > 
> > Both of these are the consequence of trying to avoid changing the current
> > SADUMP's implementation while keeping NETDUMP's and DISKDUMP's as similar as
> > possible.
> > 
> > But if there's no problem in changing SADUMP's, I would change the setters to
> > succeed unconditionally, and remove the "if (base->phys_base)" check from the
> > getters. This would make them semantically equivalent to current's
> > "diskdump_phys_base", which is the main reason I've added kaslr_phys_base,
> > allowing me to use the existing dd->sub_header_kdump->phys_base.
> 
> No, I would leave the SADUMP implementation as it is.  First, I don't maintain it,
> and secondly, apparently the hardware that it runs on always has a non-zero
> phys_base?  I really don't know.  The sadump maintainers will be ACK'ing this
> patchset as well, so I will leave it up to them.
> 
> But diskdump_phys_base() and kdump_phys_base() -- as long as the dumpfile type is
> verified -- should return whatever is there, including zero, regardless whether 
> it is the initialization value or set legitimately.  The subsequent call to 
> x86_64_virt_phys_base() will either verify it, or if lucky, calculate it += 16MB.
> 
> Make sense?

Sure, no problem.  

> > 
> > > > diff --git a/kaslr_helper.c b/kaslr_helper.c
> > > > index 1079863..5b71e3e 100644
> > > > --- a/kaslr_helper.c
> > > > +++ b/kaslr_helper.c
> > > > @@ -386,6 +386,9 @@ calc_kaslr_offset(ulong *kaslr_offset, ulong
> > > > *phys_base)
> > > >  		if (KDUMP_DUMPFILE()) {
> > > >  			idtr = kdump_get_idtr();
> > > >  			cr3 = kdump_get_cr3();
> > > > +		} else if (DISKDUMP_DUMPFILE()) {
> > > > +			idtr = diskdump_get_idtr();
> > > > +			cr3 = diskdump_get_cr3();
> > > 
> > > All 4 of these new functions above can fail and return 0.  Probably
> > > unlikely, but shouldn't there be a FALSE return if either one is 0,
> > > rather than continuing and using them?
> > 
> > Again, I was trying to keep in sync with current SADUMP's implementation.
> 
> Yeah, you're right -- looking at the current sadump implementation, it looks 
> like even though its get_sadump_smram_cpu_state_any() function can return FALSE,
> sadump_calc_kaslr_offset() doesn't bother to check it.
>  
> > Otherwise, I'd prefer implementing a single function like this:
> > 
> > int [diskdump|kdump|sadump]_get_idtr_cr3(uint64_t *idtr, uint64_t *cr3);
> > 
> > That would allow me to write some like:
> > 
> > if ((KDUMP_DUMPFILE() && !kdump_get_idtr_cr3(&idtr, &cr3)) ||
> >     (DISKDUMP_DUMPFILE() && !diskdump_get_idtr_cr3(&idtr, &cr3))) {
> >         return FALSE;
> > }
> > 
> > What do you think?
> 
> Looks OK -- but since your kdump_get_idtr() is identical to diskdump_get_idtr(),
> and kdump_get_cr3() is identical to diskdump_get_cr3(), why can't the 4 functions
> be merged into a single function, and put in the new kaslr_helper.c?  I understand
> why the two get_qemucpustate() functions exist.

Sounds good to me.

> > 
> > > >  		} else {
> > > >  			return FALSE;
> > > >  		}
> > > 
> > > > diff --git a/x86_64.c b/x86_64.c
> > > > index ed5985a..3c492e4 100644
> > > > --- a/x86_64.c
> > > > +++ b/x86_64.c
> > > > @@ -6632,8 +6632,15 @@ x86_64_calc_phys_base(void)
> > > >  	 *  Get relocation value from whatever dumpfile format is being used.
> > > >  	 */
> > > >  
> > > > -	if (QEMU_MEM_DUMP_NO_VMCOREINFO() && KDUMP_DUMPFILE()) {
> > > > -		if (kdump_phys_base(&phys_base)) {
> > > > +	if (QEMU_MEM_DUMP_NO_VMCOREINFO()) {
> > > > +		int ret;
> > > > +
> > > > +		if (KDUMP_DUMPFILE())
> > > > +			ret = kdump_phys_base(&phys_base);
> > > > +		else if (DISKDUMP_DUMPFILE())
> > > > +			ret = diskdump_kaslr_phys_base(&phys_base);
> > > > +
> > > > +		if (ret) {
> > > >  			machdep->machspec->phys_base = phys_base;
> > > >  			if (CRASHDEBUG(1))
> > > >  				fprintf(fp, "kdump-novmci: phys base: %lx\n",
> > > > --
> > > > 2.14.3
> > > 
> > > This is where the "0 phys_base" issue comes into play.  I think the section
> > > above should do the same thing as the following "if (DISKDUMP_DUMPFILE())"
> > > does, where diskump_phys_base() is only concerned if the dumpfile itself
> > > is valid.  It may return 0 as a phys_base, and that's OK, because it
> > > unconditionally calls x86_64_virt_phys_base() -- which serves a dual
> > > purpose, either to:
> > >  
> > >    (1) verify it, or
> > >    (2) if it's bogus, it checks whether plus-or-minus 16MB works.
> > 
> > After switching to using dd->sub_header_kdump->phys_base, do you think we can
> > just leave QEMU_MEM_DUMP_COMPRESSED to be dealt by the DISKDUMP_DUMPFILE
> > section, and add another equivalent one for QEMU_MEM_DUMP_ELF && !VMCOREINFO?
> 
> I would keep the QEMU_MEM_DUMP_NO_VMCOREINFO() section where it is, because it
> still potentially calls your new kdump_phys_base(). 

Just to be sure, apart from changing [kdump|diskdump]_phys_base to properly
return whatever value they have in their respective phys_base fields, is there
any other change necessary to this section, or is the patch good as it is?

Thanks,
Sergio.