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

[piliu <piliu@xxxxxxxxxx>]

On 5/3/21 2:16 PM, vinayak menon wrote:
> Hi Pingfan
>
> On Thu, Apr 29, 2021 at 3:35 PM piliu <piliu@xxxxxxxxxx> 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.
Yes, please split the patch as each patch has a dedicated purpose.

Also please limit terms to PAC instead of MTE, since this patch tries to 
resolves PAC issue.
> > > > 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@xxxxxxxxx>
> > > > > > ---
> > > > > >     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.
Sorry that I can not totally agree with you.

The tag_mask subfileds should be applied to different things, E.g. here 
PC/LR can not accept MTE subfield.

Thanks,
Pingfan

--
Crash-utility mailing list
Crash-utility@xxxxxxxxxx
https://listman.redhat.com/mailman/listinfo/crash-utility