diff mbox

Fix stack memory protection on targets where the stack grows upward

Message ID AAC3E651-7095-4CCA-BABA-498347523392@bell.net
State New
Headers show

Commit Message

John David Anglin April 16, 2017, 5:20 p.m. UTC 
The tst-cputimer1 test fails on hppa.  Using strace to look at the system calls generated by the test,
I observed that there is a mprotect call that passes a non page-aligned addr argument and it fails
with the error EINVAL.

The attached change aligns the old and new guard addresses to page boundaries and fixes the
failing mprotect call.

A version of this patch, hppa/local-stack-grows-up.diff, has been installed in Debian for a long time.
However, the old and new guard values were reversed in the compare.  As a result, the mprotect call
was skipped.  This versions checks that the new_guard value is greater than the old_guard value.

Please install.

Dave
--
John David Anglin	dave.anglin@bell.net
2017-04-16  John David Anglin  <danglin@gcc.gnu.org>

	* nptl/allocatestack.c (allocate_stack): Align old and new guard
	addresses to page boundaries when the stack grows up.

Comments

Andreas Schwab April 16, 2017, 8:06 p.m. UTC | #1 
On Apr 16 2017, John David Anglin <dave.anglin@bell.net> wrote:

> +	  char *new_guard = (char *)(((uintptr_t) pd - guardsize) & ~pagesize_m1);
> +	  char *old_guard = (char *)(((uintptr_t) pd - pd->guardsize) & ~pagesize_m1);

The lines are too long.

Andreas.
Florian Weimer April 16, 2017, 9:27 p.m. UTC | #2 
* John David Anglin:

>  #elif _STACK_GROWS_UP
> -	  if (mprotect ((char *) pd - pd->guardsize,
> -			pd->guardsize - guardsize, prot) != 0)
> +	  char *new_guard = (char *)(((uintptr_t) pd - guardsize) & ~pagesize_m1);
> +	  char *old_guard = (char *)(((uintptr_t) pd - pd->guardsize) & ~pagesize_m1);
> +	  /* The guard size difference might be > 0, but once rounded
> +	     to the nearest page the size difference might be zero.  */
> +	  if (new_guard > old_guard
> +	      && mprotect (old_guard, new_guard - old_guard, prot) != 0)
>  	    goto mprot_error;
>  #endif
>  

This is essentially HPPA-specific code, right?  So I guess we can do
whatever it takes there.
John David Anglin April 16, 2017, 11:03 p.m. UTC | #3 
On 2017-04-16, at 5:27 PM, Florian Weimer wrote:

> This is essentially HPPA-specific code, right?  So I guess we can do
> whatever it takes there.

I believe the metag architecture also has a stack that grows up.

--
John David Anglin	dave.anglin@bell.net
Florian Weimer April 17, 2017, 6:57 a.m. UTC | #4 
* John David Anglin:

> On 2017-04-16, at 5:27 PM, Florian Weimer wrote:
>
>> This is essentially HPPA-specific code, right?  So I guess we can do
>> whatever it takes there.
>
> I believe the metag architecture also has a stack that grows up.

I'm sure there are others, but HPPA is the only one with a glibc port.
John David Anglin April 17, 2017, 11:58 a.m. UTC | #5 
On 2017-04-17, at 2:57 AM, Florian Weimer wrote:

> * John David Anglin:
> 
>> On 2017-04-16, at 5:27 PM, Florian Weimer wrote:
>> 
>>> This is essentially HPPA-specific code, right?  So I guess we can do
>>> whatever it takes there.
>> 
>> I believe the metag architecture also has a stack that grows up.
> 
> I'm sure there are others, but HPPA is the only one with a glibc port.

Correct.

--
John David Anglin	dave.anglin@bell.net
diff mbox

Patch

diff --git a/nptl/allocatestack.c b/nptl/allocatestack.c
index e5c5f79a82..b3e0e9959f 100644
--- a/nptl/allocatestack.c
+++ b/nptl/allocatestack.c
@@ -647,8 +647,12 @@  allocate_stack (const struct pthread_attr *attr, struct pthread **pdp,
 			prot) != 0)
 	    goto mprot_error;
 #elif _STACK_GROWS_UP
-	  if (mprotect ((char *) pd - pd->guardsize,
-			pd->guardsize - guardsize, prot) != 0)
+	  char *new_guard = (char *)(((uintptr_t) pd - guardsize) & ~pagesize_m1);
+	  char *old_guard = (char *)(((uintptr_t) pd - pd->guardsize) & ~pagesize_m1);
+	  /* The guard size difference might be > 0, but once rounded
+	     to the nearest page the size difference might be zero.  */
+	  if (new_guard > old_guard
+	      && mprotect (old_guard, new_guard - old_guard, prot) != 0)
 	    goto mprot_error;
 #endif