| Message ID | 1524866145-20337-1-git-send-email-jhugo@codeaurora.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, Apr 27, 2018 at 2:55 PM, Jeffrey Hugo <jhugo@codeaurora.org> wrote: > load_module() creates W+X mappings via __vmalloc_node_range() (from > layout_and_allocate()->move_module()->module_alloc()) by using > PAGE_KERNEL_EXEC. These mappings are later cleaned up via > "call_rcu_sched(&freeinit->rcu, do_free_init)" from do_init_module(). > > This is a problem because call_rcu_sched() queues work, which can be run > after debug_checkwx() is run, resulting in a race condition. If hit, the > race results in a nasty splat about insecure W+X mappings, which results > in a poor user experience as these are not the mappings that > debug_checkwx() is intended to catch. > > This issue is observed on multiple arm64 platforms, and has been > artificially triggered on an x86 platform. > > Address the race by flushing the queued work before running the > arch-defined mark_rodata_ro() which then calls debug_checkwx(). > > Reported-by: Timur Tabi <timur@codeaurora.org> > Reported-by: Jan Glauber <jan.glauber@caviumnetworks.com> > Fixes: e1a58320a38d ("x86/mm: Warn on W^X mappings") > Signed-off-by: Jeffrey Hugo <jhugo@codeaurora.org> Acked-by: Kees Cook <keescook@chromium.org> -Kees > --- > > v1: > -was "arm64: mm: Fix false positives in W+X checking" (see [1]) > -moved to common code based on review and confirmation of issue on x86 > > [1] http://lists.infradead.org/pipermail/linux-arm-kernel/2018-April/573776.html > > init/main.c | 7 +++++++ > 1 file changed, 7 insertions(+) > > diff --git a/init/main.c b/init/main.c > index b795aa3..499d957 100644 > --- a/init/main.c > +++ b/init/main.c > @@ -1034,6 +1034,13 @@ static int __init set_debug_rodata(char *str) > static void mark_readonly(void) > { > if (rodata_enabled) { > + /* > + * load_module() results in W+X mappings, which are cleaned up > + * with call_rcu_sched(). Let's make sure that queued work is > + * flushed so that we don't hit false positives looking for > + * insecure pages which are W+X. > + */ > + rcu_barrier_sched(); > mark_rodata_ro(); > rodata_test(); > } else > -- > Qualcomm Datacenter Technologies as an affiliate of Qualcomm Technologies, Inc. > Qualcomm Technologies, Inc. is a member of the > Code Aurora Forum, a Linux Foundation Collaborative Project. >
* Jeffrey Hugo <jhugo@codeaurora.org> wrote: > load_module() creates W+X mappings via __vmalloc_node_range() (from > layout_and_allocate()->move_module()->module_alloc()) by using > PAGE_KERNEL_EXEC. These mappings are later cleaned up via > "call_rcu_sched(&freeinit->rcu, do_free_init)" from do_init_module(). > > This is a problem because call_rcu_sched() queues work, which can be run > after debug_checkwx() is run, resulting in a race condition. If hit, the > race results in a nasty splat about insecure W+X mappings, which results > in a poor user experience as these are not the mappings that > debug_checkwx() is intended to catch. > > This issue is observed on multiple arm64 platforms, and has been > artificially triggered on an x86 platform. > > Address the race by flushing the queued work before running the > arch-defined mark_rodata_ro() which then calls debug_checkwx(). > > Reported-by: Timur Tabi <timur@codeaurora.org> > Reported-by: Jan Glauber <jan.glauber@caviumnetworks.com> > Fixes: e1a58320a38d ("x86/mm: Warn on W^X mappings") > Signed-off-by: Jeffrey Hugo <jhugo@codeaurora.org> > --- > > v1: > -was "arm64: mm: Fix false positives in W+X checking" (see [1]) > -moved to common code based on review and confirmation of issue on x86 > > [1] http://lists.infradead.org/pipermail/linux-arm-kernel/2018-April/573776.html > > init/main.c | 7 +++++++ > 1 file changed, 7 insertions(+) > > diff --git a/init/main.c b/init/main.c > index b795aa3..499d957 100644 > --- a/init/main.c > +++ b/init/main.c > @@ -1034,6 +1034,13 @@ static int __init set_debug_rodata(char *str) > static void mark_readonly(void) > { > if (rodata_enabled) { > + /* > + * load_module() results in W+X mappings, which are cleaned up > + * with call_rcu_sched(). Let's make sure that queued work is > + * flushed so that we don't hit false positives looking for > + * insecure pages which are W+X. > + */ > + rcu_barrier_sched(); I'd suggest adding a matching comment to the module loading portion as well, to make sure this connection does not get lost. With that: Acked-by: Ingo Molnar <mingo@kernel.org> Thanks, Ingo
On Fri, Apr 27, 2018 at 03:55:45PM -0600, Jeffrey Hugo wrote: > load_module() creates W+X mappings via __vmalloc_node_range() (from > layout_and_allocate()->move_module()->module_alloc()) by using > PAGE_KERNEL_EXEC. These mappings are later cleaned up via > "call_rcu_sched(&freeinit->rcu, do_free_init)" from do_init_module(). > > This is a problem because call_rcu_sched() queues work, which can be run > after debug_checkwx() is run, resulting in a race condition. If hit, the > race results in a nasty splat about insecure W+X mappings, which results > in a poor user experience as these are not the mappings that > debug_checkwx() is intended to catch. > > This issue is observed on multiple arm64 platforms, and has been > artificially triggered on an x86 platform. > > Address the race by flushing the queued work before running the > arch-defined mark_rodata_ro() which then calls debug_checkwx(). > > Reported-by: Timur Tabi <timur@codeaurora.org> > Reported-by: Jan Glauber <jan.glauber@caviumnetworks.com> > Fixes: e1a58320a38d ("x86/mm: Warn on W^X mappings") > Signed-off-by: Jeffrey Hugo <jhugo@codeaurora.org> > --- > > v1: > -was "arm64: mm: Fix false positives in W+X checking" (see [1]) > -moved to common code based on review and confirmation of issue on x86 > > [1] http://lists.infradead.org/pipermail/linux-arm-kernel/2018-April/573776.html > > init/main.c | 7 +++++++ > 1 file changed, 7 insertions(+) > > diff --git a/init/main.c b/init/main.c > index b795aa3..499d957 100644 > --- a/init/main.c > +++ b/init/main.c > @@ -1034,6 +1034,13 @@ static int __init set_debug_rodata(char *str) > static void mark_readonly(void) > { > if (rodata_enabled) { > + /* > + * load_module() results in W+X mappings, which are cleaned up > + * with call_rcu_sched(). Let's make sure that queued work is > + * flushed so that we don't hit false positives looking for > + * insecure pages which are W+X. > + */ > + rcu_barrier_sched(); > mark_rodata_ro(); > rodata_test(); > } else Acked-by: Will Deacon <will.deacon@arm.com> Thanks for solving this for all architectures, Jeffrey. Will
On 4/28/2018 12:14 AM, Ingo Molnar wrote: > > * Jeffrey Hugo <jhugo@codeaurora.org> wrote: > >> load_module() creates W+X mappings via __vmalloc_node_range() (from >> layout_and_allocate()->move_module()->module_alloc()) by using >> PAGE_KERNEL_EXEC. These mappings are later cleaned up via >> "call_rcu_sched(&freeinit->rcu, do_free_init)" from do_init_module(). >> >> This is a problem because call_rcu_sched() queues work, which can be run >> after debug_checkwx() is run, resulting in a race condition. If hit, the >> race results in a nasty splat about insecure W+X mappings, which results >> in a poor user experience as these are not the mappings that >> debug_checkwx() is intended to catch. >> >> This issue is observed on multiple arm64 platforms, and has been >> artificially triggered on an x86 platform. >> >> Address the race by flushing the queued work before running the >> arch-defined mark_rodata_ro() which then calls debug_checkwx(). >> >> Reported-by: Timur Tabi <timur@codeaurora.org> >> Reported-by: Jan Glauber <jan.glauber@caviumnetworks.com> >> Fixes: e1a58320a38d ("x86/mm: Warn on W^X mappings") >> Signed-off-by: Jeffrey Hugo <jhugo@codeaurora.org> >> --- >> >> v1: >> -was "arm64: mm: Fix false positives in W+X checking" (see [1]) >> -moved to common code based on review and confirmation of issue on x86 >> >> [1] http://lists.infradead.org/pipermail/linux-arm-kernel/2018-April/573776.html >> >> init/main.c | 7 +++++++ >> 1 file changed, 7 insertions(+) >> >> diff --git a/init/main.c b/init/main.c >> index b795aa3..499d957 100644 >> --- a/init/main.c >> +++ b/init/main.c >> @@ -1034,6 +1034,13 @@ static int __init set_debug_rodata(char *str) >> static void mark_readonly(void) >> { >> if (rodata_enabled) { >> + /* >> + * load_module() results in W+X mappings, which are cleaned up >> + * with call_rcu_sched(). Let's make sure that queued work is >> + * flushed so that we don't hit false positives looking for >> + * insecure pages which are W+X. >> + */ >> + rcu_barrier_sched(); > > I'd suggest adding a matching comment to the module loading portion as well, > to make sure this connection does not get lost. With that: > > Acked-by: Ingo Molnar <mingo@kernel.org> Sure. Will add that in a v3.
diff --git a/init/main.c b/init/main.c index b795aa3..499d957 100644 --- a/init/main.c +++ b/init/main.c @@ -1034,6 +1034,13 @@ static int __init set_debug_rodata(char *str) static void mark_readonly(void) { if (rodata_enabled) { + /* + * load_module() results in W+X mappings, which are cleaned up + * with call_rcu_sched(). Let's make sure that queued work is + * flushed so that we don't hit false positives looking for + * insecure pages which are W+X. + */ + rcu_barrier_sched(); mark_rodata_ro(); rodata_test(); } else
load_module() creates W+X mappings via __vmalloc_node_range() (from layout_and_allocate()->move_module()->module_alloc()) by using PAGE_KERNEL_EXEC. These mappings are later cleaned up via "call_rcu_sched(&freeinit->rcu, do_free_init)" from do_init_module(). This is a problem because call_rcu_sched() queues work, which can be run after debug_checkwx() is run, resulting in a race condition. If hit, the race results in a nasty splat about insecure W+X mappings, which results in a poor user experience as these are not the mappings that debug_checkwx() is intended to catch. This issue is observed on multiple arm64 platforms, and has been artificially triggered on an x86 platform. Address the race by flushing the queued work before running the arch-defined mark_rodata_ro() which then calls debug_checkwx(). Reported-by: Timur Tabi <timur@codeaurora.org> Reported-by: Jan Glauber <jan.glauber@caviumnetworks.com> Fixes: e1a58320a38d ("x86/mm: Warn on W^X mappings") Signed-off-by: Jeffrey Hugo <jhugo@codeaurora.org> --- v1: -was "arm64: mm: Fix false positives in W+X checking" (see [1]) -moved to common code based on review and confirmation of issue on x86 [1] http://lists.infradead.org/pipermail/linux-arm-kernel/2018-April/573776.html init/main.c | 7 +++++++ 1 file changed, 7 insertions(+)