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 <[email protected]>
Reported-by: Jan Glauber <[email protected]>
Fixes: e1a58320a38d ("x86/mm: Warn on W^X mappings")
Signed-off-by: Jeffrey Hugo <[email protected]>
Acked-by: Kees Cook <[email protected]>
Acked-by: Ingo Molnar <[email protected]>
Acked-by: Will Deacon <[email protected]>
---
v3:
-added comment to module code to establish matching connection
-added acks by Kees, Ingo, and Will
v2:
-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 +++++++
kernel/module.c | 5 +++++
2 files changed, 12 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
diff --git a/kernel/module.c b/kernel/module.c
index a6e43a5..7b71c3c 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -3516,6 +3516,11 @@ static noinline int do_init_module(struct module *mod)
* walking this with preempt disabled. In all the failure paths, we
* call synchronize_sched(), but we don't want to slow down the success
* path, so use actual RCU here.
+ * Note that module_alloc() on most architectures creates W+X page
+ * mappings which won't be cleaned up until do_free_init() runs. Any
+ * code such as mark_rodata_ro() which depends on those mappings to
+ * be cleaned up needs to sync with the queued work - ie
+ * rcu_barrier_sched()
*/
call_rcu_sched(&freeinit->rcu, do_free_init);
mutex_unlock(&module_mutex);
--
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.
On 04/30/2018 08:59 AM, 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 <[email protected]>
> Reported-by: Jan Glauber <[email protected]>
> Fixes: e1a58320a38d ("x86/mm: Warn on W^X mappings")
> Signed-off-by: Jeffrey Hugo <[email protected]>
> Acked-by: Kees Cook <[email protected]>
> Acked-by: Ingo Molnar <[email protected]>
> Acked-by: Will Deacon <[email protected]>
> ---
>
Acked-by: Laura Abbott <[email protected]>
If you don't have a tree for this to go through, I might suggest having
Kees take it.
> v3:
> -added comment to module code to establish matching connection
> -added acks by Kees, Ingo, and Will
>
> v2:
> -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 +++++++
> kernel/module.c | 5 +++++
> 2 files changed, 12 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
> diff --git a/kernel/module.c b/kernel/module.c
> index a6e43a5..7b71c3c 100644
> --- a/kernel/module.c
> +++ b/kernel/module.c
> @@ -3516,6 +3516,11 @@ static noinline int do_init_module(struct module *mod)
> * walking this with preempt disabled. In all the failure paths, we
> * call synchronize_sched(), but we don't want to slow down the success
> * path, so use actual RCU here.
> + * Note that module_alloc() on most architectures creates W+X page
> + * mappings which won't be cleaned up until do_free_init() runs. Any
> + * code such as mark_rodata_ro() which depends on those mappings to
> + * be cleaned up needs to sync with the queued work - ie
> + * rcu_barrier_sched()
> */
> call_rcu_sched(&freeinit->rcu, do_free_init);
> mutex_unlock(&module_mutex);
>
On Mon, Apr 30, 2018 at 10:19 AM, Laura Abbott <[email protected]> wrote:
> On 04/30/2018 08:59 AM, 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 <[email protected]>
>> Reported-by: Jan Glauber <[email protected]>
>> Fixes: e1a58320a38d ("x86/mm: Warn on W^X mappings")
>> Signed-off-by: Jeffrey Hugo <[email protected]>
>> Acked-by: Kees Cook <[email protected]>
>> Acked-by: Ingo Molnar <[email protected]>
>> Acked-by: Will Deacon <[email protected]>
>> ---
>>
>
> Acked-by: Laura Abbott <[email protected]>
>
> If you don't have a tree for this to go through, I might suggest having
> Kees take it.
akpm has taken the W^X stuff in the past, but I'm happy to do so. Just
let me know either way. :)
-Kees
--
Kees Cook
Pixel Security
On 4/30/2018 12:40 PM, Kees Cook wrote:
> On Mon, Apr 30, 2018 at 10:19 AM, Laura Abbott <[email protected]> wrote:
>> On 04/30/2018 08:59 AM, 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 <[email protected]>
>>> Reported-by: Jan Glauber <[email protected]>
>>> Fixes: e1a58320a38d ("x86/mm: Warn on W^X mappings")
>>> Signed-off-by: Jeffrey Hugo <[email protected]>
>>> Acked-by: Kees Cook <[email protected]>
>>> Acked-by: Ingo Molnar <[email protected]>
>>> Acked-by: Will Deacon <[email protected]>
>>> ---
>>>
>>
>> Acked-by: Laura Abbott <[email protected]>
>>
>> If you don't have a tree for this to go through, I might suggest having
>> Kees take it.
>
> akpm has taken the W^X stuff in the past, but I'm happy to do so. Just
> let me know either way. :)
>
> -Kees
>
That sounds fine to me. Is that agreeable to you, Andrew?
--
Jeffrey Hugo
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.