[RFC] [PATCH] [Aarch64] : Stack guard support in glibc

classic Classic list List threaded Threaded
2 messages Options
Reply | Threaded
Open this post in threaded view
|

[RFC] [PATCH] [Aarch64] : Stack guard support in glibc

Venkataramanan Kumar-3
Hi Maintainers,

Attached is RFC patch that adds stack guard support in glibc for
Aarch64 for review.

The TCB is 16 bytes in Aarch64 and tp points to the dtvt. Before the
TCB, the pthread structure is placed. This patch places the stack
guard (SG) and pointer gaurd variable (PG) between the TCB and pthread
structures.

We can access thread pointer using "msr" instruction, the compiler
will generate the following assembly to access the stack guard placed
before the TCB .

msr tpidr_el0, x0
ldr x1, [x0-8]


                       tp
                        |
 pthread            v
 -----------------------------
|         |PG|SG| dtvt|  |
 ------------------------------
                       TCB


I did a quick check by building eglibc and moving the built runtime
linker ld-linux-aarch64.so,1 and libc "libc.so.2.17.90" to the V8
model running open embedded image.

And ran the following test case using "ld-linux-aarch64.so.1 --library
./libc.so test.out 1" where libc.so points to newly built one.

---test.c---
#include <string.h>
#include <stdio.h>

void test_stack_smashing(int corrupt)
{
    long stack_val,temp;

    char arr[5];
    char * ptr = arr;

    if (!corrupt)

    {
        strcpy( ptr,"abcd");
        printf("copied string is %s\n",ptr);
    }
    else
    {

        printf("overflowing the buffer and hitting the canary now\n");
            memset (ptr,0,12);
        printf("Overwritten the buffer\n" );

              asm("mrs %0, tpidr_el0\n" "ldr %1, [%0,-8]\n" : "=r"
(stack_val) : "r" (temp));
        printf(" Canary value is %x\n", stack_val);

    }

}

int main(char *argc, char *argv[])
{

    if (0 == strcmp(argv[1],"0"))
    {
        test_stack_smashing(0);
        printf("Passed Canary test\n");
    }
    else
    {
        test_stack_smashing(1);
        printf("Failed Canary test\n");
    }
    return 0;
}

And without patch I got:

(Snip)
overflowing the buffer and hitting the canary now
Overwritten the buffer
 Canary value is 0
Failed Canary test
(Snip)

Canary value is zero and this happens without my change because I
believe there is already space between TCB and pthread nodes due to
alignment enforcement.

With the path:

(Snip)
overflowing the buffer and hitting the canary now
Overwritten the buffer
 Canary value is 9900cf00
*** stack smashing detected ***: ./a.out terminated
Aborted
(Snip)

I also checked the canary value and keeps changing from run to run.

regards,
Venkat.

glibc.tls.stack.guard.aarch64.diff (2K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [RFC] [PATCH] [Aarch64] : Stack guard support in glibc

Carlos O'Donell-6
On 08/26/2013 12:15 AM, Venkataramanan Kumar wrote:
> I also checked the canary value and keeps changing from run to run.

Is it faster than accessing a global?

> glibc.tls.stack.guard.aarch64.diff

Please review:
http://sourceware.org/glibc/wiki/Contribution%20checklist

Please run the entire testsuite and report if there are no regressions.

You need a ChangeLog and you should also write up some text for a NEWS
entry that will go out with 2.19 talking about the new feature for ARM.
 

> Index: tls.h
> ===================================================================
> --- tls.h (revision 23742)
> +++ tls.h (working copy)
> @@ -68,10 +68,15 @@
>  # define TLS_TCB_SIZE sizeof (tcbhead_t)
>  
>  /* This is the size we need before TCB.  */
> -# define TLS_PRE_TCB_SIZE sizeof (struct pthread)
> +# define TLS_PRE_TCB_SIZE \
> +  (sizeof (struct pthread)                                              \
> +   + (PTHREAD_STRUCT_END_PADDING < 2 * sizeof (uintptr_t)               \
> +      ? ((2 * sizeof (uintptr_t) + __alignof__ (struct pthread) - 1)    \
> +         & ~(__alignof__ (struct pthread) - 1))                         \
> +      : 0))

Please use ALIGN_UP or ALIGN_DOWN from libc-internal.h.

I know you're copying this from IA64, but it should still use
the newer macros.
 

>  /* Alignment requirements for the TCB.  */
> -# define TLS_TCB_ALIGN __alignof__ (tcbhead_t)
> +# define TLS_TCB_ALIGN __alignof__ (struct pthread)
>  /* Install the dtv pointer.  The pointer passed is to the element with
>     index -1 which contain the length.  */
> @@ -98,12 +103,28 @@
>  
>  /* Return the thread descriptor for the current thread.  */
>  # define THREAD_SELF \
> - ((struct pthread *)__builtin_thread_pointer () - 1)
> + ((struct pthread *)((char *) __builtin_thread_pointer () - TLS_PRE_TCB_SIZE))
>  
>  /* Magic for libthread_db to know how to do THREAD_SELF.  */
>  # define DB_THREAD_SELF \
> -  CONST_THREAD_AREA (64, sizeof (struct pthread))
> +  CONST_THREAD_AREA (64, TLS_PRE_TCB_SIZE)

Did you check to see if this impacts gdb in any way?

e.g. Install new glibc into a system, then build and run the gdb testsuite
and see if anything fails?

> +/* Set the stack guard field in TCB head.  */
> +#define THREAD_SET_STACK_GUARD(value) \
> +  (((uintptr_t *) __builtin_thread_pointer ())[-1] = (value))
> +#define THREAD_COPY_STACK_GUARD(descr) \
> +  (((uintptr_t *) ((char *) (descr) + TLS_PRE_TCB_SIZE))[-1] \
> +   = ((uintptr_t *) __builtin_thread_pointer ())[-1])
> +
> +/* Set the pointer guard field in TCB head.  */
> +#define THREAD_GET_POINTER_GUARD() \
> +  (((uintptr_t *) __builtin_thread_pointer ())[-2])
> +#define THREAD_SET_POINTER_GUARD(value) \
> +  (((uintptr_t *) __builtin_thread_pointer ())[-2] = (value))
> +#define THREAD_COPY_POINTER_GUARD(descr) \
> +  (((uintptr_t *) ((char *) (descr) + TLS_PRE_TCB_SIZE))[-2] \
> +   = THREAD_GET_POINTER_GUARD ())
> +

Please see the Style and Conventions:
http://sourceware.org/glibc/wiki/Style_and_Conventions#Nested_C_Preprocessor_Directives

>  /* Access to data in the thread descriptor is easy.  */
>  # define THREAD_GETMEM(descr, member) \
>    descr->member

I've only done a very light review here, please repost v2
and we can do a more thorough review.

Cheers,
Carlos.