[08/17] x86/ftrace: set trampoline pages as executable
diff mbox series

Message ID 20190117003259.23141-9-rick.p.edgecombe@intel.com
State New
Headers show
Series
  • Merge text_poke fixes and executable lockdowns
Related show

Commit Message

Edgecombe, Rick P Jan. 17, 2019, 12:32 a.m. UTC
From: Nadav Amit <namit@vmware.com>

Since alloc_module() will not set the pages as executable soon, we need
to do so for ftrace trampoline pages after they are allocated.

For the time being, we do not change ftrace to use the text_poke()
interface. As a result, ftrace breaks still breaks W^X.

Cc: Steven Rostedt <rostedt@goodmis.org>
Signed-off-by: Nadav Amit <namit@vmware.com>
Signed-off-by: Rick Edgecombe <rick.p.edgecombe@intel.com>
---
 arch/x86/kernel/ftrace.c | 9 +++++++++
 1 file changed, 9 insertions(+)

Comments

Steven Rostedt Feb. 6, 2019, 4:22 p.m. UTC | #1
On Wed, 16 Jan 2019 16:32:50 -0800
Rick Edgecombe <rick.p.edgecombe@intel.com> wrote:

> From: Nadav Amit <namit@vmware.com>
> 
> Since alloc_module() will not set the pages as executable soon, we need
> to do so for ftrace trampoline pages after they are allocated.
> 
> For the time being, we do not change ftrace to use the text_poke()
> interface. As a result, ftrace breaks still breaks W^X.
> 
> Cc: Steven Rostedt <rostedt@goodmis.org>
> Signed-off-by: Nadav Amit <namit@vmware.com>
> Signed-off-by: Rick Edgecombe <rick.p.edgecombe@intel.com>
> ---
>  arch/x86/kernel/ftrace.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/arch/x86/kernel/ftrace.c b/arch/x86/kernel/ftrace.c
> index 8257a59704ae..eb4a1937e72c 100644
> --- a/arch/x86/kernel/ftrace.c
> +++ b/arch/x86/kernel/ftrace.c
> @@ -742,6 +742,7 @@ create_trampoline(struct ftrace_ops *ops, unsigned int *tramp_size)
>  	unsigned long end_offset;
>  	unsigned long op_offset;
>  	unsigned long offset;
> +	unsigned long npages;
>  	unsigned long size;
>  	unsigned long retq;
>  	unsigned long *ptr;
> @@ -774,6 +775,7 @@ create_trampoline(struct ftrace_ops *ops, unsigned int *tramp_size)
>  		return 0;
>  
>  	*tramp_size = size + RET_SIZE + sizeof(void *);
> +	npages = DIV_ROUND_UP(*tramp_size, PAGE_SIZE);
>  
>  	/* Copy ftrace_caller onto the trampoline memory */
>  	ret = probe_kernel_read(trampoline, (void *)start_offset, size);
> @@ -818,6 +820,13 @@ create_trampoline(struct ftrace_ops *ops, unsigned int *tramp_size)
>  	/* ALLOC_TRAMP flags lets us know we created it */
>  	ops->flags |= FTRACE_OPS_FL_ALLOC_TRAMP;
>  
> +	/*
> +	 * Module allocation needs to be completed by making the page
> +	 * executable. The page is still writable, which is a security hazard,
> +	 * but anyhow ftrace breaks W^X completely.
> +	 */

Perhaps we should set the page to non writable after the page is
updated? And set it to writable only when we need to update it.

As for this patch:

Reviewed-by: Steven Rostedt (VMware) <rostedt@goodmis.org>

-- Steve

> +	set_memory_x((unsigned long)trampoline, npages);
> +
>  	return (unsigned long)trampoline;
>  fail:
>  	tramp_free(trampoline, *tramp_size);
Nadav Amit Feb. 6, 2019, 5:33 p.m. UTC | #2
> On Feb 6, 2019, at 8:22 AM, Steven Rostedt <rostedt@goodmis.org> wrote:
> 
> On Wed, 16 Jan 2019 16:32:50 -0800
> Rick Edgecombe <rick.p.edgecombe@intel.com> wrote:
> 
>> From: Nadav Amit <namit@vmware.com>
>> 
>> Since alloc_module() will not set the pages as executable soon, we need
>> to do so for ftrace trampoline pages after they are allocated.
>> 
>> For the time being, we do not change ftrace to use the text_poke()
>> interface. As a result, ftrace breaks still breaks W^X.
>> 
>> Cc: Steven Rostedt <rostedt@goodmis.org>
>> Signed-off-by: Nadav Amit <namit@vmware.com>
>> Signed-off-by: Rick Edgecombe <rick.p.edgecombe@intel.com>
>> ---
>> arch/x86/kernel/ftrace.c | 9 +++++++++
>> 1 file changed, 9 insertions(+)
>> 
>> diff --git a/arch/x86/kernel/ftrace.c b/arch/x86/kernel/ftrace.c
>> index 8257a59704ae..eb4a1937e72c 100644
>> --- a/arch/x86/kernel/ftrace.c
>> +++ b/arch/x86/kernel/ftrace.c
>> @@ -742,6 +742,7 @@ create_trampoline(struct ftrace_ops *ops, unsigned int *tramp_size)
>> 	unsigned long end_offset;
>> 	unsigned long op_offset;
>> 	unsigned long offset;
>> +	unsigned long npages;
>> 	unsigned long size;
>> 	unsigned long retq;
>> 	unsigned long *ptr;
>> @@ -774,6 +775,7 @@ create_trampoline(struct ftrace_ops *ops, unsigned int *tramp_size)
>> 		return 0;
>> 
>> 	*tramp_size = size + RET_SIZE + sizeof(void *);
>> +	npages = DIV_ROUND_UP(*tramp_size, PAGE_SIZE);
>> 
>> 	/* Copy ftrace_caller onto the trampoline memory */
>> 	ret = probe_kernel_read(trampoline, (void *)start_offset, size);
>> @@ -818,6 +820,13 @@ create_trampoline(struct ftrace_ops *ops, unsigned int *tramp_size)
>> 	/* ALLOC_TRAMP flags lets us know we created it */
>> 	ops->flags |= FTRACE_OPS_FL_ALLOC_TRAMP;
>> 
>> +	/*
>> +	 * Module allocation needs to be completed by making the page
>> +	 * executable. The page is still writable, which is a security hazard,
>> +	 * but anyhow ftrace breaks W^X completely.
>> +	 */
> 
> Perhaps we should set the page to non writable after the page is
> updated? And set it to writable only when we need to update it.

You remember that I sent you a patch that changed all these writes into
text_poke() and you said that I should defer it until this series is merged?

> As for this patch:
> 
> Reviewed-by: Steven Rostedt (VMware) <rostedt@goodmis.org>

Thanks!
Steven Rostedt Feb. 6, 2019, 5:41 p.m. UTC | #3
On Wed, 6 Feb 2019 09:33:35 -0800
Nadav Amit <nadav.amit@gmail.com> wrote:


> >> 	/* Copy ftrace_caller onto the trampoline memory */
> >> 	ret = probe_kernel_read(trampoline, (void *)start_offset, size);
> >> @@ -818,6 +820,13 @@ create_trampoline(struct ftrace_ops *ops, unsigned int *tramp_size)
> >> 	/* ALLOC_TRAMP flags lets us know we created it */
> >> 	ops->flags |= FTRACE_OPS_FL_ALLOC_TRAMP;
> >> 
> >> +	/*
> >> +	 * Module allocation needs to be completed by making the page
> >> +	 * executable. The page is still writable, which is a security hazard,
> >> +	 * but anyhow ftrace breaks W^X completely.
> >> +	 */  
> > 
> > Perhaps we should set the page to non writable after the page is
> > updated? And set it to writable only when we need to update it.  
> 
> You remember that I sent you a patch that changed all these writes into
> text_poke() and you said that I should defer it until this series is merged?
> 

And I notice that it is set to RO after this call anyway.

-- Steve

Patch
diff mbox series

diff --git a/arch/x86/kernel/ftrace.c b/arch/x86/kernel/ftrace.c
index 8257a59704ae..eb4a1937e72c 100644
--- a/arch/x86/kernel/ftrace.c
+++ b/arch/x86/kernel/ftrace.c
@@ -742,6 +742,7 @@  create_trampoline(struct ftrace_ops *ops, unsigned int *tramp_size)
 	unsigned long end_offset;
 	unsigned long op_offset;
 	unsigned long offset;
+	unsigned long npages;
 	unsigned long size;
 	unsigned long retq;
 	unsigned long *ptr;
@@ -774,6 +775,7 @@  create_trampoline(struct ftrace_ops *ops, unsigned int *tramp_size)
 		return 0;
 
 	*tramp_size = size + RET_SIZE + sizeof(void *);
+	npages = DIV_ROUND_UP(*tramp_size, PAGE_SIZE);
 
 	/* Copy ftrace_caller onto the trampoline memory */
 	ret = probe_kernel_read(trampoline, (void *)start_offset, size);
@@ -818,6 +820,13 @@  create_trampoline(struct ftrace_ops *ops, unsigned int *tramp_size)
 	/* ALLOC_TRAMP flags lets us know we created it */
 	ops->flags |= FTRACE_OPS_FL_ALLOC_TRAMP;
 
+	/*
+	 * Module allocation needs to be completed by making the page
+	 * executable. The page is still writable, which is a security hazard,
+	 * but anyhow ftrace breaks W^X completely.
+	 */
+	set_memory_x((unsigned long)trampoline, npages);
+
 	return (unsigned long)trampoline;
 fail:
 	tramp_free(trampoline, *tramp_size);