[PATCH] Add Safe-Linking to fastbins and tcache

[Eyal Itkin]

Thanks for all of the feedback.

1. I handled lines longer than 80 characters, there shouldn't be any
of those now.
2. Took your suggestion and removed PAGE_SHIFT in favor of 12. It
would have been cleaner if there was a proper POSIX-defined static way
of inferring this size. Anyway, 12 is the most common value, and in
some cases it is 13 or 14 which only means we will lose 1-2 bits of
protection, which isn't that bad.
3. I implemented all of the coding convention suggestions.
4. I clarified the comment to specifically mention that the mechanism
protects the "next" pointers of the single-linked lists of the
Fast-Bin and TCache. Also updated the commit message accordingly.

The patch indeed looks cleaner now (attached), feel free to add more
comments/suggestions if needed.

Thanks again for your help with this patch.
Eyal.

On Fri, Mar 20, 2020 at 4:53 AM DJ Delorie <dj@redhat.com> wrote:
>
> Eyal Itkin <eyal.itkin@gmail.com> writes:
> > Safe-Linking is a security mechanism that protects single-linked
> > lists (such as the fastbin and tcache) from being tampered by attackers.
> > The mechanism makes use of randomness from ASLR (mmap_base), and when
> > combined with chunk alignment integrity checks, it protects the
> > pointers from being hijacked by an attacker.
>
> Based on the patch, it seems that protected pointers are only used in
> chunks, not in the heap-global structures (i.e. the fastbins themselves
> aren't protected, only the "next" pointer in each chunk).  If this is
> so, could you add such a note to the comment before PROTECT_PTR ?
>
> >   * PROTECT(P) := (L >> PAGE_SHIFT) XOR (P)
> >   * *L = PROTECT(P)
>
> I.e. any stored pointer's value is XOR'd with the pointer's address
> bits.
>
> > +#define PROTECT_PTR(pos, ptr, type) \
> > +  ((type)((((size_t)pos) >> PAGE_SHIFT) ^ ((size_t)ptr)))
> > +#define REVEAL_PTR(pos, ptr, type)  PROTECT_PTR (pos, ptr, type)
>
> Style: whitespace after casts
>
> Bug: PAGE_SHIFT is an obsolete macro these days.  Not sure what to use
> but I wouldn't be opposed to just putting "12" in there.
>
> Suggestion1: since the "type" can be determined using __typeof, there's
> no need to pass it explicitly.  I.e.
>
> #define PROTECT_PTR(pos, ptr) \
>   ((__typeof ptr)((((size_t)pos) >> PAGE_SHIFT) ^ ((size_t)ptr)))
>
> Suggestion2: REVEAL_PTR is always called with "&foo,foo" so it can be
> further reduced:
>
> #define REVEAL_PTR(ptr)  PROTECT_PTR (&ptr, ptr)
>
>
> The rest of the patch looks good to me, but I think the above changes
> will make the new code much more readable.
>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-Add-Safe-Linking-to-fastbins-and-tcache.patch
Type: application/octet-stream
Size: 9283 bytes
Desc: not available
URL: <http://sourceware.org/pipermail/libc-alpha/attachments/20200320/4efe545c/attachment-0001.obj>