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

[Dave Anderson]

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...


> 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?
 
>  		} 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.

Thanks,
  Dave