[syzbot] upstream test error: KASAN: invalid-access Read in __entry_tramp_text_end

[syzbot] upstream test error: KASAN: invalid-access Read in __entry_tramp_text_end

[syzbot]

Hello,

syzbot found the following issue on:

HEAD commit:    f1583cb1be35 Merge tag 'linux-kselftest-next-5.15-rc1' of ..
git tree:       upstream
console output: https://syzkaller.appspot.com/x/log.txt?x=16354043300000
kernel config:  https://syzkaller.appspot.com/x/.config?x=5fe535c85e8d7384
dashboard link: https://syzkaller.appspot.com/bug?extid=488ddf8087564d6de6e2
compiler:       aarch64-linux-gnu-gcc (Debian 10.2.1-6) 10.2.1 20210110, GNU ld (GNU Binutils for Debian) 2.35.1
userspace arch: arm64

IMPORTANT: if you fix the issue, please add the following tag to the commit:
Reported-by: syzbot+488ddf8087564d6de6e2@syzkaller.appspotmail.com

==================================================================
BUG: KASAN: invalid-access in __entry_tramp_text_end+0xdfc/0x3000
Read at addr f4ff000002a361a0 by task kdevtmpfs/22
Pointer tag: [f4], memory tag: [fe]

CPU: 1 PID: 22 Comm: kdevtmpfs Not tainted 5.14.0-syzkaller-09284-gf1583cb1be35 #0
Hardware name: linux,dummy-virt (DT)
Call trace:
 dump_backtrace+0x0/0x1ac arch/arm64/kernel/stacktrace.c:76
 show_stack+0x18/0x24 arch/arm64/kernel/stacktrace.c:215
 __dump_stack lib/dump_stack.c:88 [inline]
 dump_stack_lvl+0x68/0x84 lib/dump_stack.c:105
 print_address_description+0x7c/0x2b4 mm/kasan/report.c:256
 __kasan_report mm/kasan/report.c:442 [inline]
 kasan_report+0x134/0x380 mm/kasan/report.c:459
 __do_kernel_fault+0x128/0x1bc arch/arm64/mm/fault.c:317
 do_bad_area arch/arm64/mm/fault.c:466 [inline]
 do_tag_check_fault+0x74/0x90 arch/arm64/mm/fault.c:737
 do_mem_abort+0x44/0xb4 arch/arm64/mm/fault.c:813
 el1_abort+0x40/0x60 arch/arm64/kernel/entry-common.c:357
 el1h_64_sync_handler+0xb0/0xd0 arch/arm64/kernel/entry-common.c:408
 el1h_64_sync+0x78/0x7c arch/arm64/kernel/entry.S:567
 __entry_tramp_text_end+0xdfc/0x3000
 d_lookup+0x44/0x70 fs/dcache.c:2370
 lookup_dcache+0x24/0x84 fs/namei.c:1520
 __lookup_hash+0x24/0xd0 fs/namei.c:1543
 kern_path_locked+0x90/0x10c fs/namei.c:2567
 handle_remove+0x38/0x284 drivers/base/devtmpfs.c:312
 handle drivers/base/devtmpfs.c:382 [inline]
 devtmpfs_work_loop drivers/base/devtmpfs.c:395 [inline]
 devtmpfsd+0x8c/0xd0 drivers/base/devtmpfs.c:437
 kthread+0x150/0x15c kernel/kthread.c:319
 ret_from_fork+0x10/0x20 arch/arm64/kernel/entry.S:756

Allocated by task 22:
 kasan_save_stack+0x28/0x60 mm/kasan/common.c:38
 kasan_set_track mm/kasan/common.c:46 [inline]
 set_alloc_info mm/kasan/common.c:434 [inline]
 __kasan_slab_alloc+0xb0/0x110 mm/kasan/common.c:467
 kasan_slab_alloc include/linux/kasan.h:254 [inline]
 slab_post_alloc_hook mm/slab.h:519 [inline]
 slab_alloc_node mm/slub.c:2959 [inline]
 slab_alloc mm/slub.c:2967 [inline]
 kmem_cache_alloc+0x1cc/0x340 mm/slub.c:2972
 getname_kernel+0x30/0x150 fs/namei.c:226
 kern_path_locked+0x2c/0x10c fs/namei.c:2558
 handle_remove+0x38/0x284 drivers/base/devtmpfs.c:312
 handle drivers/base/devtmpfs.c:382 [inline]
 devtmpfs_work_loop drivers/base/devtmpfs.c:395 [inline]
 devtmpfsd+0x8c/0xd0 drivers/base/devtmpfs.c:437
 kthread+0x150/0x15c kernel/kthread.c:319
 ret_from_fork+0x10/0x20 arch/arm64/kernel/entry.S:756

Freed by task 22:
 kasan_save_stack+0x28/0x60 mm/kasan/common.c:38
 kasan_set_track+0x28/0x3c mm/kasan/common.c:46
 kasan_set_free_info+0x20/0x30 mm/kasan/tags.c:36
 ____kasan_slab_free.constprop.0+0x178/0x1e0 mm/kasan/common.c:366
 __kasan_slab_free+0x10/0x1c mm/kasan/common.c:374
 kasan_slab_free include/linux/kasan.h:230 [inline]
 slab_free_hook mm/slub.c:1628 [inline]
 slab_free_freelist_hook+0xc4/0x20c mm/slub.c:1653
 slab_free mm/slub.c:3213 [inline]
 kmem_cache_free+0x9c/0x420 mm/slub.c:3229
 putname.part.0+0x68/0x7c fs/namei.c:270
 putname include/linux/err.h:41 [inline]
 filename_parentat fs/namei.c:2547 [inline]
 kern_path_locked+0x64/0x10c fs/namei.c:2558
 handle_remove+0x38/0x284 drivers/base/devtmpfs.c:312
 handle drivers/base/devtmpfs.c:382 [inline]
 devtmpfs_work_loop drivers/base/devtmpfs.c:395 [inline]
 devtmpfsd+0x8c/0xd0 drivers/base/devtmpfs.c:437
 kthread+0x150/0x15c kernel/kthread.c:319
 ret_from_fork+0x10/0x20 arch/arm64/kernel/entry.S:756

The buggy address belongs to the object at ffff000002a36180
 which belongs to the cache names_cache of size 4096
The buggy address is located 32 bytes inside of
 4096-byte region [ffff000002a36180, ffff000002a37180)
The buggy address belongs to the page:
page:00000000a105b3ae refcount:1 mapcount:0 mapping:0000000000000000 index:0xf3ff000002a34100 pfn:0x42a30
head:00000000a105b3ae order:3 compound_mapcount:0 compound_pincount:0
flags: 0x1ffc00000010200(slab|head|node=0|zone=0|lastcpupid=0x7ff|kasantag=0x0)
raw: 01ffc00000010200 0000000000000000 dead000000000122 faff000002837700
raw: f3ff000002a34100 0000000080070003 00000001ffffffff 0000000000000000
page dumped because: kasan: bad access detected

Memory state around the buggy address:
 ffff000002a35f00: fe fe fe fe fe fe fe fe fe fe fe fe fe fe fe fe
 ffff000002a36000: fe fe fe fe fe fe fe fe fe fe fe fe fe fe fe fe
>ffff000002a36100: fe fe fe fe fe fe fe fe fe fe fe fe fe fe fe fe
                                                 ^
 ffff000002a36200: fe fe fe fe fe fe fe fe fe fe fe fe fe fe fe fe
 ffff000002a36300: fe fe fe fe fe fe fe fe fe fe fe fe fe fe fe fe
==================================================================


---
This report is generated by a bot. It may contain errors.
See https://goo.gl/tpsmEJ for more information about syzbot.
syzbot engineers can be reached at syzkaller@googlegroups.com.

syzbot will keep track of this issue. See:
https://goo.gl/tpsmEJ#status for how to communicate with syzbot.

Re: [syzbot] upstream test error: KASAN: invalid-access Read in __entry_tramp_text_end

[Dmitry Vyukov]

On Sat, 4 Sept 2021 at 13:57, syzbot
<syzbot+488ddf8087564d6de6e2@syzkaller.appspotmail.com> wrote:
>
> Hello,
>
> syzbot found the following issue on:
>
> HEAD commit:    f1583cb1be35 Merge tag 'linux-kselftest-next-5.15-rc1' of ..
> git tree:       upstream
> console output: https://syzkaller.appspot.com/x/log.txt?x=16354043300000
> kernel config:  https://syzkaller.appspot.com/x/.config?x=5fe535c85e8d7384
> dashboard link: https://syzkaller.appspot.com/bug?extid=488ddf8087564d6de6e2
> compiler:       aarch64-linux-gnu-gcc (Debian 10.2.1-6) 10.2.1 20210110, GNU ld (GNU Binutils for Debian) 2.35.1
> userspace arch: arm64
>
> IMPORTANT: if you fix the issue, please add the following tag to the commit:
> Reported-by: syzbot+488ddf8087564d6de6e2@syzkaller.appspotmail.com
>
> ==================================================================
> BUG: KASAN: invalid-access in __entry_tramp_text_end+0xdfc/0x3000
> Read at addr f4ff000002a361a0 by task kdevtmpfs/22
> Pointer tag: [f4], memory tag: [fe]
>
> CPU: 1 PID: 22 Comm: kdevtmpfs Not tainted 5.14.0-syzkaller-09284-gf1583cb1be35 #0
> Hardware name: linux,dummy-virt (DT)
> Call trace:
>  dump_backtrace+0x0/0x1ac arch/arm64/kernel/stacktrace.c:76
>  show_stack+0x18/0x24 arch/arm64/kernel/stacktrace.c:215
>  __dump_stack lib/dump_stack.c:88 [inline]
>  dump_stack_lvl+0x68/0x84 lib/dump_stack.c:105
>  print_address_description+0x7c/0x2b4 mm/kasan/report.c:256
>  __kasan_report mm/kasan/report.c:442 [inline]
>  kasan_report+0x134/0x380 mm/kasan/report.c:459
>  __do_kernel_fault+0x128/0x1bc arch/arm64/mm/fault.c:317
>  do_bad_area arch/arm64/mm/fault.c:466 [inline]
>  do_tag_check_fault+0x74/0x90 arch/arm64/mm/fault.c:737
>  do_mem_abort+0x44/0xb4 arch/arm64/mm/fault.c:813
>  el1_abort+0x40/0x60 arch/arm64/kernel/entry-common.c:357
>  el1h_64_sync_handler+0xb0/0xd0 arch/arm64/kernel/entry-common.c:408
>  el1h_64_sync+0x78/0x7c arch/arm64/kernel/entry.S:567
>  __entry_tramp_text_end+0xdfc/0x3000

/\/\/\/\/\/\/\

This is broken unwind on arm64. d_lookup statically calls __d_lookup,
not __entry_tramp_text_end (which is not even a function).
See the following thread for some debugging details:
https://lore.kernel.org/lkml/CACT4Y+ZByJ71QfYHTByWaeCqZFxYfp8W8oyrK0baNaSJMDzoUw@mail.gmail.com/

But there is also the use-after-free in d_lookup.

>  d_lookup+0x44/0x70 fs/dcache.c:2370
>  lookup_dcache+0x24/0x84 fs/namei.c:1520
>  __lookup_hash+0x24/0xd0 fs/namei.c:1543
>  kern_path_locked+0x90/0x10c fs/namei.c:2567
>  handle_remove+0x38/0x284 drivers/base/devtmpfs.c:312
>  handle drivers/base/devtmpfs.c:382 [inline]
>  devtmpfs_work_loop drivers/base/devtmpfs.c:395 [inline]
>  devtmpfsd+0x8c/0xd0 drivers/base/devtmpfs.c:437
>  kthread+0x150/0x15c kernel/kthread.c:319
>  ret_from_fork+0x10/0x20 arch/arm64/kernel/entry.S:756
>
> Allocated by task 22:
>  kasan_save_stack+0x28/0x60 mm/kasan/common.c:38
>  kasan_set_track mm/kasan/common.c:46 [inline]
>  set_alloc_info mm/kasan/common.c:434 [inline]
>  __kasan_slab_alloc+0xb0/0x110 mm/kasan/common.c:467
>  kasan_slab_alloc include/linux/kasan.h:254 [inline]
>  slab_post_alloc_hook mm/slab.h:519 [inline]
>  slab_alloc_node mm/slub.c:2959 [inline]
>  slab_alloc mm/slub.c:2967 [inline]
>  kmem_cache_alloc+0x1cc/0x340 mm/slub.c:2972
>  getname_kernel+0x30/0x150 fs/namei.c:226
>  kern_path_locked+0x2c/0x10c fs/namei.c:2558
>  handle_remove+0x38/0x284 drivers/base/devtmpfs.c:312
>  handle drivers/base/devtmpfs.c:382 [inline]
>  devtmpfs_work_loop drivers/base/devtmpfs.c:395 [inline]
>  devtmpfsd+0x8c/0xd0 drivers/base/devtmpfs.c:437
>  kthread+0x150/0x15c kernel/kthread.c:319
>  ret_from_fork+0x10/0x20 arch/arm64/kernel/entry.S:756
>
> Freed by task 22:
>  kasan_save_stack+0x28/0x60 mm/kasan/common.c:38
>  kasan_set_track+0x28/0x3c mm/kasan/common.c:46
>  kasan_set_free_info+0x20/0x30 mm/kasan/tags.c:36
>  ____kasan_slab_free.constprop.0+0x178/0x1e0 mm/kasan/common.c:366
>  __kasan_slab_free+0x10/0x1c mm/kasan/common.c:374
>  kasan_slab_free include/linux/kasan.h:230 [inline]
>  slab_free_hook mm/slub.c:1628 [inline]
>  slab_free_freelist_hook+0xc4/0x20c mm/slub.c:1653
>  slab_free mm/slub.c:3213 [inline]
>  kmem_cache_free+0x9c/0x420 mm/slub.c:3229
>  putname.part.0+0x68/0x7c fs/namei.c:270
>  putname include/linux/err.h:41 [inline]
>  filename_parentat fs/namei.c:2547 [inline]
>  kern_path_locked+0x64/0x10c fs/namei.c:2558
>  handle_remove+0x38/0x284 drivers/base/devtmpfs.c:312
>  handle drivers/base/devtmpfs.c:382 [inline]
>  devtmpfs_work_loop drivers/base/devtmpfs.c:395 [inline]
>  devtmpfsd+0x8c/0xd0 drivers/base/devtmpfs.c:437
>  kthread+0x150/0x15c kernel/kthread.c:319
>  ret_from_fork+0x10/0x20 arch/arm64/kernel/entry.S:756
>
> The buggy address belongs to the object at ffff000002a36180
>  which belongs to the cache names_cache of size 4096
> The buggy address is located 32 bytes inside of
>  4096-byte region [ffff000002a36180, ffff000002a37180)
> The buggy address belongs to the page:
> page:00000000a105b3ae refcount:1 mapcount:0 mapping:0000000000000000 index:0xf3ff000002a34100 pfn:0x42a30
> head:00000000a105b3ae order:3 compound_mapcount:0 compound_pincount:0
> flags: 0x1ffc00000010200(slab|head|node=0|zone=0|lastcpupid=0x7ff|kasantag=0x0)
> raw: 01ffc00000010200 0000000000000000 dead000000000122 faff000002837700
> raw: f3ff000002a34100 0000000080070003 00000001ffffffff 0000000000000000
> page dumped because: kasan: bad access detected
>
> Memory state around the buggy address:
>  ffff000002a35f00: fe fe fe fe fe fe fe fe fe fe fe fe fe fe fe fe
>  ffff000002a36000: fe fe fe fe fe fe fe fe fe fe fe fe fe fe fe fe
> >ffff000002a36100: fe fe fe fe fe fe fe fe fe fe fe fe fe fe fe fe
>                                                  ^
>  ffff000002a36200: fe fe fe fe fe fe fe fe fe fe fe fe fe fe fe fe
>  ffff000002a36300: fe fe fe fe fe fe fe fe fe fe fe fe fe fe fe fe
> ==================================================================
>
>
> ---
> This report is generated by a bot. It may contain errors.
> See https://goo.gl/tpsmEJ for more information about syzbot.
> syzbot engineers can be reached at syzkaller@googlegroups.com.
>
> syzbot will keep track of this issue. See:
> https://goo.gl/tpsmEJ#status for how to communicate with syzbot.
>
> --
> You received this message because you are subscribed to the Google Groups "syzkaller-bugs" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to syzkaller-bugs+unsubscribe@googlegroups.com.
> To view this discussion on the web visit https://groups.google.com/d/msgid/syzkaller-bugs/000000000000a3cf8605cb2a1ec0%40google.com.

Re: [syzbot] upstream test error: KASAN: invalid-access Read in __entry_tramp_text_end

[Mark Rutland]

Hi Dmitry,

The good news is that the bad unwind is a known issue, the bad news is
that we don't currently have a way to fix it (and I'm planning to talk
about this at the LPC "objtool on arm64" talk this Friday).

More info below: the gist is we can produce spurious entries at an
exception boundary, but shouldn't miss a legitimate value, and there's a
plan to make it easier to spot when entries are not legitimate.

On Fri, Sep 17, 2021 at 05:03:48PM +0200, Dmitry Vyukov wrote:
> > Call trace:
> >  dump_backtrace+0x0/0x1ac arch/arm64/kernel/stacktrace.c:76
> >  show_stack+0x18/0x24 arch/arm64/kernel/stacktrace.c:215
> >  __dump_stack lib/dump_stack.c:88 [inline]
> >  dump_stack_lvl+0x68/0x84 lib/dump_stack.c:105
> >  print_address_description+0x7c/0x2b4 mm/kasan/report.c:256
> >  __kasan_report mm/kasan/report.c:442 [inline]
> >  kasan_report+0x134/0x380 mm/kasan/report.c:459
> >  __do_kernel_fault+0x128/0x1bc arch/arm64/mm/fault.c:317
> >  do_bad_area arch/arm64/mm/fault.c:466 [inline]
> >  do_tag_check_fault+0x74/0x90 arch/arm64/mm/fault.c:737
> >  do_mem_abort+0x44/0xb4 arch/arm64/mm/fault.c:813
> >  el1_abort+0x40/0x60 arch/arm64/kernel/entry-common.c:357
> >  el1h_64_sync_handler+0xb0/0xd0 arch/arm64/kernel/entry-common.c:408
> >  el1h_64_sync+0x78/0x7c arch/arm64/kernel/entry.S:567
> >  __entry_tramp_text_end+0xdfc/0x3000
> 
> /\/\/\/\/\/\/\
> 
> This is broken unwind on arm64. d_lookup statically calls __d_lookup,
> not __entry_tramp_text_end (which is not even a function).
> See the following thread for some debugging details:
> https://lore.kernel.org/lkml/CACT4Y+ZByJ71QfYHTByWaeCqZFxYfp8W8oyrK0baNaSJMDzoUw@mail.gmail.com/

The problem here is that our calling convention (AAPCS64) only allows us
to reliably unwind at function call boundaries, where the state of both
the Link Register (LR/x30) and Frame Pointer (FP/x29) are well-defined.
Within a function, we don't know whether to start unwinding from the LR
or FP, and we currently start from the LR, which can produce spurious
entries (but ensures we don't miss anything legitimte).

In the short term, I have a plan to make the unwinder indicate when an
entry might not be legitimate, with the usual stackdump code printing an
indicator like '?' on x86.

In the longer term, we might be doing things with objtool or asking for
some toolchain help such that we can do better in these cases.

Thanks,
Mark.

Re: [syzbot] upstream test error: KASAN: invalid-access Read in __entry_tramp_text_end

[Dmitry Vyukov]

On Tue, 21 Sept 2021 at 18:51, Mark Rutland <mark.rutland@arm.com> wrote:
>
> Hi Dmitry,
>
> The good news is that the bad unwind is a known issue, the bad news is
> that we don't currently have a way to fix it (and I'm planning to talk
> about this at the LPC "objtool on arm64" talk this Friday).
>
> More info below: the gist is we can produce spurious entries at an
> exception boundary, but shouldn't miss a legitimate value, and there's a
> plan to make it easier to spot when entries are not legitimate.
>
> On Fri, Sep 17, 2021 at 05:03:48PM +0200, Dmitry Vyukov wrote:
> > > Call trace:
> > >  dump_backtrace+0x0/0x1ac arch/arm64/kernel/stacktrace.c:76
> > >  show_stack+0x18/0x24 arch/arm64/kernel/stacktrace.c:215
> > >  __dump_stack lib/dump_stack.c:88 [inline]
> > >  dump_stack_lvl+0x68/0x84 lib/dump_stack.c:105
> > >  print_address_description+0x7c/0x2b4 mm/kasan/report.c:256
> > >  __kasan_report mm/kasan/report.c:442 [inline]
> > >  kasan_report+0x134/0x380 mm/kasan/report.c:459
> > >  __do_kernel_fault+0x128/0x1bc arch/arm64/mm/fault.c:317
> > >  do_bad_area arch/arm64/mm/fault.c:466 [inline]
> > >  do_tag_check_fault+0x74/0x90 arch/arm64/mm/fault.c:737
> > >  do_mem_abort+0x44/0xb4 arch/arm64/mm/fault.c:813
> > >  el1_abort+0x40/0x60 arch/arm64/kernel/entry-common.c:357
> > >  el1h_64_sync_handler+0xb0/0xd0 arch/arm64/kernel/entry-common.c:408
> > >  el1h_64_sync+0x78/0x7c arch/arm64/kernel/entry.S:567
> > >  __entry_tramp_text_end+0xdfc/0x3000
> >
> > /\/\/\/\/\/\/\
> >
> > This is broken unwind on arm64. d_lookup statically calls __d_lookup,
> > not __entry_tramp_text_end (which is not even a function).
> > See the following thread for some debugging details:
> > https://lore.kernel.org/lkml/CACT4Y+ZByJ71QfYHTByWaeCqZFxYfp8W8oyrK0baNaSJMDzoUw@mail.gmail.com/
>
> The problem here is that our calling convention (AAPCS64) only allows us
> to reliably unwind at function call boundaries, where the state of both
> the Link Register (LR/x30) and Frame Pointer (FP/x29) are well-defined.
> Within a function, we don't know whether to start unwinding from the LR
> or FP, and we currently start from the LR, which can produce spurious
> entries (but ensures we don't miss anything legitimte).
>
> In the short term, I have a plan to make the unwinder indicate when an
> entry might not be legitimate, with the usual stackdump code printing an
> indicator like '?' on x86.
>
> In the longer term, we might be doing things with objtool or asking for
> some toolchain help such that we can do better in these cases.

Hi Mark,

Any updates after the LPC session?

If the dumper adds " ? ", then syzkaller will strip these frames
(required for x86).
However, I am worried that we can remove the true top frame then and
attribute crashes to wrong frames again?

Some naive questions:
1. Shouldn't the top frame for synchronous faults be in the PC/IP
register (I would assume LR/FP contains the caller of the current
frame)?
2. How __entry_tramp_text_end, which is not a function, even ended up
in LR? shouldn't it always contain some code pointer (even if stale)?
3. Isn't there already something in the debug info to solve this
problem? Userspace programs don't use objtool, but I assume that can
print crash stacks somehow (?).

Re: [syzbot] upstream test error: KASAN: invalid-access Read in __entry_tramp_text_end

[Dmitry Vyukov]

On Mon, 27 Sept 2021 at 16:27, Dmitry Vyukov <dvyukov@google.com> wrote:
>
> On Tue, 21 Sept 2021 at 18:51, Mark Rutland <mark.rutland@arm.com> wrote:
> >
> > Hi Dmitry,
> >
> > The good news is that the bad unwind is a known issue, the bad news is
> > that we don't currently have a way to fix it (and I'm planning to talk
> > about this at the LPC "objtool on arm64" talk this Friday).
> >
> > More info below: the gist is we can produce spurious entries at an
> > exception boundary, but shouldn't miss a legitimate value, and there's a
> > plan to make it easier to spot when entries are not legitimate.
> >
> > On Fri, Sep 17, 2021 at 05:03:48PM +0200, Dmitry Vyukov wrote:
> > > > Call trace:
> > > >  dump_backtrace+0x0/0x1ac arch/arm64/kernel/stacktrace.c:76
> > > >  show_stack+0x18/0x24 arch/arm64/kernel/stacktrace.c:215
> > > >  __dump_stack lib/dump_stack.c:88 [inline]
> > > >  dump_stack_lvl+0x68/0x84 lib/dump_stack.c:105
> > > >  print_address_description+0x7c/0x2b4 mm/kasan/report.c:256
> > > >  __kasan_report mm/kasan/report.c:442 [inline]
> > > >  kasan_report+0x134/0x380 mm/kasan/report.c:459
> > > >  __do_kernel_fault+0x128/0x1bc arch/arm64/mm/fault.c:317
> > > >  do_bad_area arch/arm64/mm/fault.c:466 [inline]
> > > >  do_tag_check_fault+0x74/0x90 arch/arm64/mm/fault.c:737
> > > >  do_mem_abort+0x44/0xb4 arch/arm64/mm/fault.c:813
> > > >  el1_abort+0x40/0x60 arch/arm64/kernel/entry-common.c:357
> > > >  el1h_64_sync_handler+0xb0/0xd0 arch/arm64/kernel/entry-common.c:408
> > > >  el1h_64_sync+0x78/0x7c arch/arm64/kernel/entry.S:567
> > > >  __entry_tramp_text_end+0xdfc/0x3000
> > >
> > > /\/\/\/\/\/\/\
> > >
> > > This is broken unwind on arm64. d_lookup statically calls __d_lookup,
> > > not __entry_tramp_text_end (which is not even a function).
> > > See the following thread for some debugging details:
> > > https://lore.kernel.org/lkml/CACT4Y+ZByJ71QfYHTByWaeCqZFxYfp8W8oyrK0baNaSJMDzoUw@mail.gmail.com/
> >
> > The problem here is that our calling convention (AAPCS64) only allows us
> > to reliably unwind at function call boundaries, where the state of both
> > the Link Register (LR/x30) and Frame Pointer (FP/x29) are well-defined.
> > Within a function, we don't know whether to start unwinding from the LR
> > or FP, and we currently start from the LR, which can produce spurious
> > entries (but ensures we don't miss anything legitimte).
> >
> > In the short term, I have a plan to make the unwinder indicate when an
> > entry might not be legitimate, with the usual stackdump code printing an
> > indicator like '?' on x86.
> >
> > In the longer term, we might be doing things with objtool or asking for
> > some toolchain help such that we can do better in these cases.
>
> Hi Mark,
>
> Any updates after the LPC session?
>
> If the dumper adds " ? ", then syzkaller will strip these frames
> (required for x86).
> However, I am worried that we can remove the true top frame then and
> attribute crashes to wrong frames again?
>
> Some naive questions:
> 1. Shouldn't the top frame for synchronous faults be in the PC/IP
> register (I would assume LR/FP contains the caller of the current
> frame)?
> 2. How __entry_tramp_text_end, which is not a function, even ended up
> in LR? shouldn't it always contain some code pointer (even if stale)?
> 3. Isn't there already something in the debug info to solve this
> problem? Userspace programs don't use objtool, but I assume that can
> print crash stacks somehow (?).

+Will, Serban,

This ARM64 unwinder issue also means that all kernel MTE reports will
contain wrong top frame, right?

Re: [syzbot] upstream test error: KASAN: invalid-access Read in __entry_tramp_text_end

[Mark Rutland]

On Mon, Sep 27, 2021 at 04:27:30PM +0200, Dmitry Vyukov wrote:
> On Tue, 21 Sept 2021 at 18:51, Mark Rutland <mark.rutland@arm.com> wrote:
> >
> > Hi Dmitry,
> >
> > The good news is that the bad unwind is a known issue, the bad news is
> > that we don't currently have a way to fix it (and I'm planning to talk
> > about this at the LPC "objtool on arm64" talk this Friday).
> >
> > More info below: the gist is we can produce spurious entries at an
> > exception boundary, but shouldn't miss a legitimate value, and there's a
> > plan to make it easier to spot when entries are not legitimate.
> >
> > On Fri, Sep 17, 2021 at 05:03:48PM +0200, Dmitry Vyukov wrote:
> > > > Call trace:
> > > >  dump_backtrace+0x0/0x1ac arch/arm64/kernel/stacktrace.c:76
> > > >  show_stack+0x18/0x24 arch/arm64/kernel/stacktrace.c:215
> > > >  __dump_stack lib/dump_stack.c:88 [inline]
> > > >  dump_stack_lvl+0x68/0x84 lib/dump_stack.c:105
> > > >  print_address_description+0x7c/0x2b4 mm/kasan/report.c:256
> > > >  __kasan_report mm/kasan/report.c:442 [inline]
> > > >  kasan_report+0x134/0x380 mm/kasan/report.c:459
> > > >  __do_kernel_fault+0x128/0x1bc arch/arm64/mm/fault.c:317
> > > >  do_bad_area arch/arm64/mm/fault.c:466 [inline]
> > > >  do_tag_check_fault+0x74/0x90 arch/arm64/mm/fault.c:737
> > > >  do_mem_abort+0x44/0xb4 arch/arm64/mm/fault.c:813
> > > >  el1_abort+0x40/0x60 arch/arm64/kernel/entry-common.c:357
> > > >  el1h_64_sync_handler+0xb0/0xd0 arch/arm64/kernel/entry-common.c:408
> > > >  el1h_64_sync+0x78/0x7c arch/arm64/kernel/entry.S:567
> > > >  __entry_tramp_text_end+0xdfc/0x3000
> > >
> > > /\/\/\/\/\/\/\
> > >
> > > This is broken unwind on arm64. d_lookup statically calls __d_lookup,
> > > not __entry_tramp_text_end (which is not even a function).
> > > See the following thread for some debugging details:
> > > https://lore.kernel.org/lkml/CACT4Y+ZByJ71QfYHTByWaeCqZFxYfp8W8oyrK0baNaSJMDzoUw@mail.gmail.com/

Looking at this again (and as you point out below), my initial analysis
was wrong, and this isn't to do with the LR -- this value should be the
PC at the time the exception boundary.

Per `aarch64-linux-objdump -t vmlinux | sort -n`, in my kernel
Image I have:

ffff800011799000 g       .text  0000000000000000 __entry_tramp_text_end
ffff8000117a0000 g     O .rodata        0000000000000008 kimage_vaddr
ffff8000117a0000 g       .rodata        0000000000000000 _etext
ffff8000117a0000 g       .rodata        0000000000000000 __kvm_nvhe___start_rodata
ffff8000117a0000 g       .rodata        0000000000000000 __start_rodata
ffff8000117a0000 l    d  .rodata        0000000000000000 .rodata

... and per vmlinux.lds.S, the space between __entry_tramp_text_end and _etext
(all 0x7000 of it in my case) is formed of:

	*(.fixup)
	*(.gnu.warning)
	*(.got)
	.got.plt : { *(.got.plt) }

... so whatever is faulting in your configuration is likely in one of
those, and as it's anonymous, the kernel ends up symbolizing it as
__entry_tramp_text_end, as that's the immediately prior symbol.

In my kernel, the +0xdfc offset would be a fixup, and very few of those
perform faulting instructions. THe ones which do are 

... where I'd expext the 


> >
> > The problem here is that our calling convention (AAPCS64) only allows us
> > to reliably unwind at function call boundaries, where the state of both
> > the Link Register (LR/x30) and Frame Pointer (FP/x29) are well-defined.
> > Within a function, we don't know whether to start unwinding from the LR
> > or FP, and we currently start from the LR, which can produce spurious
> > entries (but ensures we don't miss anything legitimte).
> >
> > In the short term, I have a plan to make the unwinder indicate when an
> > entry might not be legitimate, with the usual stackdump code printing an
> > indicator like '?' on x86.
> >
> > In the longer term, we might be doing things with objtool or asking for
> > some toolchain help such that we can do better in these cases.
> 
> Hi Mark,
> 
> Any updates after the LPC session?

Nothing concrete. We have a few things to go looking at, e.g. processing
DWARF into something usable, or using CTF unwind info (which is
currently being spec'd).

> If the dumper adds " ? ", then syzkaller will strip these frames
> (required for x86).
> However, I am worried that we can remove the true top frame then and
> attribute crashes to wrong frames again?

Yes -- since we don't know either way whether the frame is legitimate or
bogus, always removing the frame would also be misleading.

So it probably wouldn't make sense for Syzkaller to strip such frames.

> Some naive questions:
> 1. Shouldn't the top frame for synchronous faults be in the PC/IP
> register (I would assume LR/FP contains the caller of the current
> frame)?

Yes; you're right, and this appears to be a distinct issue. Looking at
vmlinux.lds.S, it seems likely this is a fixup.

it seems like we must be taking a fault somewhere unexpected.
trampoline itself...

> 2. How __entry_tramp_text_end, which is not a function, even ended up
> in LR? shouldn't it always contain some code pointer (even if stale)?

IIUC the LR is only required to be a code pointer at function call
boundaries, and at other times can be used as a scratch register.

That said, I'll go digging to find a concrete explanation.

> 3. Isn't there already something in the debug info to solve this
> problem? Userspace programs don't use objtool, but I assume that can
> print crash stacks somehow (?).

IIUC this can be solved with DWARF, but in-kernel DWARF support has been
NAK'd due ot the complexity and performance concerns, so we don't have
an in-kernel solution at present.

Re: [syzbot] upstream test error: KASAN: invalid-access Read in __entry_tramp_text_end

[Mark Rutland]

On Mon, Sep 27, 2021 at 06:01:22PM +0100, Mark Rutland wrote:
> On Mon, Sep 27, 2021 at 04:27:30PM +0200, Dmitry Vyukov wrote:
> > On Tue, 21 Sept 2021 at 18:51, Mark Rutland <mark.rutland@arm.com> wrote:
> > >
> > > Hi Dmitry,
> > >
> > > The good news is that the bad unwind is a known issue, the bad news is
> > > that we don't currently have a way to fix it (and I'm planning to talk
> > > about this at the LPC "objtool on arm64" talk this Friday).
> > >
> > > More info below: the gist is we can produce spurious entries at an
> > > exception boundary, but shouldn't miss a legitimate value, and there's a
> > > plan to make it easier to spot when entries are not legitimate.
> > >
> > > On Fri, Sep 17, 2021 at 05:03:48PM +0200, Dmitry Vyukov wrote:
> > > > > Call trace:
> > > > >  dump_backtrace+0x0/0x1ac arch/arm64/kernel/stacktrace.c:76
> > > > >  show_stack+0x18/0x24 arch/arm64/kernel/stacktrace.c:215
> > > > >  __dump_stack lib/dump_stack.c:88 [inline]
> > > > >  dump_stack_lvl+0x68/0x84 lib/dump_stack.c:105
> > > > >  print_address_description+0x7c/0x2b4 mm/kasan/report.c:256
> > > > >  __kasan_report mm/kasan/report.c:442 [inline]
> > > > >  kasan_report+0x134/0x380 mm/kasan/report.c:459
> > > > >  __do_kernel_fault+0x128/0x1bc arch/arm64/mm/fault.c:317
> > > > >  do_bad_area arch/arm64/mm/fault.c:466 [inline]
> > > > >  do_tag_check_fault+0x74/0x90 arch/arm64/mm/fault.c:737
> > > > >  do_mem_abort+0x44/0xb4 arch/arm64/mm/fault.c:813
> > > > >  el1_abort+0x40/0x60 arch/arm64/kernel/entry-common.c:357
> > > > >  el1h_64_sync_handler+0xb0/0xd0 arch/arm64/kernel/entry-common.c:408
> > > > >  el1h_64_sync+0x78/0x7c arch/arm64/kernel/entry.S:567
> > > > >  __entry_tramp_text_end+0xdfc/0x3000
> > > >
> > > > /\/\/\/\/\/\/\
> > > >
> > > > This is broken unwind on arm64. d_lookup statically calls __d_lookup,
> > > > not __entry_tramp_text_end (which is not even a function).
> > > > See the following thread for some debugging details:
> > > > https://lore.kernel.org/lkml/CACT4Y+ZByJ71QfYHTByWaeCqZFxYfp8W8oyrK0baNaSJMDzoUw@mail.gmail.com/
> 
> Looking at this again (and as you point out below), my initial analysis
> was wrong, and this isn't to do with the LR -- this value should be the
> PC at the time the exception boundary.

Whoops, I accidentally nuked the more complete/accurate analysis I just
wrote and sent the earlier version. Today is not a good day for me and
computers. :(

What's happened here is that __d_lookup() (via a few layers of inlining) called
load_unaligned_zeropad(). The `LDR` at the start of the asm faulted (I suspect
due to a tag check fault), and so the exception handler replaced the PC with
the (anonymous) fixup function. This is akin to a tail or sibling call, and so
the fixup function entirely replaces __d_lookup() in the trace.

The fixup function itself has an `LDR` which faulted (because it's
designed to fixup page alignment problems, not tag check faults), and
that is what's reported here.

As the fixup function is anonymous, and the nearest prior symbol in .text is
__entry_tramp_text_end, it gets symbolized as an offset from that.

We can make the unwinds a bit nicer by adding some markers (e.g. patch
below), but actually fixing this case will require some more thought.

Thanks,
Mark.

---->8----
diff --git a/arch/arm64/kernel/vmlinux.lds.S b/arch/arm64/kernel/vmlinux.lds.S
index 709d2c433c5e..127096a0faea 100644
--- a/arch/arm64/kernel/vmlinux.lds.S
+++ b/arch/arm64/kernel/vmlinux.lds.S
@@ -111,6 +111,11 @@ jiffies = jiffies_64;
 #define TRAMP_TEXT
 #endif
 
+#define FIXUP_TEXT                                     \
+       __fixup_text_start = .;                         \
+       *(.fixup);                                      \
+       __fixup_text_end = .;
+
 /*
  * The size of the PE/COFF section that covers the kernel image, which
  * runs from _stext to _edata, must be a round multiple of the PE/COFF
@@ -161,7 +166,7 @@ SECTIONS
                        IDMAP_TEXT
                        HIBERNATE_TEXT
                        TRAMP_TEXT
-                       *(.fixup)
+                       FIXUP_TEXT
                        *(.gnu.warning)
                . = ALIGN(16);
                *(.got)                 /* Global offset table          */

Re: [syzbot] upstream test error: KASAN: invalid-access Read in __entry_tramp_text_end

[Dmitry Vyukov]

 On Mon, 27 Sept 2021 at 19:18, Mark Rutland <mark.rutland@arm.com> wrote:
>
> On Mon, Sep 27, 2021 at 06:01:22PM +0100, Mark Rutland wrote:
> > On Mon, Sep 27, 2021 at 04:27:30PM +0200, Dmitry Vyukov wrote:
> > > On Tue, 21 Sept 2021 at 18:51, Mark Rutland <mark.rutland@arm.com> wrote:
> > > >
> > > > Hi Dmitry,
> > > >
> > > > The good news is that the bad unwind is a known issue, the bad news is
> > > > that we don't currently have a way to fix it (and I'm planning to talk
> > > > about this at the LPC "objtool on arm64" talk this Friday).
> > > >
> > > > More info below: the gist is we can produce spurious entries at an
> > > > exception boundary, but shouldn't miss a legitimate value, and there's a
> > > > plan to make it easier to spot when entries are not legitimate.
> > > >
> > > > On Fri, Sep 17, 2021 at 05:03:48PM +0200, Dmitry Vyukov wrote:
> > > > > > Call trace:
> > > > > >  dump_backtrace+0x0/0x1ac arch/arm64/kernel/stacktrace.c:76
> > > > > >  show_stack+0x18/0x24 arch/arm64/kernel/stacktrace.c:215
> > > > > >  __dump_stack lib/dump_stack.c:88 [inline]
> > > > > >  dump_stack_lvl+0x68/0x84 lib/dump_stack.c:105
> > > > > >  print_address_description+0x7c/0x2b4 mm/kasan/report.c:256
> > > > > >  __kasan_report mm/kasan/report.c:442 [inline]
> > > > > >  kasan_report+0x134/0x380 mm/kasan/report.c:459
> > > > > >  __do_kernel_fault+0x128/0x1bc arch/arm64/mm/fault.c:317
> > > > > >  do_bad_area arch/arm64/mm/fault.c:466 [inline]
> > > > > >  do_tag_check_fault+0x74/0x90 arch/arm64/mm/fault.c:737
> > > > > >  do_mem_abort+0x44/0xb4 arch/arm64/mm/fault.c:813
> > > > > >  el1_abort+0x40/0x60 arch/arm64/kernel/entry-common.c:357
> > > > > >  el1h_64_sync_handler+0xb0/0xd0 arch/arm64/kernel/entry-common.c:408
> > > > > >  el1h_64_sync+0x78/0x7c arch/arm64/kernel/entry.S:567
> > > > > >  __entry_tramp_text_end+0xdfc/0x3000
> > > > >
> > > > > /\/\/\/\/\/\/\
> > > > >
> > > > > This is broken unwind on arm64. d_lookup statically calls __d_lookup,
> > > > > not __entry_tramp_text_end (which is not even a function).
> > > > > See the following thread for some debugging details:
> > > > > https://lore.kernel.org/lkml/CACT4Y+ZByJ71QfYHTByWaeCqZFxYfp8W8oyrK0baNaSJMDzoUw@mail.gmail.com/
> >
> > Looking at this again (and as you point out below), my initial analysis
> > was wrong, and this isn't to do with the LR -- this value should be the
> > PC at the time the exception boundary.
>
> Whoops, I accidentally nuked the more complete/accurate analysis I just
> wrote and sent the earlier version. Today is not a good day for me and
> computers. :(
>
> What's happened here is that __d_lookup() (via a few layers of inlining) called
> load_unaligned_zeropad(). The `LDR` at the start of the asm faulted (I suspect
> due to a tag check fault), and so the exception handler replaced the PC with
> the (anonymous) fixup function. This is akin to a tail or sibling call, and so
> the fixup function entirely replaces __d_lookup() in the trace.
>
> The fixup function itself has an `LDR` which faulted (because it's
> designed to fixup page alignment problems, not tag check faults), and
> that is what's reported here.
>
> As the fixup function is anonymous, and the nearest prior symbol in .text is
> __entry_tramp_text_end, it gets symbolized as an offset from that.
>
> We can make the unwinds a bit nicer by adding some markers (e.g. patch
> below), but actually fixing this case will require some more thought.
>
> Thanks,
> Mark.
>
> ---->8----
> diff --git a/arch/arm64/kernel/vmlinux.lds.S b/arch/arm64/kernel/vmlinux.lds.S
> index 709d2c433c5e..127096a0faea 100644
> --- a/arch/arm64/kernel/vmlinux.lds.S
> +++ b/arch/arm64/kernel/vmlinux.lds.S
> @@ -111,6 +111,11 @@ jiffies = jiffies_64;
>  #define TRAMP_TEXT
>  #endif
>
> +#define FIXUP_TEXT                                     \
> +       __fixup_text_start = .;                         \
> +       *(.fixup);                                      \
> +       __fixup_text_end = .;
> +
>  /*
>   * The size of the PE/COFF section that covers the kernel image, which
>   * runs from _stext to _edata, must be a round multiple of the PE/COFF
> @@ -161,7 +166,7 @@ SECTIONS
>                         IDMAP_TEXT
>                         HIBERNATE_TEXT
>                         TRAMP_TEXT
> -                       *(.fixup)
> +                       FIXUP_TEXT
>                         *(.gnu.warning)
>                 . = ALIGN(16);
>                 *(.got)                 /* Global offset table          */


Oh, good it's very local to the .fixup thing rather than a common
issue that affects all unwinds.
In the other x86 thread Josh Poimboeuf suggested to use asm goto to a
cold part of the function instead of .fixup:
https://lore.kernel.org/lkml/20210927234543.6waods7rraxseind@treble/
This sounds like a more reliable solution that will cause less
maintenance burden. Would it work for arm64 as well?

Re: [syzbot] upstream test error: KASAN: invalid-access Read in __entry_tramp_text_end

[Mark Rutland]

On Tue, Sep 28, 2021 at 12:19:23PM +0200, Dmitry Vyukov wrote:
>  On Mon, 27 Sept 2021 at 19:18, Mark Rutland <mark.rutland@arm.com> wrote:
> > What's happened here is that __d_lookup() (via a few layers of inlining) called
> > load_unaligned_zeropad(). The `LDR` at the start of the asm faulted (I suspect
> > due to a tag check fault), and so the exception handler replaced the PC with
> > the (anonymous) fixup function. This is akin to a tail or sibling call, and so
> > the fixup function entirely replaces __d_lookup() in the trace.
> >
> > The fixup function itself has an `LDR` which faulted (because it's
> > designed to fixup page alignment problems, not tag check faults), and
> > that is what's reported here.
> >
> > As the fixup function is anonymous, and the nearest prior symbol in .text is
> > __entry_tramp_text_end, it gets symbolized as an offset from that.
> >
> > We can make the unwinds a bit nicer by adding some markers (e.g. patch
> > below), but actually fixing this case will require some more thought.
> >
> > Thanks,
> > Mark.
> >
> > ---->8----
> > diff --git a/arch/arm64/kernel/vmlinux.lds.S b/arch/arm64/kernel/vmlinux.lds.S
> > index 709d2c433c5e..127096a0faea 100644
> > --- a/arch/arm64/kernel/vmlinux.lds.S
> > +++ b/arch/arm64/kernel/vmlinux.lds.S
> > @@ -111,6 +111,11 @@ jiffies = jiffies_64;
> >  #define TRAMP_TEXT
> >  #endif
> >
> > +#define FIXUP_TEXT                                     \
> > +       __fixup_text_start = .;                         \
> > +       *(.fixup);                                      \
> > +       __fixup_text_end = .;
> > +
> >  /*
> >   * The size of the PE/COFF section that covers the kernel image, which
> >   * runs from _stext to _edata, must be a round multiple of the PE/COFF
> > @@ -161,7 +166,7 @@ SECTIONS
> >                         IDMAP_TEXT
> >                         HIBERNATE_TEXT
> >                         TRAMP_TEXT
> > -                       *(.fixup)
> > +                       FIXUP_TEXT
> >                         *(.gnu.warning)
> >                 . = ALIGN(16);
> >                 *(.got)                 /* Global offset table          */
> 
> 
> Oh, good it's very local to the .fixup thing rather than a common
> issue that affects all unwinds.

Yes, though the other issue I mentioned *does* exist, and can occur
separately, even if we're getting lucky and not hitting it often enough
to notice.

> In the other x86 thread Josh Poimboeuf suggested to use asm goto to a
> cold part of the function instead of .fixup:
> https://lore.kernel.org/lkml/20210927234543.6waods7rraxseind@treble/
> This sounds like a more reliable solution that will cause less
> maintenance burden. Would it work for arm64 as well?

Maybe we can use that when CC_HAS_ASM_GOTO_OUTPUT is avaiable, but in
general we can't rely on asm goto supporting output arguments (and IIRC
GCC doesn't support that at all), so we'd still have to support the
current fixup scheme.

Thanks,
Mark.

Re: [syzbot] upstream test error: KASAN: invalid-access Read in __entry_tramp_text_end

[Josh Poimboeuf]

On Tue, Sep 28, 2021 at 11:35:43AM +0100, Mark Rutland wrote:
> > In the other x86 thread Josh Poimboeuf suggested to use asm goto to a
> > cold part of the function instead of .fixup:
> > https://lore.kernel.org/lkml/20210927234543.6waods7rraxseind@treble/
> > This sounds like a more reliable solution that will cause less
> > maintenance burden. Would it work for arm64 as well?
> 
> Maybe we can use that when CC_HAS_ASM_GOTO_OUTPUT is avaiable, but in
> general we can't rely on asm goto supporting output arguments (and IIRC
> GCC doesn't support that at all), so we'd still have to support the
> current fixup scheme.

Even without CC_HAS_ASM_GOTO_OUTPUT it should still be possible to hack
something together if you split the original insn asm and the extable
asm into separate statements, like:

diff --git a/arch/x86/include/asm/msr.h b/arch/x86/include/asm/msr.h
index 6b52182e178a..8f62469f2027 100644
--- a/arch/x86/include/asm/msr.h
+++ b/arch/x86/include/asm/msr.h
@@ -137,20 +139,21 @@ static inline unsigned long long native_read_msr_safe(unsigned int msr,
 {
 	DECLARE_ARGS(val, low, high);
 
-	asm volatile("2: rdmsr ; xor %[err],%[err]\n"
-		     "1:\n\t"
-		     ".section .fixup,\"ax\"\n\t"
-		     "3: mov %[fault],%[err]\n\t"
-		     "xorl %%eax, %%eax\n\t"
-		     "xorl %%edx, %%edx\n\t"
-		     "jmp 1b\n\t"
-		     ".previous\n\t"
-		     _ASM_EXTABLE(2b, 3b)
-		     : [err] "=r" (*err), EAX_EDX_RET(val, low, high)
-		     : "c" (msr), [fault] "i" (-EIO));
+	*err = 0;
+	asm volatile("417: rdmsr\n"
+		     : EAX_EDX_RET(val, low, high)
+		     : "c" (msr));
+	asm_volatile_goto(_ASM_EXTABLE(417b, %l[Efault]) :::: Efault);
+
+done:
 	if (tracepoint_enabled(read_msr))
 		do_trace_read_msr(msr, EAX_EDX_VAL(val, low, high), *err);
 	return EAX_EDX_VAL(val, low, high);
+
+Efault:
+	*err = -EIO;
+	ZERO_ARGS(val, low, high);
+	goto done;
 }
 
 /* Can be uninlined because referenced by paravirt */

Re: [syzbot] upstream test error: KASAN: invalid-access Read in __entry_tramp_text_end

[Peter Zijlstra]

On Tue, Sep 28, 2021 at 06:36:37PM -0700, Josh Poimboeuf wrote:
> On Tue, Sep 28, 2021 at 11:35:43AM +0100, Mark Rutland wrote:
> > > In the other x86 thread Josh Poimboeuf suggested to use asm goto to a
> > > cold part of the function instead of .fixup:
> > > https://lore.kernel.org/lkml/20210927234543.6waods7rraxseind@treble/
> > > This sounds like a more reliable solution that will cause less
> > > maintenance burden. Would it work for arm64 as well?
> > 
> > Maybe we can use that when CC_HAS_ASM_GOTO_OUTPUT is avaiable, but in
> > general we can't rely on asm goto supporting output arguments (and IIRC
> > GCC doesn't support that at all), so we'd still have to support the
> > current fixup scheme.

gcc-11 has it

> Even without CC_HAS_ASM_GOTO_OUTPUT it should still be possible to hack
> something together if you split the original insn asm and the extable
> asm into separate statements, like:
> 
> diff --git a/arch/x86/include/asm/msr.h b/arch/x86/include/asm/msr.h
> index 6b52182e178a..8f62469f2027 100644
> --- a/arch/x86/include/asm/msr.h
> +++ b/arch/x86/include/asm/msr.h
> @@ -137,20 +139,21 @@ static inline unsigned long long native_read_msr_safe(unsigned int msr,
>  {
>  	DECLARE_ARGS(val, low, high);
>  
> +	*err = 0;
> +	asm volatile("417: rdmsr\n"
> +		     : EAX_EDX_RET(val, low, high)
> +		     : "c" (msr));
> +	asm_volatile_goto(_ASM_EXTABLE(417b, %l[Efault]) :::: Efault);

That's terrible :-) Could probably do with a comment, but might just
work..

> +
> +done:
>  	if (tracepoint_enabled(read_msr))
>  		do_trace_read_msr(msr, EAX_EDX_VAL(val, low, high), *err);
>  	return EAX_EDX_VAL(val, low, high);
> +
> +Efault:
> +	*err = -EIO;
> +	ZERO_ARGS(val, low, high);
> +	goto done;
>  }
>  
>  /* Can be uninlined because referenced by paravirt */
> 

Re: [syzbot] upstream test error: KASAN: invalid-access Read in __entry_tramp_text_end

[Mark Rutland]

On Wed, Sep 29, 2021 at 09:39:47AM +0200, Peter Zijlstra wrote:
> On Tue, Sep 28, 2021 at 06:36:37PM -0700, Josh Poimboeuf wrote:
> > On Tue, Sep 28, 2021 at 11:35:43AM +0100, Mark Rutland wrote:
> > > > In the other x86 thread Josh Poimboeuf suggested to use asm goto to a
> > > > cold part of the function instead of .fixup:
> > > > https://lore.kernel.org/lkml/20210927234543.6waods7rraxseind@treble/
> > > > This sounds like a more reliable solution that will cause less
> > > > maintenance burden. Would it work for arm64 as well?
> > > 
> > > Maybe we can use that when CC_HAS_ASM_GOTO_OUTPUT is avaiable, but in
> > > general we can't rely on asm goto supporting output arguments (and IIRC
> > > GCC doesn't support that at all), so we'd still have to support the
> > > current fixup scheme.
> 
> gcc-11 has it

Neat. Worth looking at for future, then.

> > Even without CC_HAS_ASM_GOTO_OUTPUT it should still be possible to hack
> > something together if you split the original insn asm and the extable
> > asm into separate statements, like:
> > 
> > diff --git a/arch/x86/include/asm/msr.h b/arch/x86/include/asm/msr.h
> > index 6b52182e178a..8f62469f2027 100644
> > --- a/arch/x86/include/asm/msr.h
> > +++ b/arch/x86/include/asm/msr.h
> > @@ -137,20 +139,21 @@ static inline unsigned long long native_read_msr_safe(unsigned int msr,
> >  {
> >  	DECLARE_ARGS(val, low, high);
> >  
> > +	*err = 0;
> > +	asm volatile("417: rdmsr\n"
> > +		     : EAX_EDX_RET(val, low, high)
> > +		     : "c" (msr));
> > +	asm_volatile_goto(_ASM_EXTABLE(417b, %l[Efault]) :::: Efault);
> 
> That's terrible :-) Could probably do with a comment, but might just
> work..

The compiler is well within its rights to spill/restore/copy/shuffle
registers or modify memory between the two asm blocks (which it's liable
to do that when optimizing this after a few layers of inlining), and
skipping that would cause all sorts of undefined behaviour.

It's akin to trying to do an asm goto without the compiler supporting
asm goto.

This would probably happen to work in the common case, but it'd cause
nightmarish bugs in others...

Thanks,
Mark.


> > +
> > +done:
> >  	if (tracepoint_enabled(read_msr))
> >  		do_trace_read_msr(msr, EAX_EDX_VAL(val, low, high), *err);
> >  	return EAX_EDX_VAL(val, low, high);
> > +
> > +Efault:
> > +	*err = -EIO;
> > +	ZERO_ARGS(val, low, high);
> > +	goto done;
> >  }
> >  
> >  /* Can be uninlined because referenced by paravirt */
> > 

Re: [syzbot] upstream test error: KASAN: invalid-access Read in __entry_tramp_text_end

[Peter Zijlstra]

On Wed, Sep 29, 2021 at 09:50:45AM +0100, Mark Rutland wrote:
> On Wed, Sep 29, 2021 at 09:39:47AM +0200, Peter Zijlstra wrote:
> > On Tue, Sep 28, 2021 at 06:36:37PM -0700, Josh Poimboeuf wrote:

> > > +	asm volatile("417: rdmsr\n"
> > > +		     : EAX_EDX_RET(val, low, high)
> > > +		     : "c" (msr));
> > > +	asm_volatile_goto(_ASM_EXTABLE(417b, %l[Efault]) :::: Efault);
> > 
> > That's terrible :-) Could probably do with a comment, but might just
> > work..
> 
> The compiler is well within its rights to spill/restore/copy/shuffle
> registers or modify memory between the two asm blocks (which it's liable
> to do that when optimizing this after a few layers of inlining), and
> skipping that would cause all sorts of undefined behaviour.

Ah, but in this case it'll work irrespective of that (which is why we
needs a comment!).

This is because _ASM_EXTABLE only generates data for another section.
There doesn't need to be code continuity between these two asm
statements.

As I said, this is terrible :-)

Re: [syzbot] upstream test error: KASAN: invalid-access Read in __entry_tramp_text_end

[Mark Rutland]

On Wed, Sep 29, 2021 at 11:59:51AM +0200, Peter Zijlstra wrote:
> On Wed, Sep 29, 2021 at 09:50:45AM +0100, Mark Rutland wrote:
> > On Wed, Sep 29, 2021 at 09:39:47AM +0200, Peter Zijlstra wrote:
> > > On Tue, Sep 28, 2021 at 06:36:37PM -0700, Josh Poimboeuf wrote:
> 
> > > > +	asm volatile("417: rdmsr\n"
> > > > +		     : EAX_EDX_RET(val, low, high)
> > > > +		     : "c" (msr));
> > > > +	asm_volatile_goto(_ASM_EXTABLE(417b, %l[Efault]) :::: Efault);
> > > 
> > > That's terrible :-) Could probably do with a comment, but might just
> > > work..
> > 
> > The compiler is well within its rights to spill/restore/copy/shuffle
> > registers or modify memory between the two asm blocks (which it's liable
> > to do that when optimizing this after a few layers of inlining), and
> > skipping that would cause all sorts of undefined behaviour.
> 
> Ah, but in this case it'll work irrespective of that (which is why we
> needs a comment!).
> 
> This is because _ASM_EXTABLE only generates data for another section.
> There doesn't need to be code continuity between these two asm
> statements.

I think you've missed my point. It doesn't matter that the
asm_volatile_goto() doesn't contain code, and this is solely about the
*state* expected at entry/exit from each asm block being different.

The problem is that when the compiler encounters the
asm_volatile_goto(), it will generate a target for `Efault` expecting
the state of registers/stack/etc to be consistent with the state at
entry to the asm_volatile_goto() block. So if the compiler places any
register/memory manipulation between the asm volatile and the
asm_volatile_goto block, that expectation will be violated, since we
effectively branch from the first asm volatile block directly to the
label handed to the asm_volatile_goto block.

Consider the following pseudo asm example:

inline unsigned long read_magic_asm_thing(void)
{
	// asm constraints allocates this into x3 for now
	unsigned long ret = 3;

	asm volatile(
	"magic_insn_that_can_only_read_into x3\n"
	"fault_insn: some_faulting_insn x3\n"
	: [x3] "x3" (ret)
	);

	// compiler moves x3 into x0 because that's simpler for later
	// code (in both the fall-through and branch case of the
	// asm_volatile_goto()).
	// Maybe it shuffles other things too, e.g. moving another
	// variable into x3.

	// This is generated expecting the register allocation at this
	// instant in the code
	asm_volatile_goto(extable_from_to(fault_isn, Efault));

	// When not faulting, x0 is used here; this works correctly.
	return ret;

Efault:
	// When we take a fault from the first asm, the `ret` value is
	// in x3, and we skipped the moves between the two asm blocks.
	// This code was generated assuming those had happened (since
	// that was the case at the start of the asm_volatile_goto(),
	// and consumes x0 here, which contains garbage.
	do_something_with(ret);

	// Maybe this uses something that was moved into x3, but we have
	// `ret` there instead.
	something_else();
	
	// Who knows if we even got here safely.
	return whatever;
}

Thanks,
Mark.

Re: [syzbot] upstream test error: KASAN: invalid-access Read in __entry_tramp_text_end

[Peter Zijlstra]

On Wed, Sep 29, 2021 at 11:37:30AM +0100, Mark Rutland wrote:

> > This is because _ASM_EXTABLE only generates data for another section.
> > There doesn't need to be code continuity between these two asm
> > statements.
> 
> I think you've missed my point. It doesn't matter that the
> asm_volatile_goto() doesn't contain code, and this is solely about the
> *state* expected at entry/exit from each asm block being different.

Urgh.. indeed :/

Re: [syzbot] upstream test error: KASAN: invalid-access Read in __entry_tramp_text_end

[Josh Poimboeuf]

On Wed, Sep 29, 2021 at 01:43:23PM +0200, Peter Zijlstra wrote:
> On Wed, Sep 29, 2021 at 11:37:30AM +0100, Mark Rutland wrote:
> 
> > > This is because _ASM_EXTABLE only generates data for another section.
> > > There doesn't need to be code continuity between these two asm
> > > statements.
> > 
> > I think you've missed my point. It doesn't matter that the
> > asm_volatile_goto() doesn't contain code, and this is solely about the
> > *state* expected at entry/exit from each asm block being different.
> 
> Urgh.. indeed :/

So much for that idea :-/

To fix the issue of the wrong .fixup code symbol names getting printed,
we could (as Mark suggested) add a '__fixup_text_start' symbol at the
start of the .fixup section.  And then remove all other symbols in the
.fixup section.

For x86, that means removing the kvm_fastop_exception symbol and a few
others.  That way it's all anonymous code, displayed by the kernel as
"__fixup_text_start+0x1234".  Which isn't all that useful, but still
better than printing the wrong symbol.

But there's still a bigger problem: the function with the faulting
instruction doesn't get reported in the stack trace.

For example, in the up-thread bug report, __d_lookup() bug report
doesn't get printed, even though its anonymous .fixup code is running in
the context of the function and will be branching back to it shortly.

Even worse, this means livepatch is broken, because if for example
__d_lookup()'s .fixup code gets preempted, __d_lookup() can get skipped
by a reliable stack trace.

So we may need to get rid of .fixup altogether.  Especially for arches
which support livepatch.

We can replace some of the custom .fixup handlers with generic handlers
like x86 does, which do the fixup work in exception context.  This
generally works better for more generic work like putting an error code
in a certain register and resuming execution at the subsequent
instruction.

However a lot of the .fixup code is rather custom and doesn't
necessarily work well with that model.

In such cases we could just move the .fixup code into the function
(inline for older compilers; out-of-line for compilers that support
CC_HAS_ASM_GOTO_OUTPUT).

Alternatively we could convert each .fixup code fragment into a proper
function which returns to a specified resume point in the function, and
then have the exception handler emulate a call to it like we do with
int3_emulate_call().

-- 
Josh

Re: [syzbot] upstream test error: KASAN: invalid-access Read in __entry_tramp_text_end

[Mark Rutland]

On Thu, Sep 30, 2021 at 12:26:38PM -0700, Josh Poimboeuf wrote:
> On Wed, Sep 29, 2021 at 01:43:23PM +0200, Peter Zijlstra wrote:
> > On Wed, Sep 29, 2021 at 11:37:30AM +0100, Mark Rutland wrote:
> > 
> > > > This is because _ASM_EXTABLE only generates data for another section.
> > > > There doesn't need to be code continuity between these two asm
> > > > statements.
> > > 
> > > I think you've missed my point. It doesn't matter that the
> > > asm_volatile_goto() doesn't contain code, and this is solely about the
> > > *state* expected at entry/exit from each asm block being different.
> > 
> > Urgh.. indeed :/
> 
> So much for that idea :-/
> 
> To fix the issue of the wrong .fixup code symbol names getting printed,
> we could (as Mark suggested) add a '__fixup_text_start' symbol at the
> start of the .fixup section.  And then remove all other symbols in the
> .fixup section.

Just to be clear, that was just as a "make debugging slightly less
painful" aid, not as a fix for reliable stackrtrace and all that.

> For x86, that means removing the kvm_fastop_exception symbol and a few
> others.  That way it's all anonymous code, displayed by the kernel as
> "__fixup_text_start+0x1234".  Which isn't all that useful, but still
> better than printing the wrong symbol.
> 
> But there's still a bigger problem: the function with the faulting
> instruction doesn't get reported in the stack trace.
> 
> For example, in the up-thread bug report, __d_lookup() bug report
> doesn't get printed, even though its anonymous .fixup code is running in
> the context of the function and will be branching back to it shortly.
> 
> Even worse, this means livepatch is broken, because if for example
> __d_lookup()'s .fixup code gets preempted, __d_lookup() can get skipped
> by a reliable stack trace.
> 
> So we may need to get rid of .fixup altogether.  Especially for arches
> which support livepatch.
> 
> We can replace some of the custom .fixup handlers with generic handlers
> like x86 does, which do the fixup work in exception context.  This
> generally works better for more generic work like putting an error code
> in a certain register and resuming execution at the subsequent
> instruction.

I reckon even ignoring the unwind problems this'd be a good thing since
it'd save on redundant copies of the fixup logic that happen to be
identical, and the common cases like uaccess all fall into this shape.

As for how to do that, in the past Peter and I had come up with some
assembler trickery to get the name of the error code register encoded
into the extable info:

  https://lore.kernel.org/lkml/20170207111011.GB28790@leverpostej/
  https://lore.kernel.org/lkml/20170207160300.GB26173@leverpostej/
  https://lore.kernel.org/lkml/20170208091250.GT6515@twins.programming.kicks-ass.net/

... but maybe that's already solved on x86 in a different way?

> However a lot of the .fixup code is rather custom and doesn't
> necessarily work well with that model.

Looking at arm64, even where we'd need custom handlers it does appear we
could mostly do that out-of-line in the exception handler. The more
exotic cases are largely in out-of-line asm functions, where we can move
the fixups within the function, after the usual return.

I reckon we can handle the fixups for load_unaligned_zeropad() in the
exception handler.

Is there anything specific that you think is painful in the exception
handler?

> In such cases we could just move the .fixup code into the function
> (inline for older compilers; out-of-line for compilers that support
> CC_HAS_ASM_GOTO_OUTPUT).
> 
> Alternatively we could convert each .fixup code fragment into a proper
> function which returns to a specified resume point in the function, and
> then have the exception handler emulate a call to it like we do with
> int3_emulate_call().

For arm64 this would be somewhat unfortunate for inline asm due to our
calling convention -- we'd have to clobber the LR, and we'd need to
force the creation of a frame record in the caller which would otherwise
not be necessary.

Thanks,
Mark.

Re: [syzbot] upstream test error: KASAN: invalid-access Read in __entry_tramp_text_end

[Josh Poimboeuf]

On Fri, Oct 01, 2021 at 01:27:06PM +0100, Mark Rutland wrote:
> > So we may need to get rid of .fixup altogether.  Especially for arches
> > which support livepatch.
> > 
> > We can replace some of the custom .fixup handlers with generic handlers
> > like x86 does, which do the fixup work in exception context.  This
> > generally works better for more generic work like putting an error code
> > in a certain register and resuming execution at the subsequent
> > instruction.
> 
> I reckon even ignoring the unwind problems this'd be a good thing since
> it'd save on redundant copies of the fixup logic that happen to be
> identical, and the common cases like uaccess all fall into this shape.
> 
> As for how to do that, in the past Peter and I had come up with some
> assembler trickery to get the name of the error code register encoded
> into the extable info:
> 
>   https://lore.kernel.org/lkml/20170207111011.GB28790@leverpostej/
>   https://lore.kernel.org/lkml/20170207160300.GB26173@leverpostej/
>   https://lore.kernel.org/lkml/20170208091250.GT6515@twins.programming.kicks-ass.net/
>
> ... but maybe that's already solved on x86 in a different way?

That's really cool :-) But it might be overkill for x86's needs.  For
the exceptions which rely on handlers rather than anonymous .fixup code,
the register assumptions are hard-coded in the assembler constraints.  I
think that works well enough.

> > However a lot of the .fixup code is rather custom and doesn't
> > necessarily work well with that model.
> 
> Looking at arm64, even where we'd need custom handlers it does appear we
> could mostly do that out-of-line in the exception handler. The more
> exotic cases are largely in out-of-line asm functions, where we can move
> the fixups within the function, after the usual return.
> 
> I reckon we can handle the fixups for load_unaligned_zeropad() in the
> exception handler.
> 
> Is there anything specific that you think is painful in the exception
> handler?

Actually, after looking at all the x86 .fixup usage, I think we can make
this two-pronged approach work.  Either move the .fixup code to an
exception handler (with a hard-coded assembler constraint register) or
put it in the function (out-of-line where possible).  I'll try to work
up some patches (x86 only of course).

-- 
Josh