ACK/cmnt: [PATCH] init: fix false positives in W+X checking

[Manoj Iyer]

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.

>  -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
============================