| Message ID | dd061dfca76dbf86af13393edacd37e0c75b6f4a.1609871239.git.andreyknvl@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | kasan: HW_TAGS tests support and fixes | expand |
On Tue, Jan 05, 2021 at 07:27:49PM +0100, Andrey Konovalov wrote: > diff --git a/arch/arm64/mm/fault.c b/arch/arm64/mm/fault.c > index 3c40da479899..57d3f165d907 100644 > --- a/arch/arm64/mm/fault.c > +++ b/arch/arm64/mm/fault.c > @@ -302,12 +302,20 @@ static void die_kernel_fault(const char *msg, unsigned long addr, > static void report_tag_fault(unsigned long addr, unsigned int esr, > struct pt_regs *regs) > { > - bool is_write = ((esr & ESR_ELx_WNR) >> ESR_ELx_WNR_SHIFT) != 0; > + static bool reported; > + bool is_write; > + > + if (READ_ONCE(reported)) > + return; > + > + if (mte_report_once()) > + WRITE_ONCE(reported, true); I guess the assumption here is that you don't get any report before the tests start and temporarily set report_once to false. It's probably fine, if we get a tag check failure we'd notice in the logs anyway. > /* > * SAS bits aren't set for all faults reported in EL1, so we can't > * find out access size. > */ > + is_write = ((esr & ESR_ELx_WNR) >> ESR_ELx_WNR_SHIFT) != 0; I now noticed, you could write this in a shorter way: is_write = !!(esr & ESR_ELx_WNR); > kasan_report(addr, 0, is_write, regs->pc); > } The patch looks fine to me. Reviewed-by: Catalin Marinas <catalin.marinas@arm.com>
On Tue, Jan 12, 2021 at 8:01 PM Catalin Marinas <catalin.marinas@arm.com> wrote: > > On Tue, Jan 05, 2021 at 07:27:49PM +0100, Andrey Konovalov wrote: > > diff --git a/arch/arm64/mm/fault.c b/arch/arm64/mm/fault.c > > index 3c40da479899..57d3f165d907 100644 > > --- a/arch/arm64/mm/fault.c > > +++ b/arch/arm64/mm/fault.c > > @@ -302,12 +302,20 @@ static void die_kernel_fault(const char *msg, unsigned long addr, > > static void report_tag_fault(unsigned long addr, unsigned int esr, > > struct pt_regs *regs) > > { > > - bool is_write = ((esr & ESR_ELx_WNR) >> ESR_ELx_WNR_SHIFT) != 0; > > + static bool reported; > > + bool is_write; > > + > > + if (READ_ONCE(reported)) > > + return; > > + > > + if (mte_report_once()) > > + WRITE_ONCE(reported, true); > > I guess the assumption here is that you don't get any report before the > tests start and temporarily set report_once to false. It's probably > fine, if we get a tag check failure we'd notice in the logs anyway. Good point. I'll add a note in a comment in v4. > > /* > > * SAS bits aren't set for all faults reported in EL1, so we can't > > * find out access size. > > */ > > + is_write = ((esr & ESR_ELx_WNR) >> ESR_ELx_WNR_SHIFT) != 0; > > I now noticed, you could write this in a shorter way: > > is_write = !!(esr & ESR_ELx_WNR); > > > kasan_report(addr, 0, is_write, regs->pc); > > } Will do in v4. > The patch looks fine to me. > > Reviewed-by: Catalin Marinas <catalin.marinas@arm.com> Thanks!
Hi Andrey, On 1/5/21 6:27 PM, Andrey Konovalov wrote: > On a high level, this patch allows running KUnit KASAN tests with the > hardware tag-based KASAN mode. > > Internally, this change reenables tag checking at the end of each KASAN > test that triggers a tag fault and leads to tag checking being disabled. > > With this patch KASAN tests are still failing for the hardware tag-based > mode; fixes come in the next few patches. > A part what Catalin noticed already: Reviewed-by: Vincenzo Frascino <vincenzo.frascino@arm.com> > Signed-off-by: Andrey Konovalov <andreyknvl@google.com> > Link: https://linux-review.googlesource.com/id/Id94dc9eccd33b23cda4950be408c27f879e474c8 > --- > arch/arm64/include/asm/memory.h | 1 + > arch/arm64/include/asm/mte-kasan.h | 12 +++++++++ > arch/arm64/kernel/mte.c | 12 +++++++++ > arch/arm64/mm/fault.c | 16 +++++++----- > lib/Kconfig.kasan | 4 +-- > lib/test_kasan.c | 42 +++++++++++++++++++++--------- > mm/kasan/kasan.h | 9 +++++++ > 7 files changed, 75 insertions(+), 21 deletions(-) > > diff --git a/arch/arm64/include/asm/memory.h b/arch/arm64/include/asm/memory.h > index 18fce223b67b..cedfc9e97bcc 100644 > --- a/arch/arm64/include/asm/memory.h > +++ b/arch/arm64/include/asm/memory.h > @@ -232,6 +232,7 @@ static inline const void *__tag_set(const void *addr, u8 tag) > > #ifdef CONFIG_KASAN_HW_TAGS > #define arch_enable_tagging() mte_enable_kernel() > +#define arch_set_tagging_report_once(state) mte_set_report_once(state) > #define arch_init_tags(max_tag) mte_init_tags(max_tag) > #define arch_get_random_tag() mte_get_random_tag() > #define arch_get_mem_tag(addr) mte_get_mem_tag(addr) > diff --git a/arch/arm64/include/asm/mte-kasan.h b/arch/arm64/include/asm/mte-kasan.h > index 26349a4b5e2e..3748d5bb88c0 100644 > --- a/arch/arm64/include/asm/mte-kasan.h > +++ b/arch/arm64/include/asm/mte-kasan.h > @@ -32,6 +32,9 @@ void *mte_set_mem_tag_range(void *addr, size_t size, u8 tag); > void mte_enable_kernel(void); > void mte_init_tags(u64 max_tag); > > +void mte_set_report_once(bool state); > +bool mte_report_once(void); > + > #else /* CONFIG_ARM64_MTE */ > > static inline u8 mte_get_ptr_tag(void *ptr) > @@ -60,6 +63,15 @@ static inline void mte_init_tags(u64 max_tag) > { > } > > +static inline void mte_set_report_once(bool state) > +{ > +} > + > +static inline bool mte_report_once(void) > +{ > + return false; > +} > + > #endif /* CONFIG_ARM64_MTE */ > > #endif /* __ASSEMBLY__ */ > diff --git a/arch/arm64/kernel/mte.c b/arch/arm64/kernel/mte.c > index dc9ada64feed..c63b3d7a3cd9 100644 > --- a/arch/arm64/kernel/mte.c > +++ b/arch/arm64/kernel/mte.c > @@ -25,6 +25,8 @@ > > u64 gcr_kernel_excl __ro_after_init; > > +static bool report_fault_once = true; > + > static void mte_sync_page_tags(struct page *page, pte_t *ptep, bool check_swap) > { > pte_t old_pte = READ_ONCE(*ptep); > @@ -158,6 +160,16 @@ void mte_enable_kernel(void) > isb(); > } > > +void mte_set_report_once(bool state) > +{ > + WRITE_ONCE(report_fault_once, state); > +} > + > +bool mte_report_once(void) > +{ > + return READ_ONCE(report_fault_once); > +} > + > static void update_sctlr_el1_tcf0(u64 tcf0) > { > /* ISB required for the kernel uaccess routines */ > diff --git a/arch/arm64/mm/fault.c b/arch/arm64/mm/fault.c > index 3c40da479899..57d3f165d907 100644 > --- a/arch/arm64/mm/fault.c > +++ b/arch/arm64/mm/fault.c > @@ -302,12 +302,20 @@ static void die_kernel_fault(const char *msg, unsigned long addr, > static void report_tag_fault(unsigned long addr, unsigned int esr, > struct pt_regs *regs) > { > - bool is_write = ((esr & ESR_ELx_WNR) >> ESR_ELx_WNR_SHIFT) != 0; > + static bool reported; > + bool is_write; > + > + if (READ_ONCE(reported)) > + return; > + > + if (mte_report_once()) > + WRITE_ONCE(reported, true); > > /* > * SAS bits aren't set for all faults reported in EL1, so we can't > * find out access size. > */ > + is_write = ((esr & ESR_ELx_WNR) >> ESR_ELx_WNR_SHIFT) != 0; > kasan_report(addr, 0, is_write, regs->pc); > } > #else > @@ -319,12 +327,8 @@ static inline void report_tag_fault(unsigned long addr, unsigned int esr, > static void do_tag_recovery(unsigned long addr, unsigned int esr, > struct pt_regs *regs) > { > - static bool reported; > > - if (!READ_ONCE(reported)) { > - report_tag_fault(addr, esr, regs); > - WRITE_ONCE(reported, true); > - } > + report_tag_fault(addr, esr, regs); > > /* > * Disable MTE Tag Checking on the local CPU for the current EL. > diff --git a/lib/Kconfig.kasan b/lib/Kconfig.kasan > index f5fa4ba126bf..3091432acb0a 100644 > --- a/lib/Kconfig.kasan > +++ b/lib/Kconfig.kasan > @@ -190,11 +190,11 @@ config KASAN_KUNIT_TEST > kernel debugging features like KASAN. > > For more information on KUnit and unit tests in general, please refer > - to the KUnit documentation in Documentation/dev-tools/kunit > + to the KUnit documentation in Documentation/dev-tools/kunit. > > config TEST_KASAN_MODULE > tristate "KUnit-incompatible tests of KASAN bug detection capabilities" > - depends on m && KASAN > + depends on m && KASAN && !KASAN_HW_TAGS > help > This is a part of the KASAN test suite that is incompatible with > KUnit. Currently includes tests that do bad copy_from/to_user > diff --git a/lib/test_kasan.c b/lib/test_kasan.c > index f1eda0bcc780..dd3d2f95c24e 100644 > --- a/lib/test_kasan.c > +++ b/lib/test_kasan.c > @@ -41,16 +41,20 @@ static bool multishot; > > /* > * Temporarily enable multi-shot mode. Otherwise, KASAN would only report the > - * first detected bug and panic the kernel if panic_on_warn is enabled. > + * first detected bug and panic the kernel if panic_on_warn is enabled. For > + * hardware tag-based KASAN also allow tag checking to be reenabled for each > + * test, see the comment for KUNIT_EXPECT_KASAN_FAIL(). > */ > static int kasan_test_init(struct kunit *test) > { > multishot = kasan_save_enable_multi_shot(); > + hw_set_tagging_report_once(false); > return 0; > } > > static void kasan_test_exit(struct kunit *test) > { > + hw_set_tagging_report_once(true); > kasan_restore_multi_shot(multishot); > } > > @@ -59,19 +63,31 @@ static void kasan_test_exit(struct kunit *test) > * KASAN report; causes a test failure otherwise. This relies on a KUnit > * resource named "kasan_data". Do not use this name for KUnit resources > * outside of KASAN tests. > + * > + * For hardware tag-based KASAN, when a tag fault happens, tag checking is > + * normally auto-disabled. When this happens, this test handler reenables > + * tag checking. As tag checking can be only disabled or enabled per CPU, this > + * handler disables migration (preemption). > */ > -#define KUNIT_EXPECT_KASAN_FAIL(test, expression) do { \ > - fail_data.report_expected = true; \ > - fail_data.report_found = false; \ > - kunit_add_named_resource(test, \ > - NULL, \ > - NULL, \ > - &resource, \ > - "kasan_data", &fail_data); \ > - expression; \ > - KUNIT_EXPECT_EQ(test, \ > - fail_data.report_expected, \ > - fail_data.report_found); \ > +#define KUNIT_EXPECT_KASAN_FAIL(test, expression) do { \ > + if (IS_ENABLED(CONFIG_KASAN_HW_TAGS)) \ > + migrate_disable(); \ > + fail_data.report_expected = true; \ > + fail_data.report_found = false; \ > + kunit_add_named_resource(test, \ > + NULL, \ > + NULL, \ > + &resource, \ > + "kasan_data", &fail_data); \ > + expression; \ > + KUNIT_EXPECT_EQ(test, \ > + fail_data.report_expected, \ > + fail_data.report_found); \ > + if (IS_ENABLED(CONFIG_KASAN_HW_TAGS)) { \ > + if (fail_data.report_found) \ > + hw_enable_tagging(); \ > + migrate_enable(); \ > + } \ > } while (0) > > static void kmalloc_oob_right(struct kunit *test) > diff --git a/mm/kasan/kasan.h b/mm/kasan/kasan.h > index c3fb9bf241d3..292dfbc37deb 100644 > --- a/mm/kasan/kasan.h > +++ b/mm/kasan/kasan.h > @@ -280,6 +280,9 @@ static inline const void *arch_kasan_set_tag(const void *addr, u8 tag) > #ifndef arch_init_tags > #define arch_init_tags(max_tag) > #endif > +#ifndef arch_set_tagging_report_once > +#define arch_set_tagging_report_once(state) > +#endif > #ifndef arch_get_random_tag > #define arch_get_random_tag() (0xFF) > #endif > @@ -292,10 +295,16 @@ static inline const void *arch_kasan_set_tag(const void *addr, u8 tag) > > #define hw_enable_tagging() arch_enable_tagging() > #define hw_init_tags(max_tag) arch_init_tags(max_tag) > +#define hw_set_tagging_report_once(state) arch_set_tagging_report_once(state) > #define hw_get_random_tag() arch_get_random_tag() > #define hw_get_mem_tag(addr) arch_get_mem_tag(addr) > #define hw_set_mem_tag_range(addr, size, tag) arch_set_mem_tag_range((addr), (size), (tag)) > > +#else /* CONFIG_KASAN_HW_TAGS */ > + > +#define hw_enable_tagging() > +#define hw_set_tagging_report_once(state) > + > #endif /* CONFIG_KASAN_HW_TAGS */ > > #ifdef CONFIG_KASAN_SW_TAGS >
diff --git a/arch/arm64/include/asm/memory.h b/arch/arm64/include/asm/memory.h index 18fce223b67b..cedfc9e97bcc 100644 --- a/arch/arm64/include/asm/memory.h +++ b/arch/arm64/include/asm/memory.h @@ -232,6 +232,7 @@ static inline const void *__tag_set(const void *addr, u8 tag) #ifdef CONFIG_KASAN_HW_TAGS #define arch_enable_tagging() mte_enable_kernel() +#define arch_set_tagging_report_once(state) mte_set_report_once(state) #define arch_init_tags(max_tag) mte_init_tags(max_tag) #define arch_get_random_tag() mte_get_random_tag() #define arch_get_mem_tag(addr) mte_get_mem_tag(addr) diff --git a/arch/arm64/include/asm/mte-kasan.h b/arch/arm64/include/asm/mte-kasan.h index 26349a4b5e2e..3748d5bb88c0 100644 --- a/arch/arm64/include/asm/mte-kasan.h +++ b/arch/arm64/include/asm/mte-kasan.h @@ -32,6 +32,9 @@ void *mte_set_mem_tag_range(void *addr, size_t size, u8 tag); void mte_enable_kernel(void); void mte_init_tags(u64 max_tag); +void mte_set_report_once(bool state); +bool mte_report_once(void); + #else /* CONFIG_ARM64_MTE */ static inline u8 mte_get_ptr_tag(void *ptr) @@ -60,6 +63,15 @@ static inline void mte_init_tags(u64 max_tag) { } +static inline void mte_set_report_once(bool state) +{ +} + +static inline bool mte_report_once(void) +{ + return false; +} + #endif /* CONFIG_ARM64_MTE */ #endif /* __ASSEMBLY__ */ diff --git a/arch/arm64/kernel/mte.c b/arch/arm64/kernel/mte.c index dc9ada64feed..c63b3d7a3cd9 100644 --- a/arch/arm64/kernel/mte.c +++ b/arch/arm64/kernel/mte.c @@ -25,6 +25,8 @@ u64 gcr_kernel_excl __ro_after_init; +static bool report_fault_once = true; + static void mte_sync_page_tags(struct page *page, pte_t *ptep, bool check_swap) { pte_t old_pte = READ_ONCE(*ptep); @@ -158,6 +160,16 @@ void mte_enable_kernel(void) isb(); } +void mte_set_report_once(bool state) +{ + WRITE_ONCE(report_fault_once, state); +} + +bool mte_report_once(void) +{ + return READ_ONCE(report_fault_once); +} + static void update_sctlr_el1_tcf0(u64 tcf0) { /* ISB required for the kernel uaccess routines */ diff --git a/arch/arm64/mm/fault.c b/arch/arm64/mm/fault.c index 3c40da479899..57d3f165d907 100644 --- a/arch/arm64/mm/fault.c +++ b/arch/arm64/mm/fault.c @@ -302,12 +302,20 @@ static void die_kernel_fault(const char *msg, unsigned long addr, static void report_tag_fault(unsigned long addr, unsigned int esr, struct pt_regs *regs) { - bool is_write = ((esr & ESR_ELx_WNR) >> ESR_ELx_WNR_SHIFT) != 0; + static bool reported; + bool is_write; + + if (READ_ONCE(reported)) + return; + + if (mte_report_once()) + WRITE_ONCE(reported, true); /* * SAS bits aren't set for all faults reported in EL1, so we can't * find out access size. */ + is_write = ((esr & ESR_ELx_WNR) >> ESR_ELx_WNR_SHIFT) != 0; kasan_report(addr, 0, is_write, regs->pc); } #else @@ -319,12 +327,8 @@ static inline void report_tag_fault(unsigned long addr, unsigned int esr, static void do_tag_recovery(unsigned long addr, unsigned int esr, struct pt_regs *regs) { - static bool reported; - if (!READ_ONCE(reported)) { - report_tag_fault(addr, esr, regs); - WRITE_ONCE(reported, true); - } + report_tag_fault(addr, esr, regs); /* * Disable MTE Tag Checking on the local CPU for the current EL. diff --git a/lib/Kconfig.kasan b/lib/Kconfig.kasan index f5fa4ba126bf..3091432acb0a 100644 --- a/lib/Kconfig.kasan +++ b/lib/Kconfig.kasan @@ -190,11 +190,11 @@ config KASAN_KUNIT_TEST kernel debugging features like KASAN. For more information on KUnit and unit tests in general, please refer - to the KUnit documentation in Documentation/dev-tools/kunit + to the KUnit documentation in Documentation/dev-tools/kunit. config TEST_KASAN_MODULE tristate "KUnit-incompatible tests of KASAN bug detection capabilities" - depends on m && KASAN + depends on m && KASAN && !KASAN_HW_TAGS help This is a part of the KASAN test suite that is incompatible with KUnit. Currently includes tests that do bad copy_from/to_user diff --git a/lib/test_kasan.c b/lib/test_kasan.c index f1eda0bcc780..dd3d2f95c24e 100644 --- a/lib/test_kasan.c +++ b/lib/test_kasan.c @@ -41,16 +41,20 @@ static bool multishot; /* * Temporarily enable multi-shot mode. Otherwise, KASAN would only report the - * first detected bug and panic the kernel if panic_on_warn is enabled. + * first detected bug and panic the kernel if panic_on_warn is enabled. For + * hardware tag-based KASAN also allow tag checking to be reenabled for each + * test, see the comment for KUNIT_EXPECT_KASAN_FAIL(). */ static int kasan_test_init(struct kunit *test) { multishot = kasan_save_enable_multi_shot(); + hw_set_tagging_report_once(false); return 0; } static void kasan_test_exit(struct kunit *test) { + hw_set_tagging_report_once(true); kasan_restore_multi_shot(multishot); } @@ -59,19 +63,31 @@ static void kasan_test_exit(struct kunit *test) * KASAN report; causes a test failure otherwise. This relies on a KUnit * resource named "kasan_data". Do not use this name for KUnit resources * outside of KASAN tests. + * + * For hardware tag-based KASAN, when a tag fault happens, tag checking is + * normally auto-disabled. When this happens, this test handler reenables + * tag checking. As tag checking can be only disabled or enabled per CPU, this + * handler disables migration (preemption). */ -#define KUNIT_EXPECT_KASAN_FAIL(test, expression) do { \ - fail_data.report_expected = true; \ - fail_data.report_found = false; \ - kunit_add_named_resource(test, \ - NULL, \ - NULL, \ - &resource, \ - "kasan_data", &fail_data); \ - expression; \ - KUNIT_EXPECT_EQ(test, \ - fail_data.report_expected, \ - fail_data.report_found); \ +#define KUNIT_EXPECT_KASAN_FAIL(test, expression) do { \ + if (IS_ENABLED(CONFIG_KASAN_HW_TAGS)) \ + migrate_disable(); \ + fail_data.report_expected = true; \ + fail_data.report_found = false; \ + kunit_add_named_resource(test, \ + NULL, \ + NULL, \ + &resource, \ + "kasan_data", &fail_data); \ + expression; \ + KUNIT_EXPECT_EQ(test, \ + fail_data.report_expected, \ + fail_data.report_found); \ + if (IS_ENABLED(CONFIG_KASAN_HW_TAGS)) { \ + if (fail_data.report_found) \ + hw_enable_tagging(); \ + migrate_enable(); \ + } \ } while (0) static void kmalloc_oob_right(struct kunit *test) diff --git a/mm/kasan/kasan.h b/mm/kasan/kasan.h index c3fb9bf241d3..292dfbc37deb 100644 --- a/mm/kasan/kasan.h +++ b/mm/kasan/kasan.h @@ -280,6 +280,9 @@ static inline const void *arch_kasan_set_tag(const void *addr, u8 tag) #ifndef arch_init_tags #define arch_init_tags(max_tag) #endif +#ifndef arch_set_tagging_report_once +#define arch_set_tagging_report_once(state) +#endif #ifndef arch_get_random_tag #define arch_get_random_tag() (0xFF) #endif @@ -292,10 +295,16 @@ static inline const void *arch_kasan_set_tag(const void *addr, u8 tag) #define hw_enable_tagging() arch_enable_tagging() #define hw_init_tags(max_tag) arch_init_tags(max_tag) +#define hw_set_tagging_report_once(state) arch_set_tagging_report_once(state) #define hw_get_random_tag() arch_get_random_tag() #define hw_get_mem_tag(addr) arch_get_mem_tag(addr) #define hw_set_mem_tag_range(addr, size, tag) arch_set_mem_tag_range((addr), (size), (tag)) +#else /* CONFIG_KASAN_HW_TAGS */ + +#define hw_enable_tagging() +#define hw_set_tagging_report_once(state) + #endif /* CONFIG_KASAN_HW_TAGS */ #ifdef CONFIG_KASAN_SW_TAGS
On a high level, this patch allows running KUnit KASAN tests with the hardware tag-based KASAN mode. Internally, this change reenables tag checking at the end of each KASAN test that triggers a tag fault and leads to tag checking being disabled. With this patch KASAN tests are still failing for the hardware tag-based mode; fixes come in the next few patches. Signed-off-by: Andrey Konovalov <andreyknvl@google.com> Link: https://linux-review.googlesource.com/id/Id94dc9eccd33b23cda4950be408c27f879e474c8 --- arch/arm64/include/asm/memory.h | 1 + arch/arm64/include/asm/mte-kasan.h | 12 +++++++++ arch/arm64/kernel/mte.c | 12 +++++++++ arch/arm64/mm/fault.c | 16 +++++++----- lib/Kconfig.kasan | 4 +-- lib/test_kasan.c | 42 +++++++++++++++++++++--------- mm/kasan/kasan.h | 9 +++++++ 7 files changed, 75 insertions(+), 21 deletions(-)