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

From: Alexander Potapenko <glider@google.com>
To: Andrey Ryabinin <aryabinin@virtuozzo.com>
Cc: Andrey Konovalov <adech.fo@gmail.com>,
	Christoph Lameter <cl@linux.com>,
	Dmitriy Vyukov <dvyukov@google.com>,
	Andrew Morton <akpm@linux-foundation.org>,
	Steven Rostedt <rostedt@goodmis.org>,
	Joonsoo Kim <iamjoonsoo.kim@lge.com>,
	Joonsoo Kim <js1304@gmail.com>,
	Kostya Serebryany <kcc@google.com>,
	kasan-dev <kasan-dev@googlegroups.com>,
	Linux Memory Management List <linux-mm@kvack.org>,
	LKML <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH] mm, kasan: introduce a special shadow value for allocator metadata
Date: Tue, 31 May 2016 19:49:45 +0200	[thread overview]
Message-ID: <CAG_fn=UuSs=aUst5Ww3RGF-SvOprUYPs2Y3-dD=w1b80-D_R-A@mail.gmail.com> (raw)
In-Reply-To: <574D7B11.8090709@virtuozzo.com>

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