RE: [PATCH] kasan: improve double-free detection

["Luruo, Kuthonuzo" <kuthonuzo.luruo@hpe.com>]

> >> >> >> I missed that Alexander already landed patches that reduce header size
> >> >> >> to 16 bytes.
> >> >> >> It is not OK to increase them again. Please leave state as bitfield
> >> >> >> and update it with CAS (if we introduce helper functions for state
> >> >> >> manipulation, they will hide the CAS loop, which is nice).
> >> >> >>
> >> >> >
> >> >> > Available CAS primitives/compiler do not support CAS with bitfield. I
> >> propose
> >> >> > to change kasan_alloc_meta to:
> >> >> >
> >> >> > struct kasan_alloc_meta {
> >> >> >         struct kasan_track track;
> >> >> >         u16 size_delta;         /* object_size - alloc size */
> >> >> >         u8 state;                    /* enum kasan_state */
> >> >> >         u8 reserved1;
> >> >> >         u32 reserved2;
> >> >> > }
> >> >> >
> >> >> > This shrinks _used_ meta object by 1 byte wrt the original. (btw, patch v1
> >> does
> >> >> > not increase overall alloc meta object size). "Alloc size", where needed,
> is
> >> >> > easily calculated as a delta from cache->object_size.
> >> >>
> >> >>
> >> >> What is the maximum size that slab can allocate?
> >> >> I remember seeing slabs as large as 4MB some time ago (or did I
> >> >> confuse it with something else?). If there are such large objects,
> >> >> that 2 bytes won't be able to hold even delta.
> >> >> However, now on my desktop I don't see slabs larger than 16KB in
> >> >> /proc/slabinfo.
> >> >
> >> > max size for SLAB's slab is 32MB; default is 4MB. I must have gotten
> confused
> >> by
> >> > SLUB's 8KB limit. Anyway, new kasan_alloc_meta in patch V2:
> >> >
> >> > struct kasan_alloc_meta {
> >> >         struct kasan_track track;
> >> >         union {
> >> >                 u8 lock;
> >> >                 struct {
> >> >                         u32 dummy : 8;
> >> >                         u32 size_delta : 24;    /* object_size - alloc size */
> >> >                 };
> >> >         };
> >> >         u32 state : 2;                          /* enum kasan_alloc_state */
> >> >         u32 unused : 30;
> >> > };
> >> >
> >> > This uses 2 more bits than current, but given the constraints I think this is
> >> > close to optimal.
> >>
> >>
> >> We plan to use the unused part for another depot_stack_handle_t (u32)
> >> to memorize stack of the last call_rcu on the object (this will
> >> greatly simplify debugging of use-after-free for objects freed by
> >> rcu). So we need that unused part.
> >>
> >> I would would simply put all these fields into a single u32:
> >>
> >> struct kasan_alloc_meta {
> >>         struct kasan_track track;
> >>         u32 status;  // contains lock, state and size
> >>         u32 unused;  // reserved for call_rcu stack handle
> >> };
> >>
> >> And then separately a helper type to pack/unpack status:
> >>
> >> union kasan_alloc_status {
> >>         u32 raw;
> >>         struct {
> >>                    u32 lock : 1;
> >>                    u32 state : 2;
> >>                    u32 unused : 5;
> >>                    u32 size : 24;
> >>         };
> >> };
> >>
> >>
> >> Then, when we need to read/update the header we do something like:
> >>
> >> kasan_alloc_status status, new_status;
> >>
> >> for (;;) {
> >>     status.raw = READ_ONCE(header->status);
> >>     // read status, form new_status, for example:
> >>     if (status.lock)
> >>           continue;
> >>     new_status.raw = status.raw;
> >>     new_status.lock = 1;
> >>     if (cas(&header->status, status.raw, new_status.raw))
> >>              break;
> >> }
> >>
> >>
> >> This will probably make state manipulation functions few lines longer,
> >> but since there are like 3 such functions I don't afraid that. And we
> >> still can use bitfield magic to extract fields and leave whole 5 bits
> >> unused bits for future.

v2 has been implemented with your suggested bitfield magic + cas loop. Thanks.

> >
> > The difficulty is that the lock managed by CAS needs 1 byte, mininum; TAS bit
> > is even 'worse': address must be that of an unsigned long.
> 
> cmpxchg function can operate on bytes, words, double words and quad words:
> http://lxr.free-electrons.com/source/arch/x86/include/asm/cmpxchg.h#L146
> 
> 
> > Might it be possible for you to employ the 'kasan_free_meta' header for your
> > RCU stack handle instead since KASAN does not currently store state for RCU
> > slabs on free?
> 
> Free meta is overlapped with user object. The object is not freed yet
> when call_rcu is invoked, so free meta cannot be used yet (user still
> holds own data there). Free meta can only be used after kfree is
> invoked on the object.

Yeah; Iwrongly assumed that kasan_cache_create() special treatment for
SLAB_DESTROY_BY_RCU slabs to store free meta at end of object covers all
uses of RCU. Anyway, "u32 reserved" in alloc meta is now available again...

Kuthonuzo