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

[piliu <piliu@xxxxxxxxxx>]

Hi Vinayak,

After paying some time on MTE and PAC material, I still have concerns on 
your 2/2.

Please see the comment inline.

On 4/27/21 9:47 PM, vinayak menon wrote:
> Hi Pingfan,
>
> On Mon, Apr 26, 2021 at 8:06 PM piliu <piliu@xxxxxxxxxx> wrote:
> > On 4/26/21 7:17 PM, piliu wrote:
> > > Hi Vinayak,
> > >
> > > On 3/30/21 9:52 PM, Vinayak Menon wrote:
> > > > With CONFIG_KASAN_HW_TAGS enabled kvaddr can be tagged
> > > > and this results in readmem, vtop etc. fail like below.
> > Sorry for the limited knowledge of this field. I have more question
> > during the reviewing.
> >
> > -1.is this reproducible with SW TAGS?
> > -2. HW TAGS aims for malloc, while here I see the handling LR, PC.
> > It seems that they are more related to "Pointer Authentication"
>
> I have not tried with SW tags. CONFIG_KASAN_HW_TAGS  can be found here
> in kernel https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/lib/Kconfig.kasan?h=v5.12#n102

Yeah, I know that. Since our lab has no MTE machine to verify your 
patch, so I wonder whether soft tagged kasan can work.
> The MTE based kasan is used for tagging malloc too. With
> CONFIG_KASAN_HW_TAGS it is used to tag kernel memory, replacing the
> regular kasan.
According to 
https://developer.arm.com/-/media/Arm%20Developer%20Community/PDF/Arm_Memory_Tagging_Extension_Whitepaper.pdf,
MTE is used to resolve out-of-range and use-after-free. I can not see 
any relation with PC and LR register, which occurs in this patch.
> > 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.

Thanks,
Pingfan
> > > >            value = htol(string, QUIET, NULL);
> > > >            free(string);
> > > >        } else if (machdep->machspec->tag_mask) {
> > > >            value = machdep->machspec->tag_mask;
> > > >        }
> > > > -    machdep->machspec->CONFIG_ARM64_KERNELPACMASK = value;
> > > > +    machdep->machspec->CONFIG_ARM64_KERNELTAGMASK = value;
> > > >        if (CRASHDEBUG(1))
> > > > -        fprintf(fp, "CONFIG_ARM64_KERNELPACMASK: %lx\n", value);
> > > > +        fprintf(fp, "CONFIG_ARM64_KERNELTAGMASK: %lx\n", value);
> > > >    }
> > > >    #endif  /* ARM64 */
> > > > diff --git a/defs.h b/defs.h
> > > > index d406f5f..770c335 100644
> > > > --- a/defs.h
> > > > +++ b/defs.h
> > > > @@ -3329,7 +3329,7 @@ struct machine_specific {
> > > >        ulong VA_BITS_ACTUAL;
> > > >        ulong CONFIG_ARM64_VA_BITS;
> > > >        ulong VA_START;
> > > > -    ulong CONFIG_ARM64_KERNELPACMASK;
> > > > +    ulong CONFIG_ARM64_KERNELTAGMASK;
> > > >        ulong physvirt_offset;
> > > >        ulong tag_mask;
> > > >    };
> > > >
> > > > --
> > > > Crash-utility mailing list
> > > > Crash-utility@xxxxxxxxxx
> > > > https://listman.redhat.com/mailman/listinfo/crash-utility
> > >
> > > --
> > > Crash-utility mailing list
> > > Crash-utility@xxxxxxxxxx
> > > https://listman.redhat.com/mailman/listinfo/crash-utility
>
> --
> Crash-utility mailing list
> Crash-utility@xxxxxxxxxx
> https://listman.redhat.com/mailman/listinfo/crash-utility

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