Re: [Crash-utility] [PATCH 2/2] arm64: make crash CONFIG_KASAN_HW_TAGS aware

Hi Pingfan On Thu, Apr 29, 2021 at 3:35 PM piliu <piliu(a)redhat.com&gt; wrote:

Okay I think I got your concern now. Actually when I started implementing the patchset PAC mask was already present in crash utility. But the issue with that was it worked only with vmcoreinfo and not raw ramdumps. So the first patch created a machdep option tasg_mask. When doing that I made it a generic tag_mask and not pac_mask to avoid the need for 2 different machdep options, one later for MTE, essentially doing similar things. So that's the reason why LR/PC modifications are seen, which is for PAC. Do you really think I need to split into 2 different machdep options or we do it later when we find an absolute need for it ? AFAICT it does not break anything and takes care of MTE, PAC, MTE+PAC scenarios.

>>> Thanks, >>> Pingfan >>>>> >>>>> " >>>>> please wait... (gathering kmem slab cache data) >>>>> crash: invalid kernel virtual address: f0ffff878000201c type: > > This bug should be caused by MTE, i.e. a tagged pointer. >>>>> "kmem_cache objsize/object_size" >>>>> crash: get_active_set: no tasks found? >>>>> please wait... (gathering task table data) >>>>> crash: invalid kernel virtual address: f1ffff87f51e3530 type: >>>>> "xa_node shift" >>>>> " >>>>> >>>>> Make the mask introduced for pointer authentication generic >>>>> and use it in vtop and kvaddr validation. >>>>> >>>>> Signed-off-by: Vinayak Menon <vinayakm.list(a)gmail.com&gt; >>>>> --- >>>>> arm64.c | 50 +++++++++++++++++++++++++++++++------------------- >>>>> defs.h | 2 +- >>>>> 2 files changed, 32 insertions(+), 20 deletions(-) >>>>> >>>>> diff --git a/arm64.c b/arm64.c >>>>> index 5b59972..bb41cbb 100644 >>>>> --- a/arm64.c >>>>> +++ b/arm64.c >>>>> @@ -85,7 +85,8 @@ 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); >>>>> +static int arm64_is_kvaddr(ulong); >>>>> +static void arm64_calc_KERNELTAGMASK(void); >>>>> /* >>>>> @@ -215,7 +216,7 @@ arm64_init(int when) >>>>> machdep->pagemask = ~((ulonglong)machdep->pageoffset); >>>>> arm64_calc_VA_BITS(); >>>>> - arm64_calc_KERNELPACMASK(); >>>>> + arm64_calc_KERNELTAGMASK(); >>>>> ms = machdep->machspec; >>>>> if (ms->VA_BITS_ACTUAL) { >>>>> ms->page_offset = ARM64_PAGE_OFFSET_ACTUAL; >>>>> @@ -228,7 +229,7 @@ arm64_init(int when) >>>>> machdep->kvbase = ARM64_VA_START; >>>>> ms->userspace_top = ARM64_USERSPACE_TOP; >>>>> } >>>>> - machdep->is_kvaddr = generic_is_kvaddr; >>>>> + machdep->is_kvaddr = arm64_is_kvaddr; >>>>> machdep->kvtop = arm64_kvtop; >>>>> if (machdep->flags & NEW_VMEMMAP) { >>>>> struct syment *sp; >>>>> @@ -477,7 +478,7 @@ arm64_init(int when) >>>>> case LOG_ONLY: >>>>> machdep->machspec = &arm64_machine_specific; >>>>> arm64_calc_VA_BITS(); >>>>> - arm64_calc_KERNELPACMASK(); >>>>> + arm64_calc_KERNELTAGMASK(); >>>>> arm64_calc_phys_offset(); >>>>> machdep->machspec->page_offset = ARM64_PAGE_OFFSET; >>>>> arm64_calc_physvirt_offset(); >>>>> @@ -608,7 +609,7 @@ arm64_dump_machdep_table(ulong arg) >>>>> fprintf(fp, " dis_filter: arm64_dis_filter()\n"); >>>>> fprintf(fp, " cmd_mach: arm64_cmd_mach()\n"); >>>>> fprintf(fp, " get_smp_cpus: arm64_get_smp_cpus()\n"); >>>>> - fprintf(fp, " is_kvaddr: generic_is_kvaddr()\n"); >>>>> + fprintf(fp, " is_kvaddr: arm64_is_kvaddr()\n"); >>>>> fprintf(fp, " is_uvaddr: arm64_is_uvaddr()\n"); >>>>> fprintf(fp, " value_to_symbol: >>>>> generic_machdep_value_to_symbol()\n"); >>>>> fprintf(fp, " init_kernel_pgd: arm64_init_kernel_pgd\n"); >>>>> @@ -668,9 +669,9 @@ arm64_dump_machdep_table(ulong arg) >>>>> fprintf(fp, "%ld\n", ms->VA_BITS_ACTUAL); >>>>> else >>>>> fprintf(fp, "(unused)\n"); >>>>> - fprintf(fp, "CONFIG_ARM64_KERNELPACMASK: "); >>>>> - if (ms->CONFIG_ARM64_KERNELPACMASK) >>>>> - fprintf(fp, "%lx\n", ms->CONFIG_ARM64_KERNELPACMASK); >>>>> + fprintf(fp, "CONFIG_ARM64_KERNELTAGMASK: "); >>>>> + if (ms->CONFIG_ARM64_KERNELTAGMASK) >>>>> + fprintf(fp, "%lx\n", ms->CONFIG_ARM64_KERNELTAGMASK); >>>>> else >>>>> fprintf(fp, "(unused)\n"); >>>>> fprintf(fp, " userspace_top: %016lx\n", ms->userspace_top); >>>>> @@ -1208,6 +1209,9 @@ arm64_kvtop(struct task_context *tc, ulong >>>>> kvaddr, physaddr_t *paddr, int verbos >>>>> if (!IS_KVADDR(kvaddr)) >>>>> return FALSE; >>>>> + if (kvaddr & (1UL << 63)) >>>>> + kvaddr |= machdep->machspec->CONFIG_ARM64_KERNELTAGMASK; >>>>> + >>>>> if (!vt->vmalloc_start) { >>>>> *paddr = VTOP(kvaddr); >>>>> return TRUE; >>>>> @@ -1828,7 +1832,7 @@ arm64_is_kernel_exception_frame(struct bt_info >>>>> *bt, ulong stkptr) >>>>> if (INSTACK(regs->sp, bt) && INSTACK(regs->regs[29], bt) && >>>>> !(regs->pstate & (0xffffffff00000000ULL | PSR_MODE32_BIT)) && >>>>> is_kernel_text(regs->pc) && >>>>> - is_kernel_text(regs->regs[30] | >>>>> ms->CONFIG_ARM64_KERNELPACMASK)) { >>>>> + is_kernel_text(regs->regs[30] | >>>>> ms->CONFIG_ARM64_KERNELTAGMASK)) { >>>>> switch (regs->pstate & PSR_MODE_MASK) >>>>> { >>>>> case PSR_MODE_EL1t: >>>>> @@ -2198,8 +2202,8 @@ arm64_unwind_frame(struct bt_info *bt, struct >>>>> arm64_stackframe *frame) >>>>> frame->sp = fp + 0x10; >>>>> frame->fp = GET_STACK_ULONG(fp); >>>>> frame->pc = GET_STACK_ULONG(fp + 8); >>>>> - if (is_kernel_text(frame->pc | ms->CONFIG_ARM64_KERNELPACMASK)) >>>>> - frame->pc |= ms->CONFIG_ARM64_KERNELPACMASK; >>>>> + if (is_kernel_text(frame->pc | ms->CONFIG_ARM64_KERNELTAGMASK)) >>>>> + frame->pc |= ms->CONFIG_ARM64_KERNELTAGMASK; >>>>> if ((frame->fp == 0) && (frame->pc == 0)) >>>>> return FALSE; >>>>> @@ -2869,8 +2873,8 @@ arm64_print_text_symbols(struct bt_info *bt, >>>>> struct arm64_stackframe *frame, FIL >>>>> for (i = (start - bt->stackbase)/sizeof(ulong); i < >>>>> LONGS_PER_STACK; i++) { >>>>> up = (ulong *)(&bt->stackbuf[i*sizeof(ulong)]); >>>>> val = *up; >>>>> - if (is_kernel_text(val | ms->CONFIG_ARM64_KERNELPACMASK)) { >>>>> - val |= ms->CONFIG_ARM64_KERNELPACMASK; >>>>> + if (is_kernel_text(val | ms->CONFIG_ARM64_KERNELTAGMASK)) { >>>>> + val |= ms->CONFIG_ARM64_KERNELTAGMASK; >>>>> name = closest_symbol(val); >>>>> fprintf(ofp, " %s[%s] %s at %lx", >>>>> bt->flags & BT_ERROR_MASK ? >>>>> @@ -3205,8 +3209,8 @@ arm64_print_exception_frame(struct bt_info *bt, >>>>> ulong pt_regs, int mode, FILE *o >>>>> rows = 4; >>>>> } else { >>>>> LR = regs->regs[30]; >>>>> - if (is_kernel_text (LR | ms->CONFIG_ARM64_KERNELPACMASK)) >>>>> - LR |= ms->CONFIG_ARM64_KERNELPACMASK; >>>>> + if (is_kernel_text (LR | ms->CONFIG_ARM64_KERNELTAGMASK)) >>>>> + LR |= ms->CONFIG_ARM64_KERNELTAGMASK; > > This looks alike to the scenario on page 17 of > https://events.static.linuxfound.org/sites/events/files/slides/slides_23.pdf > > When LR is saved on frame, it should not be MTE tagged, instead, it is > PAed. Because it is not an object pointer, and a tagged pointer is > generated by explicit code, e.g. > > __kasan_slab_alloc() > { > tag = assign_tag(cache, object, false); > tagged_object = set_tag(object, tag); > } > > I do not see any opportunity for PC/LR to handle by similar code. > > So here, I think it should be something like LR |= > ms->CONFIG_ARM64_KERNELTAGMASK.PAC_SUBMASK. >>>>> SP = regs->sp; >>>>> top_reg = 29; >>>>> is_64_bit = TRUE; >>>>> @@ -4102,6 +4106,14 @@ arm64_calc_virtual_memory_ranges(void) >>>>> } >>>>> static int >>>>> +arm64_is_kvaddr(ulong addr) >>>>> +{ >>>>> + if (addr & (1UL << 63)) >>>>> + addr |= machdep->machspec->CONFIG_ARM64_KERNELTAGMASK; >>>>> + return generic_is_kvaddr(addr); >>>>> +} >>>>> + >>>>> +static int >>>>> arm64_is_uvaddr(ulong addr, struct task_context *tc) >>>>> { >>>>> return (addr < machdep->machspec->userspace_top); >>>>> @@ -4129,21 +4141,21 @@ arm64_swp_offset(ulong pte) >>>>> return pte; >>>>> } >>>>> -static void arm64_calc_KERNELPACMASK(void) >>>>> +static void arm64_calc_KERNELTAGMASK(void) >>>>> { >>>>> ulong value = 0; >>>>> char *string; >>>>> - if ((string = pc->read_vmcoreinfo("NUMBER(KERNELPACMASK)"))) { >>>>> + if ((string = pc->read_vmcoreinfo("NUMBER(KERNELTAGMASK)"))) { >>>> >>>> I git grep 5.12 kernel, but did not find this. Do you plan to do it in >>>> kernel firstly? >>>> > Again, this does occur in v5.12 kernel. But I guess you plan to export > both MTE and PAC mask through one interface. > > But I think the usage should be separated. As above mentioned. > > Sincerely hope you can address my concerns, due to the unavailable MTE > machine to test and verify my thoughts. Trying to understand this better. Without MTE or PAC or both, it is just a matter of using the proper tasg_mask value ? Ideally it should work for any of these combinations.