[Crash-utility] [PATCH v2 0/2] Fix KASLR problem on sadump

[Takao Indoh]

Hi Dave,

Thanks for your comments.

On Mon, Oct 16, 2017 at 04:56:01PM -0400, Dave Anderson wrote:
> 
> Hi Takao,
> 
> A couple things.  Can you post against the current github sources,
> or at least use crash-7.2.0?  I'm not sure what version you're 
> working with:
>   
>   $ patch -p1 --dry-run < $dl/sadump_1.patch
>   checking file defs.h
>   Hunk #1 succeeded at 5629 (offset 7 lines).
>   checking file x86_64.c
>   Hunk #1 FAILED at 119.
>   Hunk #2 succeeded at 1998 (offset 34 lines).
>   Hunk #3 succeeded at 2016 (offset 34 lines).
>   Hunk #4 succeeded at 2030 (offset 34 lines).
>   Hunk #5 succeeded at 2086 (offset 34 lines).
>   1 out of 5 hunks FAILED
>   $ patch -p1 --dry-run < $dl/sadump_2.patch
>   checking file defs.h
>   Hunk #1 succeeded at 2590 (offset 5 lines).
>   Hunk #2 succeeded at 6312 (offset 47 lines).
>   checking file sadump.c
>   checking file sadump.h
>   checking file symbols.c
>   Hunk #3 succeeded at 12262 (offset 10 lines).
>   $
>    
> It was easy enough to address the FAILED segment above,

It seems that I made patches on old branch :-(
I'll rebase them.

> but there are a few issues with the sadump.c patch:  
>   
>   $ make warn
>   ... [ cut ] ...
>   cc -c -g -DX86_64 -DLZO -DSNAPPY -DGDB_7_6  sadump.c -Wall -O2 -Wstrict-prototypes -Wmissing-prototypes -fstack-protector -Wformat-security 
>   sadump.c:1911:1: warning: no previous prototype for ‘get_kaslr_offset_from_vmcoreinfo’ [-Wmissing-prototypes]
>    get_kaslr_offset_from_vmcoreinfo(ulong cr3, ulong orig_kaslr_offset,
>    ^
>   sadump.c: In function ‘sadump_calc_kaslr_offset’:
>   sadump.c:2088:38: warning: ‘scs.Cr3’ may be used uninitialized in this function [-Wmaybe-uninitialized]
>     if (get_kaslr_offset_from_vmcoreinfo(
>                                         ^
>   sadump.c:2044:40: warning: ‘scs.IdtLower’ may be used uninitialized in this function [-Wmaybe-uninitialized]
>     idtr = ((uint64_t)scs.IdtUpper)<<32 | (uint64_t)scs.IdtLower;
>                                           ^
>   sadump.c:2044:10: warning: ‘scs.IdtUpper’ may be used uninitialized in this function [-Wmaybe-uninitialized]
>     idtr = ((uint64_t)scs.IdtUpper)<<32 | (uint64_t)scs.IdtLower;
>   ...
> 

Will fix them in next version.

> But more importantly, the sadump.c patch won't compile against the 
> other architectures, because x86_64_kvtop_pagetable() only exists 
> when crash is built for x86_64.
> 
> Plus I don't particularly like the kvtop "pagetable" argument abstraction
> and new functions that you put in place in x86_64.c just for these 3 rather
> arcane calls from sadump.c.
> 
> I suggest adding a new machdep->flags bit (USE_PT?) which could 
> be temporarily set in sadump.c prior to each of the 3 calls that 
> could be made to the generic kvtop() function.  Then in x86_64_kvtop(),
> check for the new machdep->flag as opposed to the "use_pagetable" argument.
> It may even be possible to use SADUMP() along with some other existing 
> flag instead of creating a new one.  But please just avoid creating the 
> new functions in x86_64.c.

Ok, I'll add UST_PT or something and update patches.

Thanks,
Takao Indoh

> 
> Thanks,
>   Dave
> 
> 
> 
> ----- Original Message -----
> > Hi Dave, Hatayama-san,
> > 
> > Hatayama-san, thanks for your review, I updated may patch.
> > 
> > These patch series fix a problem that crash cannot open a dumpfile which is
> > captured by sadump on KASLR enabled kernel.
> > 
> > When KASLR feature is enabled, a kernel is placed on the memory randomly and
> > therefore crash cannot open a dumpfile because addresses of kernel symbols in
> > vmlinux are different from actual addresses. In the case of kdump,
> > information
> > to get actual address is included in the vmcoreinfo, but dumpfile of sadump
> > does
> > not have such a information.
> > 
> > These patches calculate kaslr offset and phys_base to solve this problem. The
> > basic idea is getting register (IDTR and CR3) from dump header, and calculate
> > kaslr_offset/phys_base using them.
> > 
> > changelog:
> > v2:
> > - Remove virsh-dump part
> > - Change get_vec0_addr style
> > - Some tiny fixes
> > 
> > v1:
> > https://www.redhat.com/archives/crash-utility/2017-October/msg00004.html
> > 
> > Takao Indoh (2):
> >   Introduce x86_64_kvtop_pagetable
> >   Fix a KASLR problem of sadump
> > 
> >  defs.h    |   5 +
> >  sadump.c  | 458
> >  +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
> >  sadump.h  |   1 +
> >  symbols.c |  34 +++++
> >  x86_64.c  |  26 +++-
> >  5 files changed, 522 insertions(+), 2 deletions(-)
> > 
> > --
> > 2.9.5
> > 
> > 
> > --
> > Crash-utility mailing list
> > Crash-utility at redhat.com
> > https://www.redhat.com/mailman/listinfo/crash-utility
> > 
> 
> --
> Crash-utility mailing list
> Crash-utility at redhat.com
> https://www.redhat.com/mailman/listinfo/crash-utility