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

[Luruo, Kuthonuzo]

> > +/* 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...

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