Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1410137 > unrolled thread

[PATCH] mm, kasan: introduce a special shadow value for allocator metadata

Back to article view | Back to linux.kernel

Contents

  [PATCH] mm, kasan: introduce a special shadow value for allocator metadata Alexander Potapenko <glider@google.com> - 2016-05-31 12:50 +0200
    Re: [PATCH] mm, kasan: introduce a special shadow value for allocator  metadata Andrey Ryabinin <aryabinin@virtuozzo.com> - 2016-05-31 14:00 +0200
      Re: [PATCH] mm, kasan: introduce a special shadow value for allocator metadata Alexander Potapenko <glider@google.com> - 2016-05-31 19:50 +0200
        Re: [PATCH] mm, kasan: introduce a special shadow value for allocator metadata Alexander Potapenko <glider@google.com> - 2016-06-01 18:40 +0200
          Re: [PATCH] mm, kasan: introduce a special shadow value for allocator metadata Alexander Potapenko <glider@google.com> - 2016-06-02 14:10 +0200
            Re: [PATCH] mm, kasan: introduce a special shadow value for allocator metadata Alexander Potapenko <glider@google.com> - 2016-06-02 14:20 +0200

#1410137 — [PATCH] mm, kasan: introduce a special shadow value for allocator metadata

Add a special shadow value to distinguish accesses to KASAN-specific
allocator metadata.

Unlike AddressSanitizer in the userspace, KASAN lets the kernel proceed
after a memory error. However a write to the kmalloc metadata may cause
memory corruptions that will make the tool itself unreliable and induce
crashes later on. Warning about such corruptions will ease the
debugging.

Signed-off-by: Alexander Potapenko <glider@google.com>
---
 mm/kasan/kasan.c  | 15 +++++++++++++++
 mm/kasan/kasan.h  |  1 +
 mm/kasan/report.c |  3 +++
 3 files changed, 19 insertions(+)

diff --git a/mm/kasan/kasan.c b/mm/kasan/kasan.c
index 18b6a2b..c590366 100644
--- a/mm/kasan/kasan.c
+++ b/mm/kasan/kasan.c
@@ -518,6 +518,19 @@ void kasan_poison_slab_free(struct kmem_cache *cache, void *object)
 		return;
 
 	kasan_poison_shadow(object, rounded_up_size, KASAN_KMALLOC_FREE);
+#ifdef CONFIG_SLAB
+	if (cache->flags & SLAB_KASAN) {
+		struct kasan_alloc_meta *alloc_info =
+			get_alloc_info(cache, object);
+		struct kasan_free_meta *free_info =
+			get_free_info(cache, object);
+		kasan_poison_shadow(alloc_info,
+			sizeof(struct kasan_alloc_meta), KASAN_KMALLOC_META);
+		kasan_poison_shadow(free_info,
+			sizeof(struct kasan_free_meta), KASAN_KMALLOC_META);
+	}
+#endif
+
 }
 
 bool kasan_slab_free(struct kmem_cache *cache, void *object)
@@ -584,6 +597,8 @@ void kasan_kmalloc(struct kmem_cache *cache, const void *object, size_t size,
 		alloc_info->state = KASAN_STATE_ALLOC;
 		alloc_info->alloc_size = size;
 		set_track(&alloc_info->track, flags);
+		kasan_poison_shadow(alloc_info,
+			sizeof(struct kasan_alloc_meta), KASAN_KMALLOC_META);
 	}
 #endif
 }
diff --git a/mm/kasan/kasan.h b/mm/kasan/kasan.h
index fb87923..1a0d82d 100644
--- a/mm/kasan/kasan.h
+++ b/mm/kasan/kasan.h
@@ -12,6 +12,7 @@
 #define KASAN_KMALLOC_REDZONE   0xFC  /* redzone inside slub object */
 #define KASAN_KMALLOC_FREE      0xFB  /* object was freed (kmem_cache_free/kfree) */
 #define KASAN_GLOBAL_REDZONE    0xFA  /* redzone for global variable */
+#define KASAN_KMALLOC_META      0xF9  /* redzone for kmalloc metadata */
 
 /*
  * Stack redzone shadow values
diff --git a/mm/kasan/report.c b/mm/kasan/report.c
index b3c122d..b6d3753 100644
--- a/mm/kasan/report.c
+++ b/mm/kasan/report.c
@@ -90,6 +90,9 @@ static void print_error_description(struct kasan_access_info *info)
 	case KASAN_KMALLOC_FREE:
 		bug_type = "use-after-free";
 		break;
+	case KASAN_KMALLOC_META:
+		bug_type = "touching kmalloc metadata";
+		break;
 	}
 
 	pr_err("BUG: KASAN: %s in %pS at addr %p\n",
-- 
2.8.0.rc3.226.g39d4020

[toc] | [next] | [standalone]

#1410218 — Re: [PATCH] mm, kasan: introduce a special shadow value for allocator metadata

On 05/31/2016 01:44 PM, Alexander Potapenko wrote:
> Add a special shadow value to distinguish accesses to KASAN-specific
> allocator metadata.
> 
> Unlike AddressSanitizer in the userspace, KASAN lets the kernel proceed
> after a memory error. However a write to the kmalloc metadata may cause
> memory corruptions that will make the tool itself unreliable and induce
> crashes later on. Warning about such corruptions will ease the
> debugging.

It will not. Whether out-of-bounds hits metadata or not is absolutely irrelevant
to the bug itself. This information doesn't help to understand, analyze or fix the bug.

[toc] | [prev] | [next] | [standalone]

#1410492

On Tue, May 31, 2016 at 1:52 PM, Andrey Ryabinin
<aryabinin@virtuozzo.com> wrote:
>
>
> On 05/31/2016 01:44 PM, Alexander Potapenko wrote:
>> Add a special shadow value to distinguish accesses to KASAN-specific
>> allocator metadata.
>>
>> Unlike AddressSanitizer in the userspace, KASAN lets the kernel proceed
>> after a memory error. However a write to the kmalloc metadata may cause
>> memory corruptions that will make the tool itself unreliable and induce
>> crashes later on. Warning about such corruptions will ease the
>> debugging.
>
> It will not. Whether out-of-bounds hits metadata or not is absolutely irrelevant
> to the bug itself. This information doesn't help to understand, analyze or fix the bug.
>
Here's the example that made me think the opposite.

I've been reworking KASAN hooks for mempool and added a test that did
a write-after-free to an object allocated from a mempool.
This resulted in flaky kernel crashes somewhere in quarantine
shrinking after several attempts to `insmod test_kasan.ko`.
Because there already were numerous KASAN errors in the test, it
wasn't evident that the crashes were related to the new test, so I
thought the problem was in the buggy quarantine implementation.
However the problem was indeed in the new test, which corrupted the
quarantine pointer in the object and caused a crash while traversing
the quarantine list.

My previous experience with userspace ASan shows that crashes in the
tool code itself puzzle the developers.
As a result, the users think that the tool is broken and don't believe
its reports.

I first thought about hardening the quarantine list by checksumming
the pointers and validating them on each traversal.
This prevents the crashes, but doesn't give the users any idea about
what went wrong.
On the other hand, reporting the pointer corruption right when it happens does.
Distinguishing between a regular UAF and a quarantine corruption
(which is what the patch in question is about) helps to prioritize the
KASAN reports and give the developers better understanding of the
consequences.



-- 
Alexander Potapenko
Software Engineer

Google Germany GmbH
Erika-Mann-Straße, 33
80636 München

Geschäftsführer: Matthew Scott Sucherman, Paul Terence Manicle
Registergericht und -nummer: Hamburg, HRB 86891
Sitz der Gesellschaft: Hamburg

[toc] | [prev] | [next] | [standalone]

#1411393

On Wed, Jun 1, 2016 at 5:23 PM, Andrey Ryabinin <aryabinin@virtuozzo.com> wrote:
> On 05/31/2016 08:49 PM, Alexander Potapenko wrote:
>> On Tue, May 31, 2016 at 1:52 PM, Andrey Ryabinin
>> <aryabinin@virtuozzo.com> wrote:
>>>
>>>
>>> On 05/31/2016 01:44 PM, Alexander Potapenko wrote:
>>>> Add a special shadow value to distinguish accesses to KASAN-specific
>>>> allocator metadata.
>>>>
>>>> Unlike AddressSanitizer in the userspace, KASAN lets the kernel proceed
>>>> after a memory error. However a write to the kmalloc metadata may cause
>>>> memory corruptions that will make the tool itself unreliable and induce
>>>> crashes later on. Warning about such corruptions will ease the
>>>> debugging.
>>>
>>> It will not. Whether out-of-bounds hits metadata or not is absolutely irrelevant
>>> to the bug itself. This information doesn't help to understand, analyze or fix the bug.
>>>
>> Here's the example that made me think the opposite.
>>
>> I've been reworking KASAN hooks for mempool and added a test that did
>> a write-after-free to an object allocated from a mempool.
>> This resulted in flaky kernel crashes somewhere in quarantine
>> shrinking after several attempts to `insmod test_kasan.ko`.
>> Because there already were numerous KASAN errors in the test, it
>> wasn't evident that the crashes were related to the new test, so I
>> thought the problem was in the buggy quarantine implementation.
>> However the problem was indeed in the new test, which corrupted the
>> quarantine pointer in the object and caused a crash while traversing
>> the quarantine list.
>>
>> My previous experience with userspace ASan shows that crashes in the
>> tool code itself puzzle the developers.
>> As a result, the users think that the tool is broken and don't believe
>> its reports.
>>
>> I first thought about hardening the quarantine list by checksumming
>> the pointers and validating them on each traversal.
>> This prevents the crashes, but doesn't give the users any idea about
>> what went wrong.
>> On the other hand, reporting the pointer corruption right when it happens does.
>> Distinguishing between a regular UAF and a quarantine corruption
>> (which is what the patch in question is about) helps to prioritize the
>> KASAN reports and give the developers better understanding of the
>> consequences.
>>
>
> After the first report we have memory in a corrupted state, so we are done here.
This is theoretically true, that's why we crash after the first report
in the userspace ASan.
But since the kernel proceeds after the first KASAN report, it's
possible that we see several different reports, and they are sometimes
worth looking at.

> Anything that happens after the first report can't be trusted since it can be an after-effect,
> just like in your case. Such crashes are not worthy to look at.
> Out-of-bounds that doesn't hit metadata as any other memory corruption also can lead to after-effects crashes,
> thus distinguishing such bugs doesn't make a lot of sense.
Unlike the crashes in the kernel itself, crashes with KASAN functions
in the stack trace may make the developer think the tool is broken.
>
> test_kasan module is just a quick hack, made only to make sure that KASAN works.
> It does some crappy thing, and may lead to crash as well. So I would recommend an immediate
> reboot even after single attempt to load it.
Agreed. However a plain write into the first byte of the freed object
will cause similar problems.


-- 
Alexander Potapenko
Software Engineer

Google Germany GmbH
Erika-Mann-Straße, 33
80636 München

Geschäftsführer: Matthew Scott Sucherman, Paul Terence Manicle
Registergericht und -nummer: Hamburg, HRB 86891
Sitz der Gesellschaft: Hamburg

[toc] | [prev] | [next] | [standalone]

#1412169

On Wed, Jun 1, 2016 at 6:31 PM, Alexander Potapenko <glider@google.com> wrote:
> On Wed, Jun 1, 2016 at 5:23 PM, Andrey Ryabinin <aryabinin@virtuozzo.com> wrote:
>> On 05/31/2016 08:49 PM, Alexander Potapenko wrote:
>>> On Tue, May 31, 2016 at 1:52 PM, Andrey Ryabinin
>>> <aryabinin@virtuozzo.com> wrote:
>>>>
>>>>
>>>> On 05/31/2016 01:44 PM, Alexander Potapenko wrote:
>>>>> Add a special shadow value to distinguish accesses to KASAN-specific
>>>>> allocator metadata.
>>>>>
>>>>> Unlike AddressSanitizer in the userspace, KASAN lets the kernel proceed
>>>>> after a memory error. However a write to the kmalloc metadata may cause
>>>>> memory corruptions that will make the tool itself unreliable and induce
>>>>> crashes later on. Warning about such corruptions will ease the
>>>>> debugging.
>>>>
>>>> It will not. Whether out-of-bounds hits metadata or not is absolutely irrelevant
>>>> to the bug itself. This information doesn't help to understand, analyze or fix the bug.
>>>>
>>> Here's the example that made me think the opposite.
>>>
>>> I've been reworking KASAN hooks for mempool and added a test that did
>>> a write-after-free to an object allocated from a mempool.
>>> This resulted in flaky kernel crashes somewhere in quarantine
>>> shrinking after several attempts to `insmod test_kasan.ko`.
>>> Because there already were numerous KASAN errors in the test, it
>>> wasn't evident that the crashes were related to the new test, so I
>>> thought the problem was in the buggy quarantine implementation.
>>> However the problem was indeed in the new test, which corrupted the
>>> quarantine pointer in the object and caused a crash while traversing
>>> the quarantine list.
>>>
>>> My previous experience with userspace ASan shows that crashes in the
>>> tool code itself puzzle the developers.
>>> As a result, the users think that the tool is broken and don't believe
>>> its reports.
>>>
>>> I first thought about hardening the quarantine list by checksumming
>>> the pointers and validating them on each traversal.
>>> This prevents the crashes, but doesn't give the users any idea about
>>> what went wrong.
>>> On the other hand, reporting the pointer corruption right when it happens does.
>>> Distinguishing between a regular UAF and a quarantine corruption
>>> (which is what the patch in question is about) helps to prioritize the
>>> KASAN reports and give the developers better understanding of the
>>> consequences.
>>>
>>
>> After the first report we have memory in a corrupted state, so we are done here.
> This is theoretically true, that's why we crash after the first report
> in the userspace ASan.
> But since the kernel proceeds after the first KASAN report, it's
> possible that we see several different reports, and they are sometimes
> worth looking at.
>
>> Anything that happens after the first report can't be trusted since it can be an after-effect,
>> just like in your case. Such crashes are not worthy to look at.
>> Out-of-bounds that doesn't hit metadata as any other memory corruption also can lead to after-effects crashes,
>> thus distinguishing such bugs doesn't make a lot of sense.
> Unlike the crashes in the kernel itself, crashes with KASAN functions
> in the stack trace may make the developer think the tool is broken.
>>
>> test_kasan module is just a quick hack, made only to make sure that KASAN works.
>> It does some crappy thing, and may lead to crash as well. So I would recommend an immediate
>> reboot even after single attempt to load it.
> Agreed. However a plain write into the first byte of the freed object
> will cause similar problems.

On a second thought, we could do without the additional shadow byte
value, by just comparing the address to the metadata offset.

>
> --
> Alexander Potapenko
> Software Engineer
>
> Google Germany GmbH
> Erika-Mann-Straße, 33
> 80636 München
>
> Geschäftsführer: Matthew Scott Sucherman, Paul Terence Manicle
> Registergericht und -nummer: Hamburg, HRB 86891
> Sitz der Gesellschaft: Hamburg



-- 
Alexander Potapenko
Software Engineer

Google Germany GmbH
Erika-Mann-Straße, 33
80636 München

Geschäftsführer: Matthew Scott Sucherman, Paul Terence Manicle
Registergericht und -nummer: Hamburg, HRB 86891
Sitz der Gesellschaft: Hamburg

[toc] | [prev] | [next] | [standalone]

#1412177

On Thu, Jun 2, 2016 at 2:17 PM, Andrey Ryabinin <aryabinin@virtuozzo.com> wrote:
>
>
> On 06/02/2016 03:02 PM, Alexander Potapenko wrote:
>> On Wed, Jun 1, 2016 at 6:31 PM, Alexander Potapenko <glider@google.com> wrote:
>>> On Wed, Jun 1, 2016 at 5:23 PM, Andrey Ryabinin <aryabinin@virtuozzo.com> wrote:
>>>> On 05/31/2016 08:49 PM, Alexander Potapenko wrote:
>>>>> On Tue, May 31, 2016 at 1:52 PM, Andrey Ryabinin
>>>>> <aryabinin@virtuozzo.com> wrote:
>>>>>>
>>>>>>
>>>>>> On 05/31/2016 01:44 PM, Alexander Potapenko wrote:
>>>>>>> Add a special shadow value to distinguish accesses to KASAN-specific
>>>>>>> allocator metadata.
>>>>>>>
>>>>>>> Unlike AddressSanitizer in the userspace, KASAN lets the kernel proceed
>>>>>>> after a memory error. However a write to the kmalloc metadata may cause
>>>>>>> memory corruptions that will make the tool itself unreliable and induce
>>>>>>> crashes later on. Warning about such corruptions will ease the
>>>>>>> debugging.
>>>>>>
>>>>>> It will not. Whether out-of-bounds hits metadata or not is absolutely irrelevant
>>>>>> to the bug itself. This information doesn't help to understand, analyze or fix the bug.
>>>>>>
>>>>> Here's the example that made me think the opposite.
>>>>>
>>>>> I've been reworking KASAN hooks for mempool and added a test that did
>>>>> a write-after-free to an object allocated from a mempool.
>>>>> This resulted in flaky kernel crashes somewhere in quarantine
>>>>> shrinking after several attempts to `insmod test_kasan.ko`.
>>>>> Because there already were numerous KASAN errors in the test, it
>>>>> wasn't evident that the crashes were related to the new test, so I
>>>>> thought the problem was in the buggy quarantine implementation.
>>>>> However the problem was indeed in the new test, which corrupted the
>>>>> quarantine pointer in the object and caused a crash while traversing
>>>>> the quarantine list.
>>>>>
>>>>> My previous experience with userspace ASan shows that crashes in the
>>>>> tool code itself puzzle the developers.
>>>>> As a result, the users think that the tool is broken and don't believe
>>>>> its reports.
>>>>>
>>>>> I first thought about hardening the quarantine list by checksumming
>>>>> the pointers and validating them on each traversal.
>>>>> This prevents the crashes, but doesn't give the users any idea about
>>>>> what went wrong.
>>>>> On the other hand, reporting the pointer corruption right when it happens does.
>>>>> Distinguishing between a regular UAF and a quarantine corruption
>>>>> (which is what the patch in question is about) helps to prioritize the
>>>>> KASAN reports and give the developers better understanding of the
>>>>> consequences.
>>>>>
>>>>
>>>> After the first report we have memory in a corrupted state, so we are done here.
>>> This is theoretically true, that's why we crash after the first report
>>> in the userspace ASan.
>>> But since the kernel proceeds after the first KASAN report, it's
>>> possible that we see several different reports, and they are sometimes
>>> worth looking at.
>>>
>>>> Anything that happens after the first report can't be trusted since it can be an after-effect,
>>>> just like in your case. Such crashes are not worthy to look at.
>>>> Out-of-bounds that doesn't hit metadata as any other memory corruption also can lead to after-effects crashes,
>>>> thus distinguishing such bugs doesn't make a lot of sense.
>>> Unlike the crashes in the kernel itself, crashes with KASAN functions
>>> in the stack trace may make the developer think the tool is broken.
>>>>
>>>> test_kasan module is just a quick hack, made only to make sure that KASAN works.
>>>> It does some crappy thing, and may lead to crash as well. So I would recommend an immediate
>>>> reboot even after single attempt to load it.
>>> Agreed. However a plain write into the first byte of the freed object
>>> will cause similar problems.
>>
>> On a second thought, we could do without the additional shadow byte
>> value, by just comparing the address to the metadata offset.
>>
>
> We could. But still, there is no point in doing anything like that.
Ok, got it.


-- 
Alexander Potapenko
Software Engineer

Google Germany GmbH
Erika-Mann-Straße, 33
80636 München

Geschäftsführer: Matthew Scott Sucherman, Paul Terence Manicle
Registergericht und -nummer: Hamburg, HRB 86891
Sitz der Gesellschaft: Hamburg

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web