ARM: ftrace: Only set kernel memory back to read-only after boot

Commit Message

Steven Rostedt June 21, 2018, 4:47 p.m. UTC

From: Steven Rostedt (VMware) <[e-mail]>

Dynamic ftrace requires modifying the code segments that are usually
set to read-only. To do this, a per arch function is called both before
and after the ftrace modifications are performed. The "before" function
will set kernel code text to read-write to allow for ftrace to make the
modifications, and the "after" function will set the kernel code text
back to "read-only" to keep the kernel code text protected.

The issue happens when dynamic ftrace is tested at boot up. The test is
done before the kernel code text has been set to read-only. But the
"before" and "after" calls are still performed. The "after" call will
change the kernel code text to read-only prematurely, and other boot
code that expects this code to be read-write will fail.

The solution is to add a variable that is set when the kernel code text
is expected to be converted to read-only, and make the ftrace "before"
and "after" calls do nothing if that variable is not yet set. This is
similar to the x86 solution from commit 162396309745 ("ftrace, x86:
make kernel text writable only for conversions").

Reported-by: Stefan Agner <[e-mail]>
Tested-by: Stefan Agner <[e-mail]>
Link: http://lkml.kernel.org/r/[e-mail]
Signed-off-by: Steven Rostedt (VMware) <[e-mail]>
---

Comments

Steven Rostedt June 21, 2018, 4:50 p.m. UTC | #1

On Thu, 21 Jun 2018 12:47:10 -0400
Steven Rostedt <[e-mail]> wrote:

> From: Steven Rostedt (VMware) <[e-mail]>
> 
> Dynamic ftrace requires modifying the code segments that are usually
> set to read-only. To do this, a per arch function is called both before
> and after the ftrace modifications are performed. The "before" function
> will set kernel code text to read-write to allow for ftrace to make the
> modifications, and the "after" function will set the kernel code text
> back to "read-only" to keep the kernel code text protected.
> 
> The issue happens when dynamic ftrace is tested at boot up. The test is
> done before the kernel code text has been set to read-only. But the
> "before" and "after" calls are still performed. The "after" call will
> change the kernel code text to read-only prematurely, and other boot
> code that expects this code to be read-write will fail.
> 
> The solution is to add a variable that is set when the kernel code text
> is expected to be converted to read-only, and make the ftrace "before"
> and "after" calls do nothing if that variable is not yet set. This is
> similar to the x86 solution from commit 162396309745 ("ftrace, x86:
> make kernel text writable only for conversions").
> 
> Reported-by: Stefan Agner <[e-mail]>
> Tested-by: Stefan Agner <[e-mail]>
> Link: http://lkml.kernel.org/r/[e-mail]
> Signed-off-by: Steven Rostedt (VMware) <[e-mail]>
> ---

Perhaps I should have Cc'd stable too?

-- Steve

Stefan Agner June 21, 2018, 5:54 p.m. UTC | #2

On 21.06.2018 18:50, Steven Rostedt wrote:
> On Thu, 21 Jun 2018 12:47:10 -0400
> Steven Rostedt <[e-mail]> wrote:
> 
>> From: Steven Rostedt (VMware) <[e-mail]>
>>
>> Dynamic ftrace requires modifying the code segments that are usually
>> set to read-only. To do this, a per arch function is called both before
>> and after the ftrace modifications are performed. The "before" function
>> will set kernel code text to read-write to allow for ftrace to make the
>> modifications, and the "after" function will set the kernel code text
>> back to "read-only" to keep the kernel code text protected.
>>
>> The issue happens when dynamic ftrace is tested at boot up. The test is
>> done before the kernel code text has been set to read-only. But the
>> "before" and "after" calls are still performed. The "after" call will
>> change the kernel code text to read-only prematurely, and other boot
>> code that expects this code to be read-write will fail.
>>
>> The solution is to add a variable that is set when the kernel code text
>> is expected to be converted to read-only, and make the ftrace "before"
>> and "after" calls do nothing if that variable is not yet set. This is
>> similar to the x86 solution from commit 162396309745 ("ftrace, x86:
>> make kernel text writable only for conversions").
>>
>> Reported-by: Stefan Agner <[e-mail]>
>> Tested-by: Stefan Agner <[e-mail]>
>> Link: http://lkml.kernel.org/r/[e-mail]
>> Signed-off-by: Steven Rostedt (VMware) <[e-mail]>
>> ---
> 
> Perhaps I should have Cc'd stable too?
> 

As it is self tests only which are broken probably not super important.
But then, the fix is also rather clean and safe IMHO, so why not.

--
Stefan

Stefan Agner July 4, 2018, 11:30 a.m. UTC | #3

On 21.06.2018 18:47, Steven Rostedt wrote:
> From: Steven Rostedt (VMware) <[e-mail]>
> 
> Dynamic ftrace requires modifying the code segments that are usually
> set to read-only. To do this, a per arch function is called both before
> and after the ftrace modifications are performed. The "before" function
> will set kernel code text to read-write to allow for ftrace to make the
> modifications, and the "after" function will set the kernel code text
> back to "read-only" to keep the kernel code text protected.
> 
> The issue happens when dynamic ftrace is tested at boot up. The test is
> done before the kernel code text has been set to read-only. But the
> "before" and "after" calls are still performed. The "after" call will
> change the kernel code text to read-only prematurely, and other boot
> code that expects this code to be read-write will fail.
> 
> The solution is to add a variable that is set when the kernel code text
> is expected to be converted to read-only, and make the ftrace "before"
> and "after" calls do nothing if that variable is not yet set. This is
> similar to the x86 solution from commit 162396309745 ("ftrace, x86:
> make kernel text writable only for conversions").
> 
> Reported-by: Stefan Agner <[e-mail]>
> Tested-by: Stefan Agner <[e-mail]>
> Link: http://lkml.kernel.org/r/[e-mail]
> Signed-off-by: Steven Rostedt (VMware) <[e-mail]>
> ---
> diff --git a/arch/arm/mm/init.c b/arch/arm/mm/init.c
> index c186474422f3..0cc8e04295a4 100644
> --- a/arch/arm/mm/init.c
> +++ b/arch/arm/mm/init.c

I guess this can be seen as reviewed?

Patches touching core ARM port usually go through Russels patch tracker.

You have to submit the patch here:
http://www.armlinux.org.uk/developer/patches/

--
Stefan


> @@ -736,20 +736,29 @@ static int __mark_rodata_ro(void *unused)
>  	return 0;
>  }
>  
> +static int kernel_set_to_readonly __read_mostly;
> +
>  void mark_rodata_ro(void)
>  {
> +	kernel_set_to_readonly = 1;
>  	stop_machine(__mark_rodata_ro, NULL, NULL);
>  	debug_checkwx();
>  }
>  
>  void set_kernel_text_rw(void)
>  {
> +	if (!kernel_set_to_readonly)
> +		return;
> +
>  	set_section_perms(ro_perms, ARRAY_SIZE(ro_perms), false,
>  				current->active_mm);
>  }
>  
>  void set_kernel_text_ro(void)
>  {
> +	if (!kernel_set_to_readonly)
> +		return;
> +
>  	set_section_perms(ro_perms, ARRAY_SIZE(ro_perms), true,
>  				current->active_mm);
>  }

Stefan Agner July 10, 2018, 7:24 a.m. UTC | #4

On 04.07.2018 13:30, Stefan Agner wrote:
> On 21.06.2018 18:47, Steven Rostedt wrote:
>> From: Steven Rostedt (VMware) <[e-mail]>
>>
>> Dynamic ftrace requires modifying the code segments that are usually
>> set to read-only. To do this, a per arch function is called both before
>> and after the ftrace modifications are performed. The "before" function
>> will set kernel code text to read-write to allow for ftrace to make the
>> modifications, and the "after" function will set the kernel code text
>> back to "read-only" to keep the kernel code text protected.
>>
>> The issue happens when dynamic ftrace is tested at boot up. The test is
>> done before the kernel code text has been set to read-only. But the
>> "before" and "after" calls are still performed. The "after" call will
>> change the kernel code text to read-only prematurely, and other boot
>> code that expects this code to be read-write will fail.
>>
>> The solution is to add a variable that is set when the kernel code text
>> is expected to be converted to read-only, and make the ftrace "before"
>> and "after" calls do nothing if that variable is not yet set. This is
>> similar to the x86 solution from commit 162396309745 ("ftrace, x86:
>> make kernel text writable only for conversions").
>>
>> Reported-by: Stefan Agner <[e-mail]>
>> Tested-by: Stefan Agner <[e-mail]>
>> Link: http://lkml.kernel.org/r/[e-mail]
>> Signed-off-by: Steven Rostedt (VMware) <[e-mail]>
>> ---
>> diff --git a/arch/arm/mm/init.c b/arch/arm/mm/init.c
>> index c186474422f3..0cc8e04295a4 100644
>> --- a/arch/arm/mm/init.c
>> +++ b/arch/arm/mm/init.c
> 
> I guess this can be seen as reviewed?
> 
> Patches touching core ARM port usually go through Russels patch tracker.
> 
> You have to submit the patch here:
> http://www.armlinux.org.uk/developer/patches/
> 

FWIW, I added this patch to the patch tracker, its patch ID is 8780/1.

--
Stefan

> 
> 
>> @@ -736,20 +736,29 @@ static int __mark_rodata_ro(void *unused)
>>  	return 0;
>>  }
>>
>> +static int kernel_set_to_readonly __read_mostly;
>> +
>>  void mark_rodata_ro(void)
>>  {
>> +	kernel_set_to_readonly = 1;
>>  	stop_machine(__mark_rodata_ro, NULL, NULL);
>>  	debug_checkwx();
>>  }
>>
>>  void set_kernel_text_rw(void)
>>  {
>> +	if (!kernel_set_to_readonly)
>> +		return;
>> +
>>  	set_section_perms(ro_perms, ARRAY_SIZE(ro_perms), false,
>>  				current->active_mm);
>>  }
>>
>>  void set_kernel_text_ro(void)
>>  {
>> +	if (!kernel_set_to_readonly)
>> +		return;
>> +
>>  	set_section_perms(ro_perms, ARRAY_SIZE(ro_perms), true,
>>  				current->active_mm);
>>  }

Patch

diff --git a/arch/arm/mm/init.c b/arch/arm/mm/init.c
index c186474422f3..0cc8e04295a4 100644
--- a/arch/arm/mm/init.c
+++ b/arch/arm/mm/init.c
@@ -736,20 +736,29 @@  static int __mark_rodata_ro(void *unused)
 	return 0;
 }
 
+static int kernel_set_to_readonly __read_mostly;
+
 void mark_rodata_ro(void)
 {
+	kernel_set_to_readonly = 1;
 	stop_machine(__mark_rodata_ro, NULL, NULL);
 	debug_checkwx();
 }
 
 void set_kernel_text_rw(void)
 {
+	if (!kernel_set_to_readonly)
+		return;
+
 	set_section_perms(ro_perms, ARRAY_SIZE(ro_perms), false,
 				current->active_mm);
 }
 
 void set_kernel_text_ro(void)
 {
+	if (!kernel_set_to_readonly)
+		return;
+
 	set_section_perms(ro_perms, ARRAY_SIZE(ro_perms), true,
 				current->active_mm);
 }