public inbox for libc-ports@sourceware.org
 help / color / mirror / Atom feed
* [RFC] [PATCH] [Aarch64] : Stack guard support in glibc
@ 2013-08-26  4:15 Venkataramanan Kumar
  2013-08-28  2:13 ` Carlos O'Donell
  0 siblings, 1 reply; 2+ messages in thread
From: Venkataramanan Kumar @ 2013-08-26  4:15 UTC (permalink / raw)
  To: libc-ports, Marcus Shawcroft, Marcus Shawcroft, Patch Tracking

[-- Attachment #1: Type: text/plain, Size: 2447 bytes --]

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.

[-- Attachment #2: glibc.tls.stack.guard.aarch64.diff --]
[-- Type: application/octet-stream, Size: 2151 bytes --]

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))
 
 /* 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)
 
+/* 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 ())
+
 /* Access to data in the thread descriptor is easy.  */
 # define THREAD_GETMEM(descr, member) \
   descr->member

^ permalink raw reply	[flat|nested] 2+ messages in thread

* Re: [RFC] [PATCH] [Aarch64] : Stack guard support in glibc
  2013-08-26  4:15 [RFC] [PATCH] [Aarch64] : Stack guard support in glibc Venkataramanan Kumar
@ 2013-08-28  2:13 ` Carlos O'Donell
  0 siblings, 0 replies; 2+ messages in thread
From: Carlos O'Donell @ 2013-08-28  2:13 UTC (permalink / raw)
  To: Venkataramanan Kumar
  Cc: libc-ports, Marcus Shawcroft, Marcus Shawcroft, Patch Tracking

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.

^ permalink raw reply	[flat|nested] 2+ messages in thread

end of thread, other threads:[~2013-08-28  2:13 UTC | newest]

Thread overview: 2+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2013-08-26  4:15 [RFC] [PATCH] [Aarch64] : Stack guard support in glibc Venkataramanan Kumar
2013-08-28  2:13 ` Carlos O'Donell

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).