[Crash-utility] [PATCH] Determine the ARM64 kernel's Pointer Authentication mask value by reading the new KERNELPACMASK vmcoreinfo entry.

[Dave Anderson]

Hi Amit,

I have a few suggestions and a question regarding your patch.

First, these two warnings need to be addressed:

$ make warn
...
cc -c -g -DARM64 -DLZO -DSNAPPY -DGDB_7_6  arm64.c -Wall -O2 -Wstrict-prototypes -Wmissing-prototypes -fstack-protector -Wformat-security 
arm64.c: In function ‘arm64_calc_KERNELPACMASK’:
arm64.c:4090:2: warning: format ‘%llx’ expects argument of type ‘long long unsigned int’, but argument 3 has type ‘ulong’ [-Wformat=]
  fprintf(fp, " got NUMBER(KERNELPACMASK) =%llx\n", value);
  ^
arm64.c:4090:9: warning: ‘value’ may be used uninitialized in this function [-Wmaybe-uninitialized]
  fprintf(fp, " got NUMBER(KERNELPACMASK) =%llx\n", value);
         ^
...

The message should be moved inside the if statement, and also should be gated 
with CRASHDEBUG(1) to prevent it from being displayed unconditionally:
  
  static void 
  arm64_calc_KERNELPACMASK(void)
  {
          ulong value;
          char *string;
  
          if ((string = pc->read_vmcoreinfo("NUMBER(KERNELPACMASK)"))) {
                  value = htol(string, QUIET, NULL);
                  free(string);
                  machdep->machspec->CONFIG_ARM64_KERNELPACMASK = value;
                  if (CRASHDEBUG(1))
                          fprintf(fp, "CONFIG_ARM64_KERNELPACMASK: %lx\n", value);
          }
  }
  
And the CONFIG_ARM64_KERNELPACMASK value should be displayed in arm64_dump_machdep_table().

But given that the patch only modifies text return addresses on the kernel
stack is here:

@@ -1932,6 +1936,9 @@ arm64_print_stackframe_entry(struct bt_info *bt, int level, struct arm64_stackfr
          * See, for example, "bl schedule" before ret_to_user().
          */

        branch_pc = frame->pc - 4;
+       if (ms->CONFIG_ARM64_KERNELPACMASK)
+               branch_pc |= ms->CONFIG_ARM64_KERNELPACMASK;
+
         name = closest_symbol(branch_pc);
         name_plus_offset = NULL;

I'm wondering how all of the other places that check addresses found
on the kernel stack will work?  For example, all of these places 
check whether an address found on the stack is a kernel text address:

  $ grep is_kernel_text arm64.c
  	      is_kernel_text(regs->pc) &&
	      is_kernel_text(regs->regs[30])) {
		  if (is_kernel_text(*up)) {   <===== from arm64_print_text_symbols()
		  if (is_kernel_text(frame->pc) ||
			  if (is_kernel_text(frame->pc) && 
		  if (is_kernel_text(regs->pc) &&
		  if (is_kernel_text(LR) &&
	  if (is_kernel_text(regs->pc) && (bt->flags & BT_LINE_NUMBERS)) {
  $

Except for the call arm64_print_text_symbols(), these are checking register 
values found in exception frames.  Can you confirm that they will still be
unmodified kernel text addresses?

The call from arm64_print_text_symbols() is for "bt -[tT]", which just
scours a kernel stack for text addresses and dumps them.  So presumably
that needs to apply the mask to each stack value as you've done in 
arm64_print_stackframe_entry()?  (while still recognizing unmodified text
addresses found in exception frames)

Thanks,
  Dave


----- Original Message -----
> This value is used to mask the PAC bits and generate correct backtrace.
> (amit.kachhap at arm.com)
> ---
> The kernel version for the corresponding vmcoreinfo entry is posted here[1].
> 
> [1]: https://lore.kernel.org/patchwork/patch/1211981/
> 
>  arm64.c | 20 ++++++++++++++++++++
>  defs.h  |  1 +
>  2 files changed, 21 insertions(+)
> 
> diff --git a/arm64.c b/arm64.c
> index 09b1b76..55e084f 100644
> --- a/arm64.c
> +++ b/arm64.c
> @@ -84,6 +84,7 @@ static int arm64_get_kvaddr_ranges(struct vaddr_range *);
>  static void arm64_get_crash_notes(void);
>  static void arm64_calc_VA_BITS(void);
>  static int arm64_is_uvaddr(ulong, struct task_context *);
> +static void arm64_calc_KERNELPACMASK(void);
>  
>  
>  /*
> @@ -213,6 +214,7 @@ arm64_init(int when)
>  		machdep->pagemask = ~((ulonglong)machdep->pageoffset);
>  
>  		arm64_calc_VA_BITS();
> +		arm64_calc_KERNELPACMASK();
>  		ms = machdep->machspec;
>  		if (ms->VA_BITS_ACTUAL) {
>  			ms->page_offset = ARM64_PAGE_OFFSET_ACTUAL;
> @@ -472,6 +474,7 @@ arm64_init(int when)
>  	case LOG_ONLY:
>  		machdep->machspec = &arm64_machine_specific;
>  		arm64_calc_VA_BITS();
> +		arm64_calc_KERNELPACMASK();
>  		arm64_calc_phys_offset();
>  		machdep->machspec->page_offset = ARM64_PAGE_OFFSET;
>  		break;
> @@ -1925,6 +1928,7 @@ arm64_print_stackframe_entry(struct bt_info *bt, int
> level, struct arm64_stackfr
>  	struct syment *sp;
>  	struct load_module *lm;
>  	char buf[BUFSIZE];
> +	struct machine_specific *ms = machdep->machspec;
>  
>          /*
>           * if pc comes from a saved lr, it actually points to an instruction
> @@ -1932,6 +1936,9 @@ arm64_print_stackframe_entry(struct bt_info *bt, int
> level, struct arm64_stackfr
>           * See, for example, "bl schedule" before ret_to_user().
>           */
>  	branch_pc = frame->pc - 4;
> +	if (ms->CONFIG_ARM64_KERNELPACMASK)
> +		branch_pc |= ms->CONFIG_ARM64_KERNELPACMASK;
> +
>          name = closest_symbol(branch_pc);
>          name_plus_offset = NULL;
>  
> @@ -4070,6 +4077,19 @@ arm64_swp_offset(ulong pte)
>  	return pte;
>  }
>  
> +static void arm64_calc_KERNELPACMASK(void)
> +{
> +	ulong value;
> +	char *string;
> +
> +	if ((string = pc->read_vmcoreinfo("NUMBER(KERNELPACMASK)"))) {
> +		value = htol(string, QUIET, NULL);
> +		free(string);
> +		machdep->machspec->CONFIG_ARM64_KERNELPACMASK = value;
> +	}
> +	fprintf(fp, " got NUMBER(KERNELPACMASK) =%llx\n", value);
> +}
> +
>  #endif  /* ARM64 */
>  
>  
> diff --git a/defs.h b/defs.h
> index a3f828d..f37a957 100644
> --- a/defs.h
> +++ b/defs.h
> @@ -3269,6 +3269,7 @@ struct machine_specific {
>  	ulong machine_kexec_end;
>  	ulong VA_BITS_ACTUAL;
>  	ulong CONFIG_ARM64_VA_BITS;
> +	ulong CONFIG_ARM64_KERNELPACMASK;
>  	ulong VA_START;
>  };
>  
> --
> 2.7.4
> 
>