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

[Dmitry Vyukov]

On Sun, May 29, 2016 at 4:45 PM, Luruo, Kuthonuzo
<kuthonuzo.luruo@xxxxxxx> 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