linux-kernel - Re: [PATCH v2] kasan: add kunit tests for kmalloc_track_caller, kmalloc_node_track

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <ef2eac3f-5a26-4397-9bcd-e0d7d652b282@gmail.com>
Date: Mon, 14 Oct 2024 20:35:56 +0530
From: Nihar Chaithanya <niharchaithanya@...il.com>
To: Andrey Konovalov <andreyknvl@...il.com>
Cc: ryabinin.a.a@...il.com, dvyukov@...gle.com, skhan@...uxfoundation.org,
 kasan-dev@...glegroups.com, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2] kasan: add kunit tests for kmalloc_track_caller,
 kmalloc_node_track_caller


On 14/10/24 18:19, Andrey Konovalov wrote:
> On Mon, Oct 14, 2024 at 6:32 AM Nihar Chaithanya
> <niharchaithanya@...il.com> wrote:
>> The Kunit tests for kmalloc_track_caller and kmalloc_node_track_caller
>> were missing in kasan_test_c.c, which check that these functions poison
>> the memory properly.
>>
>> Add a Kunit test:
>> -> kmalloc_tracker_caller_oob_right(): This includes out-of-bounds
>>     access test for kmalloc_track_caller and kmalloc_node_track_caller.
>>
>> Signed-off-by: Nihar Chaithanya <niharchaithanya@...il.com>
>> Fixes: https://bugzilla.kernel.org/show_bug.cgi?id=216509
>> ---
>> v1->v2: Simplified the three separate out-of-bounds tests to a single test for
>> kmalloc_track_caller.
>>
>> Link to v1: https://lore.kernel.org/all/20241013172912.1047136-1-niharchaithanya@gmail.com/
>>
>>   mm/kasan/kasan_test_c.c | 32 ++++++++++++++++++++++++++++++++
>>   1 file changed, 32 insertions(+)
>>
>> diff --git a/mm/kasan/kasan_test_c.c b/mm/kasan/kasan_test_c.c
>> index a181e4780d9d..62efc1ee9612 100644
>> --- a/mm/kasan/kasan_test_c.c
>> +++ b/mm/kasan/kasan_test_c.c
>> @@ -213,6 +213,37 @@ static void kmalloc_node_oob_right(struct kunit *test)
>>          kfree(ptr);
>>   }
>>
>> +static void kmalloc_track_caller_oob_right(struct kunit *test)
>> +{
>> +       char *ptr;
>> +       size_t size = 128 - KASAN_GRANULE_SIZE;
>> +
>> +       /*
>> +        * Check that KASAN detects out-of-bounds access for object allocated via
>> +        * kmalloc_track_caller().
>> +        */
>> +       ptr = kmalloc_track_caller(size, GFP_KERNEL);
>> +       KUNIT_ASSERT_NOT_ERR_OR_NULL(test, ptr);
>> +
>> +       OPTIMIZER_HIDE_VAR(ptr);
>> +       KUNIT_EXPECT_KASAN_FAIL(test, ptr[size] = 'y');
>> +
>> +       kfree(ptr);
>> +
>> +       /*
>> +        * Check that KASAN detects out-of-bounds access for object allocated via
>> +        * kmalloc_node_track_caller().
>> +        */
>> +       size = 4096;
>> +       ptr = kmalloc_node_track_caller(size, GFP_KERNEL, 0);
>> +       KUNIT_ASSERT_NOT_ERR_OR_NULL(test, ptr);
>> +
>> +       OPTIMIZER_HIDE_VAR(ptr);
>> +       KUNIT_EXPECT_KASAN_FAIL(test, ptr[size] = 'y');
> What you had here before (ptr[0] = ptr[size]) was better. ptr[size] =
> 'y' with size == 4096 does an out-of-bounds write access, which
> corrupts uncontrolled memory for the tag-based KASAN modes, which do
> not use redzones. We try to avoid corrupting memory in KASAN tests, as
> the kernel might crash otherwise before all tests complete.
>
> So let's either change this back to ptr[0] = ptr[size] or just reuse
> the same size for both test cases (or does kmalloc_node_track_caller
> require size >= 4K?).

We can reuse the same test for both cases without changing the size, I ran
the test without changing the size (i.e., size == 128 - KASAN_GRANULE_SIZE),
the KASAN report was generated. I found instances (drm/tiny) where the size
passed to the kmalloc_node_track_caller is < 4k.

>> +
>> +       kfree(ptr);
>> +}
>> +
>>   /*
>>    * Check that KASAN detects an out-of-bounds access for a big object allocated
>>    * via kmalloc(). But not as big as to trigger the page_alloc fallback.
>> @@ -1958,6 +1989,7 @@ static struct kunit_case kasan_kunit_test_cases[] = {
>>          KUNIT_CASE(kmalloc_oob_right),
>>          KUNIT_CASE(kmalloc_oob_left),
>>          KUNIT_CASE(kmalloc_node_oob_right),
>> +       KUNIT_CASE(kmalloc_track_caller_oob_right),
>>          KUNIT_CASE(kmalloc_big_oob_right),
>>          KUNIT_CASE(kmalloc_large_oob_right),
>>          KUNIT_CASE(kmalloc_large_uaf),
>> --
>> 2.34.1
>>