linux-kernel - Re: [PATCH v3 1/2] mm, kasan: improve double-free detection

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: Windows password security audit tool. GUI, reports in PDF.

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

Date:	Sun, 29 May 2016 16:56:38 +0200
From:	Dmitry Vyukov <dvyukov@...gle.com>
To:	"Luruo, Kuthonuzo" <kuthonuzo.luruo@....com>
Cc:	Andrey Ryabinin <aryabinin@...tuozzo.com>,
	Alexander Potapenko <glider@...gle.com>,
	Christoph Lameter <cl@...ux.com>,
	Pekka Enberg <penberg@...nel.org>,
	David Rientjes <rientjes@...gle.com>,
	Joonsoo Kim <iamjoonsoo.kim@....com>,
	Andrew Morton <akpm@...ux-foundation.org>,
	kasan-dev <kasan-dev@...glegroups.com>,
	LKML <linux-kernel@...r.kernel.org>,
	"linux-mm@...ck.org" <linux-mm@...ck.org>,
	Yury Norov <ynorov@...iumnetworks.com>
Subject: Re: [PATCH v3 1/2] mm, kasan: improve double-free detection

On Sun, May 29, 2016 at 4:45 PM, Luruo, Kuthonuzo
<kuthonuzo.luruo@....com> wrote:
>> > +/* flags shadow for object header if it has been overwritten. */
>> > +void kasan_mark_bad_meta(struct kasan_alloc_meta *alloc_info,
>> > +               struct kasan_access_info *info)
>> > +{
>> > +       u8 *datap = (u8 *)&alloc_info->data;
>> > +
>> > +       if ((((u8 *)info->access_addr + info->access_size) > datap) &&
>> > +                       ((u8 *)info->first_bad_addr <= datap) &&
>> > +                       info->is_write)
>> > +               kasan_poison_shadow((void *)datap, KASAN_SHADOW_SCALE_SIZE,
>> > +                               KASAN_KMALLOC_BAD_META);
>>
>>
>> Is it only to prevent deadlocks in kasan_meta_lock?
>>
>> If so, it is still unrelable because an OOB write can happen in
>> non-instrumented code. Or, kasan_meta_lock can successfully lock
>> overwritten garbage before noticing KASAN_KMALLOC_BAD_META. Or, two
>> threads can assume lock ownership after noticing
>> KASAN_KMALLOC_BAD_META.
>>
>> After the first report we continue working in kind of best effort
>> mode: we can try to mitigate some things, but generally all bets are
>> off. Because of that there is no need to build something complex,
>> global (and still unrelable). I would just wait for at most, say, 10
>> seconds in kasan_meta_lock, if we can't get the lock -- print an error
>> and return. That's simple, local and won't deadlock under any
>> circumstances.
>> The error message will be helpful, because there are chances we will
>> report a double-free on free of the corrupted object.
>>  e
>> Tests can be arranged so that they write 0 (unlocked) into the meta
>> (if necessary).
>
> Dmitry,
>
> Thanks very much for review & comments. Yes, the locking scheme in v3
> is flawed in the presence of OOB writes on header, safety valve
> notwithstanding. The core issue is that when thread finds lock held, it is
> difficult to tell whether a legit lock holder exists or lock bit got flipped
> from OOB. Earlier, I did consider a lock timeout but felt it to be a bit ugly...
>
> However, I believe I've found a solution and was about to push out v4
> when your comments came in. It takes concept from v3 - exploiting
> shadow memory - to make lock much more reliable/resilient even in the
> presence of OOB writes. I'll push out v4 within the hour...


Locking shadow will probably work. Need to think more.


>> > +       switch (alloc_info->state) {
>> >                 case KASAN_STATE_QUARANTINE:
>> >                 case KASAN_STATE_FREE:
>> > -                       pr_err("Double free");
>> > -                       dump_stack();
>> > -                       break;
>> > +                       kasan_report((unsigned long)object, 0, false, caller);
>> > +                       kasan_meta_unlock(alloc_info);
>> > +                       return true;
>> >                 default:
>>
>> Please at least print some here (it is not meant to happen, right?).
>
> ok.
>
>> >  struct kasan_alloc_meta {
>> > +       union {
>> > +               u64 data;
>> > +               struct {
>> > +                       u32 lock : 1;           /* lock bit */
>>
>>
>> Add a comment that kasan_meta_lock expects this to be the first bit.
>
> Not required in v4...
>
> Thank you, once again.
>
> Kuthonuzo