This is the mail archive of the
libc-alpha@sourceware.org
mailing list for the glibc project.
Re: [PATCH 2/2][BZ #12416] Use stack boundaries from /proc/PID/mapsto make stack executable
On Mon, 7 May 2012 13:02:21 -0700 (PDT), Roland wrote:
> What "kernel stack accounting" are you talking about? The [...]
> monikers for /proc/PID/maps output are just meant to be vaguely
> informative in a minimal-best-effort sort of way and should not be
> construed as signs of anything deeper AFAIK. If you think there is
> some concrete sense in which the kernel purports to grok what is and
> isn't part of your stack and let that affect its behavior in any
> particular way, be explicit about what you mean.
>
I was talking about mm->vm_stack that accounts for all vmas allocated
with MAP_GROWS*. But I'm wrong anyway, since that accounting is not
affected by vma splits, since the vmas still remain MAP_GROWS*.
So I propose the following simpler patch instead in the interest of
marking as less of the stack as possible as executable. The patch
modifies pthread_getattr_np to use the page end containing
__libc_stack_end as the end of stack and hence return stack_size and
stack_end + stack_size as the size and address of stack respectively.
One other thing I realized when I wrote the test case is that
__pthread_attr_getstackaddr simply subtracts stacksize from stackaddr
and returns it, which is wrong for architectures with _STACK_GROWS_UP.
In fact, pthread_getattr_np also probably has a similar problem. I am
currently surrounded by x86 boxes, so I'll see if I find myself an ia64
box to confirm this. I'll post patches for those problems separately if
they're applicable.
Regards,
Siddhesh
ChangeLog:
2012-05-08 Siddhesh Poyarekar <siddhesh@redhat.com>
* elf/tst-execstack.c (do_test): Adjust test case to ensure that
pthread_getattr_np behaviour remains the same after marking
stack executable.
nptl/ChangeLog
2012-05-08 Siddhesh Poyarekar <siddhesh@redhat.com>
* nptl/pthread_getattr_np.c (pthread_getattr_np): Use
__libc_stack_end rounded to the end of containing page as the
real stack end.
diff --git a/elf/tst-execstack.c b/elf/tst-execstack.c
index 6632e53..c3479ee 100644
--- a/elf/tst-execstack.c
+++ b/elf/tst-execstack.c
@@ -46,6 +46,17 @@ waiter_thread (void *arg)
}
#endif
+static bool
+stack_grows_down (const bool arg)
+{
+ bool local;
+
+ if (__builtin_frame_address (0) > __builtin_frame_address (1))
+ return true;
+ else {
+ return false;
+ }
+}
static bool allow_execstack = true;
@@ -107,6 +118,32 @@ do_test (void)
print_maps ();
+#if USE_PTHREADS
+ void *old_stack_addr, *new_stack_addr;
+ size_t stack_size;
+ pthread_t me = pthread_self ();
+ pthread_attr_t attr;
+ int ret = 0;
+
+ ret = pthread_getattr_np (me, &attr);
+ if (ret)
+ {
+ printf ("pthread_getattr_np returned error: %s\n", strerror (ret));
+ return 1;
+ }
+
+ ret = pthread_attr_getstack (&attr, &old_stack_addr, &stack_size);
+ if (ret)
+ {
+ printf ("pthread_getattr_np returned error: %s\n", strerror (ret));
+ return 1;
+ }
+ if (stack_grows_down(stack_size))
+ old_stack_addr += stack_size;
+ else
+ old_stack_addr -= stack_size;
+#endif
+
/* Loading this module should force stacks to become executable. */
void *h = dlopen ("tst-execstack-mod.so", RTLD_LAZY);
if (h == NULL)
@@ -129,6 +166,39 @@ do_test (void)
print_maps ();
+#if USE_PTHREADS
+ ret = pthread_getattr_np (me, &attr);
+ if (ret)
+ {
+ printf ("pthread_getattr_np returned error: %s\n", strerror (ret));
+ return 1;
+ }
+
+ ret = pthread_attr_getstack (&attr, &new_stack_addr, &stack_size);
+ if (ret)
+ {
+ printf ("pthread_getattr_np returned error: %s\n", strerror (ret));
+ return 1;
+ }
+
+ if (stack_grows_down(stack_size))
+ new_stack_addr += stack_size;
+ else
+ new_stack_addr -= stack_size;
+
+ /* Make sure that the stack end address did not change. We do not check the
+ stack top and size separately because we evaluate the stack top as
+ stack_end + size and as a result of that, both may change when the vma
+ above the stack changes. */
+ if (old_stack_addr != new_stack_addr)
+ {
+ printf ("Stack end changed, old: %p, new: %p\n",
+ old_stack_addr, new_stack_addr);
+ return 1;
+ }
+ printf ("Stack address remains the same: %p\n", old_stack_addr);
+#endif
+
/* Test that growing the stack region gets new executable pages too. */
deeper ((void (*) (void)) f);
diff --git a/nptl/pthread_getattr_np.c b/nptl/pthread_getattr_np.c
index f1268dd..742bc4f 100644
--- a/nptl/pthread_getattr_np.c
+++ b/nptl/pthread_getattr_np.c
@@ -84,6 +84,20 @@ pthread_getattr_np (thread_id, attr)
ret = errno;
else
{
+ /* We consider the main process stack to have ended with
+ the page containing __libc_stack_end. There is stuff below
+ it in the stack too, like the program arguments, environment
+ variables and auxv info, but we ignore those pages when
+ returning size so that the output is consistent when the
+ stack is marked executable due to a loaded dso requiring
+ it. */
+ void *real_stack_end;
+
+ real_stack_end = (void *) ((uintptr_t) __libc_stack_end
+ & -(uintptr_t) GLRO(dl_pagesize));
+#if _STACK_GROWS_DOWN
+ real_stack_end += GLRO(dl_pagesize);
+#endif
/* We need no locking. */
__fsetlocking (fp, FSETLOCKING_BYCALLER);
@@ -109,7 +123,7 @@ pthread_getattr_np (thread_id, attr)
{
/* Found the entry. Now we have the info we need. */
iattr->stacksize = rl.rlim_cur;
- iattr->stackaddr = (void *) to;
+ iattr->stackaddr = real_stack_end;
/* The limit might be too high. */
if ((size_t) iattr->stacksize