Bug 212513 - KASAN (hw-tags): annotate no_sanitize_address functions
Summary: KASAN (hw-tags): annotate no_sanitize_address functions
Status: NEW
Alias: None
Product: Memory Management
Classification: Unclassified
Component: Sanitizers (show other bugs)
Hardware: All Linux
: P1 normal
Assignee: MM/Sanitizers virtual assignee
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2021-03-31 21:52 UTC by Andrey Konovalov
Modified: 2021-04-02 21:11 UTC (History)
3 users (show)

See Also:
Kernel Version: upstream
Subsystem:
Regression: No
Bisected commit-id:


Attachments

Description Andrey Konovalov 2021-03-31 21:52:39 UTC
For software modes, KASAN relies on the no_sanitize_address function attribute to disable instrumentation in some functions. There a few annotations that enable this attribute: __no_sanitize_address, __no_kasan_or_inline, __no_sanitize_or_inline.

For the HW_TAGS mode, nothing is done at the moment. Accesses in these annotated functions are still checked and this might lead to false-positive reports. We need to annotate those functions with kasan_reset_tag() to ignore the accesses.

Note, that some of the no_sanitize_address functions (e.g. read_word_at_a_time()) perform custom KASAN checks with kasan_check_read/write(), which are currently ignored for HW_TAGS. This needs to be addressed as well, when adding kasan_reset_tag() annotations.
Comment 1 Andrey Konovalov 2021-03-31 21:57:11 UTC
Here's a draft of test case that checks functions from include/asm-generic/rwonce.h:

diff --git a/lib/test_kasan.c b/lib/test_kasan.c
index bf9225002a7e..b9d942ae5c7a 100644
--- a/lib/test_kasan.c
+++ b/lib/test_kasan.c
@@ -1062,6 +1062,37 @@ static void match_all_mem_tag(struct kunit *test)
        kfree(ptr);
 }
 
+/* Check that different variants of READ/WRITE_ONCE() work properly. */
+/* XXX: some of these fail in QEMU due to:
+ * https://bugs.launchpad.net/qemu/+bug/1921948 */
+static void access_once(struct kunit *test)
+{
+       char *data;
+       unsigned long *aligned_ptr, *unaligned_ptr;
+       size_t size = 128 - KASAN_GRANULE_SIZE;
+
+       data = kmalloc(size, GFP_KERNEL);
+       aligned_ptr = (unsigned long *)(data + size);
+       unaligned_ptr = (unsigned long *)(data + size - 4);
+
+       KUNIT_EXPECT_KASAN_FAIL(test, WRITE_ONCE(*aligned_ptr, 0));
+       KUNIT_EXPECT_KASAN_FAIL(test, READ_ONCE(*aligned_ptr));
+
+       KUNIT_EXPECT_KASAN_FAIL(test, WRITE_ONCE(*unaligned_ptr, 0));
+       KUNIT_EXPECT_KASAN_FAIL(test, READ_ONCE(*unaligned_ptr));
+
+       /* XXX: these fail due to missing kasan_reset_tag(). */
+       READ_ONCE_NOCHECK(*aligned_ptr);
+       READ_ONCE_NOCHECK(*unaligned_ptr);
+
+       KUNIT_EXPECT_KASAN_FAIL(test,
+               kasan_int_result = read_word_at_a_time(aligned_ptr));
+       /* XXX: this fails due to missing kasan_reset_tag(). */
+       kasan_int_result = read_word_at_a_time(unaligned_ptr);
+
+       kfree(data);
+}
+
 static struct kunit_case kasan_kunit_test_cases[] = {
        KUNIT_CASE(kmalloc_oob_right),
        KUNIT_CASE(kmalloc_oob_left),
@@ -1111,6 +1142,7 @@ static struct kunit_case kasan_kunit_test_cases[] = {
        KUNIT_CASE(match_all_not_assigned),
        KUNIT_CASE(match_all_ptr_tag),
        KUNIT_CASE(match_all_mem_tag),
+       KUNIT_CASE(access_once),
        {}
 };

Note, that there are more functions that need to be annotated. Grep for __no_sanitize_address, __no_kasan_or_inline, __no_sanitize_or_inline.

Note, that the test relies on the patch below, which is not yet in the mainline:

https://lore.kernel.org/linux-mm/CANpmjNMpT0rYKfywkGvqLy8tk3iP6wAuGxHpHVJA77+EG4c5Gg@mail.gmail.com/T/#t
Comment 2 Peter Collingbourne 2021-04-01 02:10:44 UTC
An alternative idea would be to create a compiler attribute that does

mrs reg, tco
msr tco, #1

on entry and

msr tco, reg

on exit.
Comment 3 Andrey Konovalov 2021-04-02 14:52:15 UTC
This sounds like an idea worth considering. This way we don't need to add more annotations. But we would need to bump the compiler version requirement.
Comment 4 Peter Collingbourne 2021-04-02 14:58:34 UTC
One complication though is that it would need alternatives in order to support hardware without MTE, which implies that the compiler would need to know about alternatives.

Perhaps we could use __attribute__((cleanup)) to run the code on exit instead of an attribute. It looks like this attribute is allowed in the kernel, e.g. it's used in include/linux/kcsan-checks.h.
Comment 5 Marco Elver 2021-04-02 21:11:45 UTC
Re __attribute__((cleanup)): "allowed" under very restricted circumstances. I'd rather say it's "tolerated" :-)

For KCSAN I decided to use it because the abstraction it's hidden behind is not explicitly about resource management, but a special assertion. Furthermore, we know that all compilers that support KCSAN support the cleanup attribute (although these days, with the kernel requiring at least GCC 4.9 and Clang 10, this isn't a problem either). Last but not least, nobody really objected I believe because KCSAN is a debugging tool that is usually off and nobody would notice.

My guess is that any attempts to diversify the kernel's resource management strategies (e.g. vs normal "goto out" or some such) with __attribute__((cleanup)) will be frowned upon and be rejected -- which I'd also agree with because it'd create inconsistencies and in some cases also makes it hard to reason about what is executed on function return vs. a simple "goto out" (at least that's one argument I recall hearing somewhere), which can be quite crucial in certain contexts (e.g. see all the 'noinstr' functions restrictions).

So, my guess is that if the usecase is closer to KCSAN's usecase, then it should be fine, i.e. it's hidden behind some abstraction that is for KASAN_HW_TAGS only, and not about resource management explicitly.

Note You need to log in before you can comment on or make changes to this bug.