[LTP] [PATCH] mprotect04: Support execute-only page access permissions

[LTP] [PATCH] mprotect04: Support execute-only page access permissions

[Daniel Mentz]

Linux version 4.9 introduced support for execute-only page access permissions on
arm64. As a result, user space processes, by default, cannot read from
their own .text sections. This change adds an extra call to mprotect()
to explicitly change access protections to allow relevant parts of the
.text section to be read.

Without this change, mprotect04 generates false TBROK results. We
previously saw this test output:

mprotect04    1  TPASS  :  test PROT_NONE for mprotect success
mprotect04    0  TINFO  :  exec_func: 0x5ac82d3588, page_to_copy: 0x5ac82d3000
mprotect04    2  TBROK  :  ltp/testcases/kernel/syscalls/mprotect/mprotect04.c:236: page_to_copy not present
mprotect04    3  TBROK  :  ltp/testcases/kernel/syscalls/mprotect/mprotect04.c:236: Remaining cases broken

Signed-off-by: Daniel Mentz <danielmentz@google.com>
---
 .../kernel/syscalls/mprotect/mprotect04.c     | 31 ++++++++++++++++---
 1 file changed, 27 insertions(+), 4 deletions(-)

diff --git a/testcases/kernel/syscalls/mprotect/mprotect04.c b/testcases/kernel/syscalls/mprotect/mprotect04.c
index 60941a422..811449f6a 100644
--- a/testcases/kernel/syscalls/mprotect/mprotect04.c
+++ b/testcases/kernel/syscalls/mprotect/mprotect04.c
@@ -217,6 +217,7 @@ static void *get_func(void *mem)
 	uintptr_t func_page_offset;
 	void *func_copy_start, *page_to_copy;
 	void *mem_start = mem;
+	int exec_only_platform = 0;
 
 #ifdef USE_FUNCTION_DESCRIPTORS
 	func_descr_t *opd =  (func_descr_t *)&exec_func;
@@ -229,17 +230,35 @@ static void *get_func(void *mem)
 	page_to_copy = (void *)((uintptr_t)&exec_func & page_mask);
 #endif
 
-	/* copy 1st page, if it's not present something is wrong */
+	/* Copy 1st page. If it's not accessible, we might be running on a
+	 * platform that supports execute-only page access permissions, in which
+	 * case we have to explicitly change access protections to allow the
+	 * memory to be read. */
 	if (!page_present(page_to_copy)) {
-		tst_resm(TINFO, "exec_func: %p, page_to_copy: %p\n",
-			&exec_func, page_to_copy);
-		tst_brkm(TBROK, cleanup, "page_to_copy not present\n");
+		TEST(mprotect(page_to_copy, page_sz, PROT_READ | PROT_EXEC));
+		if (TEST_RETURN == -1) {
+			tst_resm(TFAIL | TTERRNO,
+				 "mprotect(PROT_READ|PROT_EXEC) failed");
+			return NULL;
+		}
+		/* If the memory is still not accessible, then something must be
+		 * wrong. */
+		if (!page_present(page_to_copy)) {
+			tst_resm(TINFO, "exec_func: %p, page_to_copy: %p\n",
+				&exec_func, page_to_copy);
+			tst_brkm(TBROK, cleanup, "page_to_copy not present\n");
+		}
+		exec_only_platform = 1;
 	}
 	memcpy(mem, page_to_copy, page_sz);
 
 	/* copy 2nd page if possible */
 	mem += page_sz;
 	page_to_copy += page_sz;
+	/* Mark page readable on platforms that support execute-only page access
+	 * permissions. */
+	if (exec_only_platform)
+		mprotect(page_to_copy, page_sz, PROT_READ | PROT_EXEC);
 	if (page_present(page_to_copy))
 		memcpy(mem, page_to_copy, page_sz);
 	else
@@ -271,6 +290,9 @@ static void testfunc_protexec(void)
 	func = get_func(p);
 #endif
 
+	if (!func)
+		goto out;
+
 	/* Change the protection to PROT_EXEC. */
 	TEST(mprotect(p, copy_sz, PROT_EXEC));
 
@@ -294,6 +316,7 @@ static void testfunc_protexec(void)
 		}
 	}
 
+out:
 	SAFE_MUNMAP(cleanup, p, copy_sz);
 }
 
-- 
2.20.1.611.gfbb209baf1-goog

[LTP] [PATCH] mprotect04: Support execute-only page access permissions

[Jan Stancek]

----- Original Message -----
> Linux version 4.9 introduced support for execute-only page access permissions
> on
> arm64. As a result, user space processes, by default, cannot read from
> their own .text sections. This change adds an extra call to mprotect()
> to explicitly change access protections to allow relevant parts of the
> .text section to be read.
> 
> Without this change, mprotect04 generates false TBROK results. We
> previously saw this test output:
> 
> mprotect04    1  TPASS  :  test PROT_NONE for mprotect success
> mprotect04    0  TINFO  :  exec_func: 0x5ac82d3588, page_to_copy:
> 0x5ac82d3000
> mprotect04    2  TBROK  :
> ltp/testcases/kernel/syscalls/mprotect/mprotect04.c:236: page_to_copy not
> present
> mprotect04    3  TBROK  :
> ltp/testcases/kernel/syscalls/mprotect/mprotect04.c:236: Remaining cases
> broken
> 
> Signed-off-by: Daniel Mentz <danielmentz@google.com>
> ---
>  .../kernel/syscalls/mprotect/mprotect04.c     | 31 ++++++++++++++++---
>  1 file changed, 27 insertions(+), 4 deletions(-)
> 
> diff --git a/testcases/kernel/syscalls/mprotect/mprotect04.c
> b/testcases/kernel/syscalls/mprotect/mprotect04.c
> index 60941a422..811449f6a 100644
> --- a/testcases/kernel/syscalls/mprotect/mprotect04.c
> +++ b/testcases/kernel/syscalls/mprotect/mprotect04.c
> @@ -217,6 +217,7 @@ static void *get_func(void *mem)
>  	uintptr_t func_page_offset;
>  	void *func_copy_start, *page_to_copy;
>  	void *mem_start = mem;
> +	int exec_only_platform = 0;
>  
>  #ifdef USE_FUNCTION_DESCRIPTORS
>  	func_descr_t *opd =  (func_descr_t *)&exec_func;
> @@ -229,17 +230,35 @@ static void *get_func(void *mem)
>  	page_to_copy = (void *)((uintptr_t)&exec_func & page_mask);
>  #endif
>  
> -	/* copy 1st page, if it's not present something is wrong */
> +	/* Copy 1st page. If it's not accessible, we might be running on a
> +	 * platform that supports execute-only page access permissions, in which
> +	 * case we have to explicitly change access protections to allow the
> +	 * memory to be read. */
>  	if (!page_present(page_to_copy)) {
> -		tst_resm(TINFO, "exec_func: %p, page_to_copy: %p\n",
> -			&exec_func, page_to_copy);
> -		tst_brkm(TBROK, cleanup, "page_to_copy not present\n");
> +		TEST(mprotect(page_to_copy, page_sz, PROT_READ | PROT_EXEC));
> +		if (TEST_RETURN == -1) {
> +			tst_resm(TFAIL | TTERRNO,
> +				 "mprotect(PROT_READ|PROT_EXEC) failed");
> +			return NULL;
> +		}
> +		/* If the memory is still not accessible, then something must be
> +		 * wrong. */
> +		if (!page_present(page_to_copy)) {
> +			tst_resm(TINFO, "exec_func: %p, page_to_copy: %p\n",
> +				&exec_func, page_to_copy);
> +			tst_brkm(TBROK, cleanup, "page_to_copy not present\n");
> +		}
> +		exec_only_platform = 1;
>  	}
>  	memcpy(mem, page_to_copy, page_sz);
>  
>  	/* copy 2nd page if possible */
>  	mem += page_sz;
>  	page_to_copy += page_sz;
> +	/* Mark page readable on platforms that support execute-only page access
> +	 * permissions. */
> +	if (exec_only_platform)
> +		mprotect(page_to_copy, page_sz, PROT_READ | PROT_EXEC);

Is there a chance 2nd page will be something else than code?
E.g. some section that was previously also writeable.


>  	if (page_present(page_to_copy))
>  		memcpy(mem, page_to_copy, page_sz);
>  	else
> @@ -271,6 +290,9 @@ static void testfunc_protexec(void)
>  	func = get_func(p);
>  #endif
>  
> +	if (!func)
> +		goto out;
> +
>  	/* Change the protection to PROT_EXEC. */
>  	TEST(mprotect(p, copy_sz, PROT_EXEC));
>  
> @@ -294,6 +316,7 @@ static void testfunc_protexec(void)
>  		}
>  	}
>  
> +out:
>  	SAFE_MUNMAP(cleanup, p, copy_sz);
>  }
>  
> --
> 2.20.1.611.gfbb209baf1-goog
> 
> 

[LTP] [PATCH] mprotect04: Support execute-only page access permissions

[Daniel Mentz]

On Wed, Feb 6, 2019 at 11:04 PM Jan Stancek <jstancek@redhat.com> wrote:
> > +     /* Mark page readable on platforms that support execute-only page access
> > +      * permissions. */
> > +     if (exec_only_platform)
> > +             mprotect(page_to_copy, page_sz, PROT_READ | PROT_EXEC);
>
> Is there a chance 2nd page will be something else than code?
> E.g. some section that was previously also writeable.

Ok. I can see that concern. We could read /proc/$$/maps to determine
if the 2nd page is writable, but that'd be a lot of work to implement.
What about making the 2nd page writable (PROT_WRITE) just in case? Is
that a solution you would support? Can you think of another solution?

[LTP] [PATCH] mprotect04: Support execute-only page access permissions

[Jan Stancek]

----- Original Message -----
> From: "Daniel Mentz" <danielmentz@google.com>
> To: "Jan Stancek" <jstancek@redhat.com>
> Cc: ltp@lists.linux.it, liwang@redhat.com, "peter maydell" <peter.maydell@linaro.org>, chrubis@suse.cz, "gux fnst"
> <gux.fnst@cn.fujitsu.com>
> Sent: Friday, 8 February, 2019 1:12:41 AM
> Subject: Re: [PATCH] mprotect04: Support execute-only page access permissions
> 
> On Wed, Feb 6, 2019 at 11:04 PM Jan Stancek <jstancek@redhat.com> wrote:
> > > +     /* Mark page readable on platforms that support execute-only page
> > > access
> > > +      * permissions. */
> > > +     if (exec_only_platform)
> > > +             mprotect(page_to_copy, page_sz, PROT_READ | PROT_EXEC);
> >
> > Is there a chance 2nd page will be something else than code?
> > E.g. some section that was previously also writeable.
> 
> Ok. I can see that concern. We could read /proc/$$/maps to determine
> if the 2nd page is writable, but that'd be a lot of work to implement.
> What about making the 2nd page writable (PROT_WRITE) just in case? Is
> that a solution you would support? Can you think of another solution?
> 

I'm thinking using another "(!page_present(page_to_copy))" check
for 2nd page as well.

Or making sure we never cross page boundary, then we
could drop 2nd page entirely:

---

diff --git a/testcases/kernel/syscalls/mprotect/Makefile b/testcases/kernel/syscalls/mprotect/Makefile
index bd617d806675..bc5c8bc10395 100644
--- a/testcases/kernel/syscalls/mprotect/Makefile
+++ b/testcases/kernel/syscalls/mprotect/Makefile
@@ -20,4 +20,6 @@ top_srcdir            ?= ../../../..

 include $(top_srcdir)/include/mk/testcases.mk

+mprotect04: CFLAGS += -falign-functions=64
+
 include $(top_srcdir)/include/mk/generic_leaf_target.mk
diff --git a/testcases/kernel/syscalls/mprotect/mprotect04.c b/testcases/kernel/syscalls/mprotect/mprotect04.c
index 60941a4220d5..6894b31fc528 100644
--- a/testcases/kernel/syscalls/mprotect/mprotect04.c
+++ b/testcases/kernel/syscalls/mprotect/mprotect04.c
@@ -133,7 +133,7 @@ static void testfunc_protnone(void)

 #ifdef __ia64__

-static char exec_func[] = {
+static char exec_func[] __attribute__ ((aligned (64))) = {
        0x11, 0x00, 0x00, 0x00, 0x01, 0x00, /* nop.m 0x0             */
        0x00, 0x00, 0x00, 0x02, 0x00, 0x80, /* nop.i 0x0             */
        0x08, 0x00, 0x84, 0x00,             /* br.ret.sptk.many b0;; */
@@ -237,14 +237,6 @@ static void *get_func(void *mem)
        }
        memcpy(mem, page_to_copy, page_sz);

-       /* copy 2nd page if possible */
-       mem += page_sz;
-       page_to_copy += page_sz;
-       if (page_present(page_to_copy))
-               memcpy(mem, page_to_copy, page_sz);
-       else
-               memset(mem, 0, page_sz);
-
        clear_cache(mem_start, copy_sz);

        /* return pointer to area where copy of exec_func resides */