ACK/cmnt: [PATCH] init: fix false positives in W+X checking
dann frazier
dann.frazier at canonical.com
Tue May 15 13:43:32 UTC 2018
On Fri, May 11, 2018 at 9:08 AM, Manoj Iyer <manoj.iyer at canonical.com> wrote:
> On Thu, 10 May 2018, dann frazier wrote:
>
>> On Wed, May 9, 2018 at 2:32 PM, Manoj Iyer <manoj.iyer at canonical.com>
>> wrote:
>>>
>>> On Wed, 9 May 2018, Joseph Salisbury wrote:
>>>
>>>> On 05/08/2018 12:24 PM, Manoj Iyer wrote:
>>>>>
>>>>>
>>>>> From: Jeffrey Hugo <jhugo at codeaurora.org>
>>>>>
>>>>> 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().
>>>>>
>>>>> BugLink: https://launchpad.net/bugs/1769696
>>>>>
>>>>> Link:
>>>>>
>>>>> http://lkml.kernel.org/r/1525103946-29526-1-git-send-email-jhugo@codeaurora.org
>>>>> Fixes: e1a58320a38d ("x86/mm: Warn on W^X mappings")
>>>>> Signed-off-by: Jeffrey Hugo <jhugo at codeaurora.org>
>>>>> Reported-by: Timur Tabi <timur at codeaurora.org>
>>>>> Reported-by: Jan Glauber <jan.glauber at caviumnetworks.com>
>>>>> Acked-by: Kees Cook <keescook at chromium.org>
>>>>> Acked-by: Ingo Molnar <mingo at kernel.org>
>>>>> Acked-by: Will Deacon <will.deacon at arm.com>
>>>>> Acked-by: Laura Abbott <labbott at redhat.com>
>>>>> Cc: Mark Rutland <mark.rutland at arm.com>
>>>>> Cc: Ard Biesheuvel <ard.biesheuvel at linaro.org>
>>>>> Cc: Catalin Marinas <catalin.marinas at arm.com>
>>>>> Cc: Stephen Smalley <sds at tycho.nsa.gov>
>>>>> Cc: Thomas Gleixner <tglx at linutronix.de>
>>>>> Cc: Peter Zijlstra <peterz at infradead.org>
>>>>> Signed-off-by: Andrew Morton <akpm at linux-foundation.org>
>>>>> Signed-off-by: Stephen Rothwell <sfr at canb.auug.org.au>
>>>>> (cherry picked from commit 65d313ee1a7d41611b8ee6063db53bc976db5ba2
>>>>> linux-next)
>>>>> Signed-off-by: Manoj Iyer <manoj.iyer at canonical.com>
>>>>> ---
>>>>> init/main.c | 7 +++++++
>>>>> kernel/module.c | 5 +++++
>>>>> 2 files changed, 12 insertions(+)
>>>>>
>>>>> diff --git a/init/main.c b/init/main.c
>>>>> index b8b121c17ff1..44f88af9b191 100644
>>>>> --- a/init/main.c
>>>>> +++ b/init/main.c
>>>>> @@ -980,6 +980,13 @@ __setup("rodata=", set_debug_rodata);
>>>>> 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 2612f760df84..0da7f3468350 100644
>>>>> --- a/kernel/module.c
>>>>> +++ b/kernel/module.c
>>>>> @@ -3517,6 +3517,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);
>>>>
>>>>
>>>> Hi Manoj,
>>>>
>>>> The patch says that it fixes e1a58320a38d. This commit is in mainline
>>>> as of v4.4-rc1. However, this SRU request is only for Artful and
>>>> Bionic. You may also want to investigate to see if it's needed in
>>>> Xenial. If it is, the patch you submitted does not apply to Xenial and
>>>> you would need to submit a separate patch/SRU request that is specific
>>>> to Xenial.
>>>
>>>
>>>
>>> That is correct. At this time we are only interested in fixing it in
>>> Artful
>>> (Xenial linux-hwe) and Bionic, and apply to Cosmic (if applicable). The
>>> platforms we are interested in were certified with Xenial and linux-hwe.
>>
>>
>> Manoj,
>>
>> ThunderX CRBs were certified with xenial GA - we need to fix 4.4 too,
>> if applicable.
>> (Good catch Joseph).
>>
>
> ouch.. Joe, will send you a backport to xenial soon.
btw, this is now in Linus' tree:
commit ae646f0b9ca135b87bc73ff606ef996c3029780a
Author: Jeffrey Hugo <jhugo at codeaurora.org>
Date: Fri May 11 16:01:42 2018 -0700
init: fix false positives in W+X checking
I suggest cherry-picking that one instead for the various releases so
that we're referencing the upstream commit hash.
-dann
>
>> -dann
>>
>>>>
>>>> For A and B, this patch applies and builds cleanly. It fixes a specific
>>>> bug, so:
>>>>
>>>> Acked-by: Joseph Salisbury <joseph.salisbury at canonical.com>
>>>>
>>>>
>>>>
>>>
>>> --
>>> ============================
>>> Manoj Iyer
>>> Ubuntu/Canonical
>>> ARM Servers - Cloud
>>> ============================
>>> --
>>> kernel-team mailing list
>>> kernel-team at lists.ubuntu.com
>>> https://lists.ubuntu.com/mailman/listinfo/kernel-team
>>>
>>
>>
>
> --
> ============================
> Manoj Iyer
> Ubuntu/Canonical
> ARM Servers - Cloud
> ============================
More information about the kernel-team
mailing list