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

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(a)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 > >