Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1718557 > unrolled thread

[PATCH] Arm: mm: ftrace: Only set text back to ro after kernel has been marked ro

Back to article view | Back to linux.kernel

Contents

  [PATCH] Arm: mm: ftrace: Only set text back to ro after kernel has  been marked ro Steven Rostedt <rostedt@goodmis.org> - 2017-08-23 20:00 +0200
    Re: [PATCH] Arm: mm: ftrace: Only set text back to ro after kernel  has been marked ro Matthias Reichl <hias@horus.com> - 2017-08-23 20:30 +0200
    Re: [PATCH] Arm: mm: ftrace: Only set text back to ro after kernel  has been marked ro Kees Cook <keescook@chromium.org> - 2017-08-23 20:50 +0200
      Re: [PATCH] Arm: mm: ftrace: Only set text back to ro after kernel  has been marked ro Steven Rostedt <rostedt@goodmis.org> - 2017-08-23 21:10 +0200

#1718557 — [PATCH] Arm: mm: ftrace: Only set text back to ro after kernel has been marked ro

ftrace needs to modify the kernel text in order to enable function tracing.
For security reasons, the kernel text is marked to read-only (ro) at the end
of system bootup. When enabling function tracing after that, ftrace calls
arch specific code that needs to enable the modification of kernel text
while ftrace does the update, and reset it back again when finished.

The issue arises when function tracing is enabled during system bootup. The
text hasn't been marked as read-only yet, but the same code to modify the
kernel is executed, and when it is finished, it will cause the kernel to
become read-only. This causes issues for other init code that requires
modification of kernel text during system bootup. This appears to cause
issue with Raspberry Pi 2.

By implementing the feature that is used in x86 to deal with this issue, it
fixes the problem. The solution is simple. Have a variable
(kernel_set_to_readonly) get set when the system finished boot and marks the
kernel to readonly. If that variable is not set, both
kernel_set_to_readonly() and kernel_set_to_rw() return without doing any
modifications. Those functions are used by ftrace to change the permissions
of the kernel text. By not doing anything, ftrace will not mess with the
permissions when it is enabled at system bootup.

Link: http://lkml.kernel.org/r/20170821153402.7so2u364htvt6tnf@camel2.lan
Link: https://github.com/raspberrypi/linux/issues/2166#issuecomment-323355145
Reported-by: Matthias Reichl <hias@horus.com>
Cc: Russell King <linux@armlinux.org.uk>
Cc: Kees Cook <keescook@chromium.org>
Cc: Eric Anholt <eric@anholt.net>
Cc: Stefan Wahren <stefan.wahren@i2se.com>
Cc: Phil Elwell <phil@raspberrypi.org>
Cc: linux-rpi-kernel@lists.infradead.org
Cc: linux-arm-kernel@lists.infradead.org
Cc: stable@vger.kernel.org
Fixes: 80d6b0c2ee ("ARM: mm: allow text and rodata sections to be read-only")
Signed-off-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
---
 arch/arm/mm/init.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/arch/arm/mm/init.c b/arch/arm/mm/init.c
index ad80548..fd75f38 100644
--- a/arch/arm/mm/init.c
+++ b/arch/arm/mm/init.c
@@ -745,19 +745,29 @@ static int __mark_rodata_ro(void *unused)
 	return 0;
 }
 
+static int kernel_set_to_readonly;
+
 void mark_rodata_ro(void)
 {
+	kernel_set_to_readonly = 1;
+
 	stop_machine(__mark_rodata_ro, NULL, NULL);
 }
 
 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);
 }
-- 
2.9.5

[toc] | [next] | [standalone]

#1718575 — Re: [PATCH] Arm: mm: ftrace: Only set text back to ro after kernel has been marked ro

On Wed, Aug 23, 2017 at 01:58:36PM -0400, Steven Rostedt wrote:
> 
> ftrace needs to modify the kernel text in order to enable function tracing.
> For security reasons, the kernel text is marked to read-only (ro) at the end
> of system bootup. When enabling function tracing after that, ftrace calls
> arch specific code that needs to enable the modification of kernel text
> while ftrace does the update, and reset it back again when finished.
> 
> The issue arises when function tracing is enabled during system bootup. The
> text hasn't been marked as read-only yet, but the same code to modify the
> kernel is executed, and when it is finished, it will cause the kernel to
> become read-only. This causes issues for other init code that requires
> modification of kernel text during system bootup. This appears to cause
> issue with Raspberry Pi 2.
> 
> By implementing the feature that is used in x86 to deal with this issue, it
> fixes the problem. The solution is simple. Have a variable
> (kernel_set_to_readonly) get set when the system finished boot and marks the
> kernel to readonly. If that variable is not set, both
> kernel_set_to_readonly() and kernel_set_to_rw() return without doing any
> modifications. Those functions are used by ftrace to change the permissions
> of the kernel text. By not doing anything, ftrace will not mess with the
> permissions when it is enabled at system bootup.
> 
> Link: http://lkml.kernel.org/r/20170821153402.7so2u364htvt6tnf@camel2.lan
> Link: https://github.com/raspberrypi/linux/issues/2166#issuecomment-323355145
> Reported-by: Matthias Reichl <hias@horus.com>
> Cc: Russell King <linux@armlinux.org.uk>
> Cc: Kees Cook <keescook@chromium.org>
> Cc: Eric Anholt <eric@anholt.net>
> Cc: Stefan Wahren <stefan.wahren@i2se.com>
> Cc: Phil Elwell <phil@raspberrypi.org>
> Cc: linux-rpi-kernel@lists.infradead.org
> Cc: linux-arm-kernel@lists.infradead.org
> Cc: stable@vger.kernel.org
> Fixes: 80d6b0c2ee ("ARM: mm: allow text and rodata sections to be read-only")
> Signed-off-by: Steven Rostedt (VMware) <rostedt@goodmis.org>

Tested-by: Matthias Reichl <hias@horus.com>

Thanks, again, for resolving the issue so quickly!

so long,

Hias

> ---
>  arch/arm/mm/init.c | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/arch/arm/mm/init.c b/arch/arm/mm/init.c
> index ad80548..fd75f38 100644
> --- a/arch/arm/mm/init.c
> +++ b/arch/arm/mm/init.c
> @@ -745,19 +745,29 @@ static int __mark_rodata_ro(void *unused)
>  	return 0;
>  }
>  
> +static int kernel_set_to_readonly;
> +
>  void mark_rodata_ro(void)
>  {
> +	kernel_set_to_readonly = 1;
> +
>  	stop_machine(__mark_rodata_ro, NULL, NULL);
>  }
>  
>  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);
>  }
> -- 
> 2.9.5
> 

[toc] | [prev] | [next] | [standalone]

#1718581 — Re: [PATCH] Arm: mm: ftrace: Only set text back to ro after kernel has been marked ro

On Wed, Aug 23, 2017 at 10:58 AM, Steven Rostedt <rostedt@goodmis.org> wrote:
>
> ftrace needs to modify the kernel text in order to enable function tracing.
> For security reasons, the kernel text is marked to read-only (ro) at the end
> of system bootup. When enabling function tracing after that, ftrace calls
> arch specific code that needs to enable the modification of kernel text
> while ftrace does the update, and reset it back again when finished.
>
> The issue arises when function tracing is enabled during system bootup. The
> text hasn't been marked as read-only yet, but the same code to modify the
> kernel is executed, and when it is finished, it will cause the kernel to
> become read-only. This causes issues for other init code that requires
> modification of kernel text during system bootup. This appears to cause
> issue with Raspberry Pi 2.
>
> By implementing the feature that is used in x86 to deal with this issue, it
> fixes the problem. The solution is simple. Have a variable
> (kernel_set_to_readonly) get set when the system finished boot and marks the
> kernel to readonly. If that variable is not set, both
> kernel_set_to_readonly() and kernel_set_to_rw() return without doing any
> modifications. Those functions are used by ftrace to change the permissions
> of the kernel text. By not doing anything, ftrace will not mess with the
> permissions when it is enabled at system bootup.
>
> Link: http://lkml.kernel.org/r/20170821153402.7so2u364htvt6tnf@camel2.lan
> Link: https://github.com/raspberrypi/linux/issues/2166#issuecomment-323355145
> Reported-by: Matthias Reichl <hias@horus.com>
> Cc: Russell King <linux@armlinux.org.uk>
> Cc: Kees Cook <keescook@chromium.org>
> Cc: Eric Anholt <eric@anholt.net>
> Cc: Stefan Wahren <stefan.wahren@i2se.com>
> Cc: Phil Elwell <phil@raspberrypi.org>
> Cc: linux-rpi-kernel@lists.infradead.org
> Cc: linux-arm-kernel@lists.infradead.org
> Cc: stable@vger.kernel.org
> Fixes: 80d6b0c2ee ("ARM: mm: allow text and rodata sections to be read-only")
> Signed-off-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
> ---
>  arch/arm/mm/init.c | 10 ++++++++++
>  1 file changed, 10 insertions(+)
>
> diff --git a/arch/arm/mm/init.c b/arch/arm/mm/init.c
> index ad80548..fd75f38 100644
> --- a/arch/arm/mm/init.c
> +++ b/arch/arm/mm/init.c
> @@ -745,19 +745,29 @@ static int __mark_rodata_ro(void *unused)
>         return 0;
>  }
>
> +static int kernel_set_to_readonly;

Adding a comment here might be a good idea, something like:

/* Has system boot-up reached mark_rodata_ro() yet? */

Otherwise:

Acked-by: Kees Cook <keescook@chromium.org>

> +
>  void mark_rodata_ro(void)
>  {
> +       kernel_set_to_readonly = 1;
> +
>         stop_machine(__mark_rodata_ro, NULL, NULL);
>  }
>
>  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);
>  }

Does arm64 suffer from a similar condition? (It looks like no, as text
patching is done with a fixmap poke.)

-Kees

-- 
Kees Cook
Pixel Security

[toc] | [prev] | [next] | [standalone]

#1718588 — Re: [PATCH] Arm: mm: ftrace: Only set text back to ro after kernel has been marked ro

On Wed, 23 Aug 2017 11:48:13 -0700
Kees Cook <keescook@chromium.org> wrote:

> > diff --git a/arch/arm/mm/init.c b/arch/arm/mm/init.c
> > index ad80548..fd75f38 100644
> > --- a/arch/arm/mm/init.c
> > +++ b/arch/arm/mm/init.c
> > @@ -745,19 +745,29 @@ static int __mark_rodata_ro(void *unused)
> >         return 0;
> >  }
> >
> > +static int kernel_set_to_readonly;  
> 
> Adding a comment here might be a good idea, something like:
> 
> /* Has system boot-up reached mark_rodata_ro() yet? */

I don't mind adding a comment, but the above is rather self explanatory
(one can easily see that it is set in mark_rodata_ro() with a simple
search).

If a comment is to be added, something a bit more descriptive of the
functionality of the variable would be appropriate:

/*
 * Ignore modifying kernel text permissions until the kernel core calls
 * make_rodata_ro() at system start up.
 */

I can resend with the comment, or whoever takes this could add it
themselves.

-- Steve


> 
> Otherwise:
> 
> Acked-by: Kees Cook <keescook@chromium.org>
> 
> > +
> >  void mark_rodata_ro(void)
> >  {
> > +       kernel_set_to_readonly = 1;
> > +
> >         stop_machine(__mark_rodata_ro, NULL, NULL);
> >  }
> >
> >  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);
> >  }  
> 
> Does arm64 suffer from a similar condition? (It looks like no, as text
> patching is done with a fixmap poke.)
> 
> -Kees
> 

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web