lkml.org 
[lkml]   [2016]   [May]   [29]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    From
    Date
    SubjectRe: [PATCH v3 1/2] mm, kasan: improve double-free detection
    On Sun, May 29, 2016 at 4:45 PM, Luruo, Kuthonuzo
    <kuthonuzo.luruo@hpe.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

    \
     
     \ /
      Last update: 2016-05-29 17:21    [W:2.478 / U:0.016 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site