| Message ID | 20230914080833.50026-1-haibo.li@mediatek.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | kasan:fix access invalid shadow address when input is illegal | expand |
On Thu, Sep 14, 2023 at 10:08 AM 'Haibo Li' via kasan-dev <kasan-dev@googlegroups.com> wrote: > > when the input address is illegal,the corresponding shadow address > from kasan_mem_to_shadow may have no mapping in mmu table. > Access such shadow address causes kernel oops. > Here is a sample about oops on arm64(VA 39bit) with KASAN_SW_TAGS on: > > [ffffffb80aaaaaaa] pgd=000000005d3ce003, p4d=000000005d3ce003, > pud=000000005d3ce003, pmd=0000000000000000 > Internal error: Oops: 0000000096000006 [#1] PREEMPT SMP > Modules linked in: > CPU: 3 PID: 100 Comm: sh Not tainted 6.6.0-rc1-dirty #43 > Hardware name: linux,dummy-virt (DT) > pstate: 80000005 (Nzcv daif -PAN -UAO -TCO -DIT -SSBS BTYPE=--) > pc : __hwasan_load8_noabort+0x5c/0x90 > lr : do_ib_ob+0xf4/0x110 > ffffffb80aaaaaaa is the shadow address for efffff80aaaaaaaa. > The problem is reading invalid shadow in kasan_check_range. > > The generic kasan also has similar oops. > > To fix it,check shadow address by reading it with no fault. > > After this patch,KASAN is able to report invalid memory access > for this case. Hi Haibo, I thought this should be covered by the kasan_non_canonical_hook handler, which prints some additional information about how the GPF could be caused by accessing shadow memory. Does it not work in your case? It might be that we need to add kasan_non_canonical_hook to some other arm64 internal fault handler functions then. Thanks!
On Thu, 14 Sep 2023 16:08:33 +0800 Haibo Li <haibo.li@mediatek.com> wrote: > when the input address is illegal,the corresponding shadow address > from kasan_mem_to_shadow may have no mapping in mmu table. > Access such shadow address causes kernel oops. > Here is a sample about oops on arm64(VA 39bit) with KASAN_SW_TAGS on: > > [ffffffb80aaaaaaa] pgd=000000005d3ce003, p4d=000000005d3ce003, > pud=000000005d3ce003, pmd=0000000000000000 > Internal error: Oops: 0000000096000006 [#1] PREEMPT SMP > Modules linked in: > CPU: 3 PID: 100 Comm: sh Not tainted 6.6.0-rc1-dirty #43 > Hardware name: linux,dummy-virt (DT) > pstate: 80000005 (Nzcv daif -PAN -UAO -TCO -DIT -SSBS BTYPE=--) > pc : __hwasan_load8_noabort+0x5c/0x90 > lr : do_ib_ob+0xf4/0x110 > ffffffb80aaaaaaa is the shadow address for efffff80aaaaaaaa. > The problem is reading invalid shadow in kasan_check_range. > > The generic kasan also has similar oops. > > To fix it,check shadow address by reading it with no fault. > > After this patch,KASAN is able to report invalid memory access > for this case. > Thanks. > --- a/mm/kasan/kasan.h > +++ b/mm/kasan/kasan.h > @@ -304,8 +304,17 @@ static __always_inline bool addr_has_metadata(const void *addr) > #ifdef __HAVE_ARCH_SHADOW_MAP > return (kasan_mem_to_shadow((void *)addr) != NULL); > #else > - return (kasan_reset_tag(addr) >= > - kasan_shadow_to_mem((void *)KASAN_SHADOW_START)); > + u8 *shadow, shadow_val; > + > + if (kasan_reset_tag(addr) < > + kasan_shadow_to_mem((void *)KASAN_SHADOW_START)) > + return false; > + /* use read with nofault to check whether the shadow is accessible */ > + shadow = kasan_mem_to_shadow((void *)addr); > + __get_kernel_nofault(&shadow_val, shadow, u8, fault); > + return true; > +fault: > + return false; > #endif > } Are we able to identify a Fixes: target for this? 9d7b7dd946924de43021f57a8bee122ff0744d93 ("kasan: split out print_report from __kasan_report") altered the code but I expect the bug was present before that commit. Seems this bug has been there for over a year. Can you suggest why it has been discovered after such a lengthy time?
On Thu, Sep 14, 2023 at 8:29 PM Andrew Morton <akpm@linux-foundation.org> wrote: > > --- a/mm/kasan/kasan.h > > +++ b/mm/kasan/kasan.h > > @@ -304,8 +304,17 @@ static __always_inline bool addr_has_metadata(const void *addr) > > #ifdef __HAVE_ARCH_SHADOW_MAP > > return (kasan_mem_to_shadow((void *)addr) != NULL); > > #else > > - return (kasan_reset_tag(addr) >= > > - kasan_shadow_to_mem((void *)KASAN_SHADOW_START)); > > + u8 *shadow, shadow_val; > > + > > + if (kasan_reset_tag(addr) < > > + kasan_shadow_to_mem((void *)KASAN_SHADOW_START)) > > + return false; > > + /* use read with nofault to check whether the shadow is accessible */ > > + shadow = kasan_mem_to_shadow((void *)addr); > > + __get_kernel_nofault(&shadow_val, shadow, u8, fault); > > + return true; > > +fault: > > + return false; > > #endif > > } > > Are we able to identify a Fixes: target for this? > 9d7b7dd946924de43021f57a8bee122ff0744d93 ("kasan: split out > print_report from __kasan_report") altered the code but I expect the > bug was present before that commit. > > Seems this bug has been there for over a year. Can you suggest why it > has been discovered after such a lengthy time? Accessing unmapped memory with KASAN always led to a crash when checking shadow memory. This was reported/discussed before. To improve crash reporting for this case, Jann added kasan_non_canonical_hook and Mark integrated it into arm64. But AFAIU, for some reason, it stopped working. Instead of this patch, we need to figure out why kasan_non_canonical_hook stopped working and fix it. This approach taken by this patch won't work for shadow checks added by compiler instrumentation. It only covers explicitly checked accesses, such as via memcpy, etc.
On Thu, Sep 14, 2023 at 10:35 PM Andrey Konovalov <andreyknvl@gmail.com> wrote: > On Thu, Sep 14, 2023 at 8:29 PM Andrew Morton <akpm@linux-foundation.org> wrote: > > > --- a/mm/kasan/kasan.h > > > +++ b/mm/kasan/kasan.h > > > @@ -304,8 +304,17 @@ static __always_inline bool addr_has_metadata(const void *addr) > > > #ifdef __HAVE_ARCH_SHADOW_MAP > > > return (kasan_mem_to_shadow((void *)addr) != NULL); > > > #else > > > - return (kasan_reset_tag(addr) >= > > > - kasan_shadow_to_mem((void *)KASAN_SHADOW_START)); > > > + u8 *shadow, shadow_val; > > > + > > > + if (kasan_reset_tag(addr) < > > > + kasan_shadow_to_mem((void *)KASAN_SHADOW_START)) > > > + return false; > > > + /* use read with nofault to check whether the shadow is accessible */ > > > + shadow = kasan_mem_to_shadow((void *)addr); > > > + __get_kernel_nofault(&shadow_val, shadow, u8, fault); > > > + return true; > > > +fault: > > > + return false; > > > #endif > > > } > > > > Are we able to identify a Fixes: target for this? > > 9d7b7dd946924de43021f57a8bee122ff0744d93 ("kasan: split out > > print_report from __kasan_report") altered the code but I expect the > > bug was present before that commit. > > > > Seems this bug has been there for over a year. Can you suggest why it > > has been discovered after such a lengthy time? > > Accessing unmapped memory with KASAN always led to a crash when > checking shadow memory. This was reported/discussed before. To improve > crash reporting for this case, Jann added kasan_non_canonical_hook and > Mark integrated it into arm64. But AFAIU, for some reason, it stopped > working. > > Instead of this patch, we need to figure out why > kasan_non_canonical_hook stopped working and fix it. > > This approach taken by this patch won't work for shadow checks added > by compiler instrumentation. It only covers explicitly checked > accesses, such as via memcpy, etc. FWIW, AFAICS kasan_non_canonical_hook() currently only does anything under CONFIG_KASAN_INLINE; I think the idea when I added that was that it assumes that when KASAN checks an access in out-of-line instrumentation or a slowpath, it will do the required checks to avoid this kind of fault?
On Thu, Sep 14, 2023 at 10:41 PM Jann Horn <jannh@google.com> wrote: > > > Accessing unmapped memory with KASAN always led to a crash when > > checking shadow memory. This was reported/discussed before. To improve > > crash reporting for this case, Jann added kasan_non_canonical_hook and > > Mark integrated it into arm64. But AFAIU, for some reason, it stopped > > working. > > > > Instead of this patch, we need to figure out why > > kasan_non_canonical_hook stopped working and fix it. > > > > This approach taken by this patch won't work for shadow checks added > > by compiler instrumentation. It only covers explicitly checked > > accesses, such as via memcpy, etc. > > FWIW, AFAICS kasan_non_canonical_hook() currently only does anything > under CONFIG_KASAN_INLINE; Ah, right. I was thinking about the inline mode, but the patch refers to the issue with the outline mode. However, I just checked kasan_non_canonical_hook for SW_TAGS with the inline mode: it does not work when accessing 0x42ffffb80aaaaaaa, the addr < KASAN_SHADOW_OFFSET check fails. It appears there's something unusual about how instrumentation calculates the shadow address. I didn't investigate further yet. > I think the idea when I added that was that > it assumes that when KASAN checks an access in out-of-line > instrumentation or a slowpath, it will do the required checks to avoid > this kind of fault? Ah, no, KASAN doesn't do it. However, I suppose we could add what the original patch proposes for the outline mode. For the inline mode, it seems to be pointless, as most access checks happen though the compiler inserted code anyway. I also wonder how much slowdown this patch will introduce. Haibo, could you check how much slower the kernel becomes with your patch? If possible, with all GENERIC/SW_TAGS and INLINE/OUTLINE combinations. If the slowdown is large, we can just make kasan_non_canonical_hook work for both modes (and fix it for SW_TAGS).
> On Thu, Sep 14, 2023 at 10:41 PM Jann Horn <jannh@google.com> wrote: > > > > > Accessing unmapped memory with KASAN always led to a crash when > > > checking shadow memory. This was reported/discussed before. To improve > > > crash reporting for this case, Jann added kasan_non_canonical_hook and > > > Mark integrated it into arm64. But AFAIU, for some reason, it stopped > > > working. > > > > > > Instead of this patch, we need to figure out why > > > kasan_non_canonical_hook stopped working and fix it. > > > > > > This approach taken by this patch won't work for shadow checks added > > > by compiler instrumentation. It only covers explicitly checked > > > accesses, such as via memcpy, etc. > > > > FWIW, AFAICS kasan_non_canonical_hook() currently only does anything > > under CONFIG_KASAN_INLINE; > > Ah, right. I was thinking about the inline mode, but the patch refers > to the issue with the outline mode. > > However, I just checked kasan_non_canonical_hook for SW_TAGS with the > inline mode: it does not work when accessing 0x42ffffb80aaaaaaa, the > addr < KASAN_SHADOW_OFFSET check fails. It appears there's something > unusual about how instrumentation calculates the shadow address. I > didn't investigate further yet. > > > I think the idea when I added that was that > > it assumes that when KASAN checks an access in out-of-line > > instrumentation or a slowpath, it will do the required checks to avoid > > this kind of fault? > > Ah, no, KASAN doesn't do it. > > However, I suppose we could add what the original patch proposes for > the outline mode. For the inline mode, it seems to be pointless, as > most access checks happen though the compiler inserted code anyway. > > I also wonder how much slowdown this patch will introduce. > > Haibo, could you check how much slower the kernel becomes with your > patch? If possible, with all GENERIC/SW_TAGS and INLINE/OUTLINE > combinations. > > If the slowdown is large, we can just make kasan_non_canonical_hook > work for both modes (and fix it for SW_TAGS). Thanks. The patch checks each shadow address,so it introduces extra overhead. Now kasan_non_canonical_hook only works for CONFIG_KASAN_INLINE. And CONFIG_KASAN_OUTLINE is set in my case. Is it possible to make kasan_non_canonical_hook works for both INLINE and OUTLINE by simply remove the "#ifdef CONFIG_KASAN_INLINE"? Since kasan_non_canonical_hook is only used after kernel fault,it is better if there is no limit.
> > On Thu, Sep 14, 2023 at 10:41 PM Jann Horn <jannh@google.com> wrote: > > > > > > > Accessing unmapped memory with KASAN always led to a crash when > > > > checking shadow memory. This was reported/discussed before. To improve > > > > crash reporting for this case, Jann added kasan_non_canonical_hook and > > > > Mark integrated it into arm64. But AFAIU, for some reason, it stopped > > > > working. > > > > > > > > Instead of this patch, we need to figure out why > > > > kasan_non_canonical_hook stopped working and fix it. > > > > > > > > This approach taken by this patch won't work for shadow checks added > > > > by compiler instrumentation. It only covers explicitly checked > > > > accesses, such as via memcpy, etc. > > > > > > FWIW, AFAICS kasan_non_canonical_hook() currently only does anything > > > under CONFIG_KASAN_INLINE; > > > > Ah, right. I was thinking about the inline mode, but the patch refers > > to the issue with the outline mode. > > > > However, I just checked kasan_non_canonical_hook for SW_TAGS with the > > inline mode: it does not work when accessing 0x42ffffb80aaaaaaa, the > > addr < KASAN_SHADOW_OFFSET check fails. It appears there's something > > unusual about how instrumentation calculates the shadow address. I > > didn't investigate further yet. Sorry to miss this message. I checked inline mode just now.kasan_non_canonical_hook can print something like below: Unable to handle kernel paging request at virtual address ffffffb80aaaaaaa KASAN: maybe wild-memory-access in range [0xffffff80aaaaaaa0-0xffffff80aaaaaaaf] ... [ffffffb80aaaaaaa] pgd=000000005d3d6003, p4d=000000005d3d6003, pud=000000005d3d6003, pmd=0000000000000000 ... pc : __hwasan_check_x20_67043363+0x4/0x34 lr : do_ib_ob+0x108/0x114 ... Call trace: __hwasan_check_x20_67043363+0x4/0x34 die_selftest+0x68/0x80 param_attr_store+0xec/0x164 module_attr_store+0x34/0x4c sysfs_kf_write+0x78/0x8c kernfs_fop_write_iter+0x154/0x214 vfs_write+0x36c/0x4c4 ksys_write+0x98/0x110 __arm64_sys_write+0x3c/0x48 invoke_syscall+0x58/0x154 el0_svc_common+0xe8/0x120 do_el0_svc_compat+0x2c/0x38 el0_svc_compat+0x34/0x84 el0t_32_sync_handler+0x78/0xb4 el0t_32_sync+0x194/0x198 When addr < KASAN_SHADOW_OFFSET meets,the original addr_has_metadata should return false and trigger kasan_report in kasan_check_range. > > > > > I think the idea when I added that was that > > > it assumes that when KASAN checks an access in out-of-line > > > instrumentation or a slowpath, it will do the required checks to avoid > > > this kind of fault? > > > > Ah, no, KASAN doesn't do it. > > > > However, I suppose we could add what the original patch proposes for > > the outline mode. For the inline mode, it seems to be pointless, as > > most access checks happen though the compiler inserted code anyway. > > > > I also wonder how much slowdown this patch will introduce. > > > > Haibo, could you check how much slower the kernel becomes with your > > patch? If possible, with all GENERIC/SW_TAGS and INLINE/OUTLINE > > combinations. > > > > If the slowdown is large, we can just make kasan_non_canonical_hook > > work for both modes (and fix it for SW_TAGS). > > Thanks. > The patch checks each shadow address,so it introduces extra overhead. > Now kasan_non_canonical_hook only works for CONFIG_KASAN_INLINE. > And CONFIG_KASAN_OUTLINE is set in my case. > Is it possible to make kasan_non_canonical_hook works for both > INLINE and OUTLINE by simply remove the "#ifdef CONFIG_KASAN_INLINE"? > Since kasan_non_canonical_hook is only used after kernel fault,it > is better if there is no limit.
On Fri, Sep 15, 2023 at 4:46 AM 'Haibo Li' via kasan-dev <kasan-dev@googlegroups.com> wrote: > > The patch checks each shadow address,so it introduces extra overhead. Ack. Could still be fine, depends on the overhead. But if the message printed by kasan_non_canonical_hook is good enough for your use case, I would rather stick to that. > Now kasan_non_canonical_hook only works for CONFIG_KASAN_INLINE. > > And CONFIG_KASAN_OUTLINE is set in my case. > > Is it possible to make kasan_non_canonical_hook works for both > INLINE and OUTLINE by simply remove the "#ifdef CONFIG_KASAN_INLINE"? Yes, it should just work if you remove the ifdefs in mm/kasan/report.c and in include/linux/kasan.h. Jann, do you have any objections to enabling kasan_non_canonical_hook for the outline mode too? > Since kasan_non_canonical_hook is only used after kernel fault,it > is better if there is no limit.
On Fri, Sep 15, 2023 at 11:40 AM 'Haibo Li' via kasan-dev <kasan-dev@googlegroups.com> wrote: > > I checked inline mode just now.kasan_non_canonical_hook can print > something like below: > > Unable to handle kernel paging request at virtual address ffffffb80aaaaaaa > KASAN: maybe wild-memory-access in range [0xffffff80aaaaaaa0-0xffffff80aaaaaaaf] > > When addr < KASAN_SHADOW_OFFSET meets,the original addr_has_metadata should return false > and trigger kasan_report in kasan_check_range. It should, but I don't think it always does. But if it works for you, let's leave it at that. I'll double check why it failed for me later. Thanks!
On Fri, Sep 15, 2023 at 6:51 PM Andrey Konovalov <andreyknvl@gmail.com> wrote: > On Fri, Sep 15, 2023 at 4:46 AM 'Haibo Li' via kasan-dev > <kasan-dev@googlegroups.com> wrote: > > > > The patch checks each shadow address,so it introduces extra overhead. > > Ack. Could still be fine, depends on the overhead. > > But if the message printed by kasan_non_canonical_hook is good enough > for your use case, I would rather stick to that. > > > Now kasan_non_canonical_hook only works for CONFIG_KASAN_INLINE. > > > > And CONFIG_KASAN_OUTLINE is set in my case. > > > > Is it possible to make kasan_non_canonical_hook works for both > > INLINE and OUTLINE by simply remove the "#ifdef CONFIG_KASAN_INLINE"? > > Yes, it should just work if you remove the ifdefs in mm/kasan/report.c > and in include/linux/kasan.h. > > Jann, do you have any objections to enabling kasan_non_canonical_hook > for the outline mode too? No objections from me.
> On Fri, Sep 15, 2023 at 4:46 AM 'Haibo Li' via kasan-dev > <kasan-dev@googlegroups.com> wrote: > > > > The patch checks each shadow address,so it introduces extra overhead. > > Ack. Could still be fine, depends on the overhead. > I do a simple test by reading memory. Read 4096 memory by loop and the reading unit is 8 bytes. __hwasan_load8_noabort is called 512(4096/8) times. Measure the time of memory read. Here is the result on ARM CA7X(repeat 100 times): ---------------min-------max-----avg---- before patch | 77.3ms | 80.6ms | 79.2ms| after patch | 77.2ms | 80.7ms | 79.2ms| ---------------------------------------- There is no obvious drop in this scenario. It may differ in different arch. just for information if you are intrested in it. > But if the message printed by kasan_non_canonical_hook is good enough > for your use case, I would rather stick to that.
> On Fri, Sep 15, 2023 at 6:51 PM Andrey Konovalov <andreyknvl@gmail.com> wrote: > > On Fri, Sep 15, 2023 at 4:46 AM 'Haibo Li' via kasan-dev > > <kasan-dev@googlegroups.com> wrote: > > > > > > The patch checks each shadow address,so it introduces extra overhead. > > > > Ack. Could still be fine, depends on the overhead. > > > > But if the message printed by kasan_non_canonical_hook is good enough > > for your use case, I would rather stick to that. If we check shadow address before invalid access, we get below message before oops: " BUG: KASAN: invalid-access in do_ib_ob+0xf4/0x110 Read of size 8 at addr caffff80aaaaaaaa by task sh/100 " We get below message while using kasan_non_canonical_hook: " Unable to handle kernel paging request at virtual address ffffff80aaaaaaaa KASAN: maybe wild-memory-access in range [0xfffffc0aaaaaaaa0-0xfffffc0aaaaaaaaf] " Both indicate the original accessed address which causes oops. > > > > > Now kasan_non_canonical_hook only works for CONFIG_KASAN_INLINE. > > > > > > And CONFIG_KASAN_OUTLINE is set in my case. > > > > > > Is it possible to make kasan_non_canonical_hook works for both > > > INLINE and OUTLINE by simply remove the "#ifdef CONFIG_KASAN_INLINE"? > > > > Yes, it should just work if you remove the ifdefs in mm/kasan/report.c > > and in include/linux/kasan.h. > > > > Jann, do you have any objections to enabling kasan_non_canonical_hook > > for the outline mode too? > > No objections from me. Thanks. Shall I send a new patch to fix this problem by using kasan_non_canonical_hook
diff --git a/mm/kasan/kasan.h b/mm/kasan/kasan.h index f70e3d7a602e..bd30f35e18b2 100644 --- a/mm/kasan/kasan.h +++ b/mm/kasan/kasan.h @@ -304,8 +304,17 @@ static __always_inline bool addr_has_metadata(const void *addr) #ifdef __HAVE_ARCH_SHADOW_MAP return (kasan_mem_to_shadow((void *)addr) != NULL); #else - return (kasan_reset_tag(addr) >= - kasan_shadow_to_mem((void *)KASAN_SHADOW_START)); + u8 *shadow, shadow_val; + + if (kasan_reset_tag(addr) < + kasan_shadow_to_mem((void *)KASAN_SHADOW_START)) + return false; + /* use read with nofault to check whether the shadow is accessible */ + shadow = kasan_mem_to_shadow((void *)addr); + __get_kernel_nofault(&shadow_val, shadow, u8, fault); + return true; +fault: + return false; #endif }
when the input address is illegal,the corresponding shadow address from kasan_mem_to_shadow may have no mapping in mmu table. Access such shadow address causes kernel oops. Here is a sample about oops on arm64(VA 39bit) with KASAN_SW_TAGS on: [ffffffb80aaaaaaa] pgd=000000005d3ce003, p4d=000000005d3ce003, pud=000000005d3ce003, pmd=0000000000000000 Internal error: Oops: 0000000096000006 [#1] PREEMPT SMP Modules linked in: CPU: 3 PID: 100 Comm: sh Not tainted 6.6.0-rc1-dirty #43 Hardware name: linux,dummy-virt (DT) pstate: 80000005 (Nzcv daif -PAN -UAO -TCO -DIT -SSBS BTYPE=--) pc : __hwasan_load8_noabort+0x5c/0x90 lr : do_ib_ob+0xf4/0x110 ffffffb80aaaaaaa is the shadow address for efffff80aaaaaaaa. The problem is reading invalid shadow in kasan_check_range. The generic kasan also has similar oops. To fix it,check shadow address by reading it with no fault. After this patch,KASAN is able to report invalid memory access for this case. Signed-off-by: Haibo Li <haibo.li@mediatek.com> --- mm/kasan/kasan.h | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-)