1:16 a.m.
Hi Pingfan
On Thu, Apr 29, 2021 at 3:35 PM piliu <piliu(a)redhat.com> wrote:
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(a)redhat.com> 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/l...
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_Mem...,
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.
>>
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>
>>>> ---
>>>> 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.
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(a)redhat.com
>>>> https://listman.redhat.com/mailman/listinfo/crash-utility
>>>>
>>>
>>> --
>>> Crash-utility mailing list
>>> Crash-utility(a)redhat.com
>>> https://listman.redhat.com/mailman/listinfo/crash-utility
>>
>
> --
> Crash-utility mailing list
> Crash-utility(a)redhat.com
> https://listman.redhat.com/mailman/listinfo/crash-utility
>