Re: [PATCH v5 7/7] mm: kasan: Initial memory quarantine implementation

[Alexander Potapenko]

On Thu, Mar 10, 2016 at 9:14 PM, Andrew Morton
<akpm@xxxxxxxxxxxxxxxxxxxx> wrote:
> On Thu, 10 Mar 2016 14:50:56 +0100 Alexander Potapenko <glider@xxxxxxxxxx> wrote:
>
>> On Wed, Mar 9, 2016 at 9:21 PM, Andrew Morton <akpm@xxxxxxxxxxxxxxxxxxxx> wrote:
>> > On Wed,  9 Mar 2016 12:05:48 +0100 Alexander Potapenko <glider@xxxxxxxxxx> wrote:
>> >
>> >> Quarantine isolates freed objects in a separate queue. The objects are
>> >> returned to the allocator later, which helps to detect use-after-free
>> >> errors.
>> >
>> > I'd like to see some more details on precisely *how* the parking of
>> > objects in the qlists helps "detect use-after-free"?
>> When the object is freed, its state changes from KASAN_STATE_ALLOC to
>> KASAN_STATE_QUARANTINE. The object is poisoned and put into quarantine
>> instead of being returned to the allocator, therefore every subsequent
>> access to that object triggers a KASAN error, and the error handler is
>> able to say where the object has been allocated and deallocated.
>> When it's time for the object to leave quarantine, its state becomes
>> KASAN_STATE_FREE and it's returned to the allocator. From now on the
>> allocator may reuse it for another allocation.
>> Before that happens, it's still possible to detect a use-after free on
>> that object (it retains the allocation/deallocation stacks).
>> When the allocator reuses this object, the shadow is unpoisoned and
>> old allocation/deallocation stacks are wiped. Therefore a use of this
>> object, even an incorrect one, won't trigger ASan warning.
>> Without the quarantine, it's not guaranteed that the objects aren't
>> reused immediately, that's why the probability of catching a
>> use-after-free is lower than with quarantine in place.
>
> I see, thanks.  I'll slurp that into the changelog for posterity.

I've also added a paragraph about that to the patch description.

>> >> +}
>> >
>> > We could avoid th4ese ifdefs in the usual way: an empty version of
>> > quarantine_remove_cache() if CONFIG_SLAB=n.
>> Yes, agreed.
>> I am sorry, I don't fully understand the review process now, when
>> you've pulled the patches into mm-tree.
>> Shall I send the new patch series version, as before, or is anything
>> else needs to be done?
>> Do I need to rebase against mm- or linux-next? Thanks in advance.
>
> I like to queue a delta patch so I and others can see what changed and
> also to keep track of who fixed what and why.  It's a bit harsh on the
> reviewers to send them a slightly altered version of a 500 line patch
> which they've already read through.
I'm listing the differences between patch versions after the patch
description (between the triple dashes), hope that helps the
reviewers.

> Before sending the patch up to Linus I'll clump everything into a
> single patch and a lot of that history is somewhat lost.
>
> Sending a replacement patch is often more convenient for the originator
> so that's fine - I'll turn the replacement into a delta locally and
> will review then queue that delta.  Also a new revision of a patch has
> an altered changelog so I'll manually move that into the older original
> patch's changelog immediately.
>
> IOW: either a new patch or a delta is fine.
Ok, got it.
> Your patch is in linux-next now so a diff against -next will work OK.
>
> Probably the easiest thing for you to do is to just alter the patch you
> have in-place and send out the new one.  A "[v2" in the Subject: helps
> people keep track of things.
Ok, will do.



-- 
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