From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-11.8 required=3.0 tests=DKIMWL_WL_HIGH,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS,HTML_MESSAGE, INCLUDES_PATCH,MAILING_LIST_MULTI,MENTIONS_GIT_HOSTING,SIGNED_OFF_BY, SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 6FF6DC432C0 for ; Tue, 19 Nov 2019 21:52:21 +0000 (UTC) Received: from mother.openwall.net (mother.openwall.net [195.42.179.200]) by mail.kernel.org (Postfix) with SMTP id 629CE2244A for ; Tue, 19 Nov 2019 21:52:19 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (1024-bit key) header.d=digitalocean.com header.i=@digitalocean.com header.b="KcX5BxpB" DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 629CE2244A Authentication-Results: mail.kernel.org; dmarc=fail (p=reject dis=none) header.from=digitalocean.com Authentication-Results: mail.kernel.org; spf=pass smtp.mailfrom=kernel-hardening-return-17401-kernel-hardening=archiver.kernel.org@lists.openwall.com Received: (qmail 21753 invoked by uid 550); 19 Nov 2019 21:52:10 -0000 Mailing-List: contact kernel-hardening-help@lists.openwall.com; run by ezmlm Precedence: bulk List-Post: List-Help: List-Unsubscribe: List-Subscribe: List-ID: Received: (qmail 21730 invoked from network); 19 Nov 2019 21:52:08 -0000 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=digitalocean.com; s=google; h=from:message-id:mime-version:subject:date:in-reply-to:cc:to :references; bh=U+SbCq3ocNyIo8FGHGBsSRQOyqzMfc7Ayipxb/773i8=; b=KcX5BxpBvCiVa5obG46C4S3He8tf3OTP3Bosy1rZ40/JcilaiK1Qp7vtJFf8FecVhj FkGDrmQhiII+OxUHVxYjmq9y0NdMZfJyYFRgUqcJ+81xRnzeoqzPQYjcPjVGG2Bb7GP/ 6Lk/SophoJYhXwO+VaBH5Lh8emWMoPzR06Cw0= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:from:message-id:mime-version:subject:date :in-reply-to:cc:to:references; bh=U+SbCq3ocNyIo8FGHGBsSRQOyqzMfc7Ayipxb/773i8=; b=DxPzWRoSGV2EUniBxs1EZswl0A3IYXCVnBsZLojjsjkH0iBBNPODzHJzbrqNPsxQHs mWEZx7WqDw5LDTmp5k28npdzvvfvttaCO9bGki0D2B0sV5rl2HuGY5ioQYZmpvlJeDl9 3K53AZQPUqTZ5vHllsFCeyvPhHSlhePEgOs1NxpJk9mnoKYM/0GlsgPTsWx4jOrOAeSl IG3id2R1XIFjudYFMQlOiuStucLAb+d//Vzqu0ET5JCwDA00vGgnfkQqFOokb/Jyu1Xj VCCa8MIcTu2044ckE13RyTpfF+lT1vTj4mNIiHTHLs1OCBXIH6rPgLIkE6CRQEVXngmm r1bg== X-Gm-Message-State: APjAAAXYiGl/oOj0yPdKazuCLvJbY+8uxtXlyCXbkau/WjWOa164B1JI KvxYMazePkty8c9wIl2jnjxw X-Google-Smtp-Source: APXvYqwbMonlNXEBTlC6fuNAVLqY1dMdsThFjtpsB2CKiUHG/J/14xV5j3VhfSfRlbbsM4wAchm2Lg== X-Received: by 2002:a17:902:a516:: with SMTP id s22mr29120783plq.295.1574200316220; Tue, 19 Nov 2019 13:51:56 -0800 (PST) From: Tianlin Li Message-Id: <180F067D-0E86-479C-942A-E1E9118D3431@digitalocean.com> Content-Type: multipart/alternative; boundary="Apple-Mail=_96CC2F9E-A2F9-4977-908B-8F533F03F8D0" Mime-Version: 1.0 (Mac OS X Mail 12.4 \(3445.104.11\)) Subject: Re: [RFC PATCH] kernel/module: have the callers of set_memory_*() check the return value Date: Tue, 19 Nov 2019 15:51:52 -0600 In-Reply-To: <201911190849.131691D@keescook> Cc: kernel-hardening@lists.openwall.com, Steven Rostedt , Ingo Molnar , Russell King , Catalin Marinas , Will Deacon , Greentime Hu , Vincent Chen , Thomas Gleixner , Borislav Petkov , "H . Peter Anvin" , x86@kernel.org, Jessica Yu , Josh Poimboeuf , Jiri Kosina , Miroslav Benes , Petr Mladek , Joe Lawrence To: Kees Cook References: <20191119155149.20396-1-tli@digitalocean.com> <201911190849.131691D@keescook> X-Mailer: Apple Mail (2.3445.104.11) --Apple-Mail=_96CC2F9E-A2F9-4977-908B-8F533F03F8D0 Content-Transfer-Encoding: quoted-printable Content-Type: text/plain; charset=utf-8 > On Nov 19, 2019, at 11:07 AM, Kees Cook wrote: >=20 > On Tue, Nov 19, 2019 at 09:51:49AM -0600, Tianlin Li wrote: >> Right now several architectures allow their set_memory_*() family of=20= >> functions to fail, but callers may not be checking the return values. = We=20 >> need to fix the callers and add the __must_check attribute. They also = may >> not provide any level of atomicity, in the sense that the memory=20 >> protections may be left incomplete on failure. This issue likely has = a few=20 >> steps on effects architectures[1]: >=20 > Awesome; thanks for working on this! >=20 > A few suggestions on this patch, which will help reviewers, below... >=20 >> 1)Have all callers of set_memory_*() helpers check the return value. >> 2)Add __much_check to all set_memory_*() helpers so that new uses do = not=20 >> ignore the return value. >> 3)Add atomicity to the calls so that the memory protections aren't = left in=20 >> a partial state. >=20 > Maybe clarify to say something like "this series is step 1=E2=80=9D? ok. Will add the clarification. Thanks! >=20 >>=20 >> Ideally, the failure of set_memory_*() should be passed up the call = stack,=20 >> and callers should examine the failure and deal with it. But = currently,=20 >> some callers just have void return type. >>=20 >> We need to fix the callers to handle the return all the way to the = top of=20 >> stack, and it will require a large series of patches to finish all = the three=20 >> steps mentioned above. I start with kernel/module, and will move onto = other=20 >> subsystems. I am not entirely sure about the failure modes for each = caller.=20 >> So I would like to get some comments before I move forward. This = single=20 >> patch is just for fixing the return value of set_memory_*() function = in=20 >> kernel/module, and also the related callers. Any feedback would be = greatly=20 >> appreciated. >>=20 >> [1]:https://github.com/KSPP/linux/issues/7 >>=20 >> Signed-off-by: Tianlin Li >> --- >> arch/arm/kernel/ftrace.c | 8 +- >> arch/arm64/kernel/ftrace.c | 6 +- >> arch/nds32/kernel/ftrace.c | 6 +- >> arch/x86/kernel/ftrace.c | 13 ++- >> include/linux/module.h | 16 ++-- >> kernel/livepatch/core.c | 15 +++- >> kernel/module.c | 170 = +++++++++++++++++++++++++++---------- >> kernel/trace/ftrace.c | 15 +++- >> 8 files changed, 175 insertions(+), 74 deletions(-) >=20 > - Can you break this patch into 3 separate patches, by "subsystem": > - ftrace > - module > - livepatch (unless Jessica thinks maybe livepatch should go > with module?) > - Please run patches through scripts/checkpatch.pl to catch common > coding style issues (e.g. blank line between variable declarations > and function body). > - These changes look pretty straight forward, so I think it'd be fine > to drop the "RFC" tag and include linux-kernel@vger.kernel.org = in the > Cc list for the v2. For lots of detail, see: > = https://www.kernel.org/doc/html/latest/process/submitting-patches.html = ok. I will fix them in v2. Thanks a lot!=20 > More below... >=20 >>=20 >> diff --git a/arch/arm/kernel/ftrace.c b/arch/arm/kernel/ftrace.c >> index bda949fd84e8..7ea1338821d6 100644 >> --- a/arch/arm/kernel/ftrace.c >> +++ b/arch/arm/kernel/ftrace.c >> @@ -59,13 +59,15 @@ static unsigned long adjust_address(struct = dyn_ftrace *rec, unsigned long addr) >>=20 >> int ftrace_arch_code_modify_prepare(void) >> { >> - set_all_modules_text_rw(); >> - return 0; >> + return set_all_modules_text_rw(); >> } >>=20 >> int ftrace_arch_code_modify_post_process(void) >> { >> - set_all_modules_text_ro(); >> + int ret; >=20 > Blank line here... >=20 >> + ret =3D set_all_modules_text_ro(); >> + if (ret) >> + return ret; >> /* Make sure any TLB misses during machine stop are cleared. */ >> flush_tlb_all(); >> return 0; >=20 > Are callers of these ftrace functions checking return values too? ftrace_arch_code_modify_prepare is called in ftrace_run_update_code and = ftrace_module_enable.=20 ftrace_run_update_code is checking the return value of = ftrace_arch_code_modify_prepare already.=20 ftrace_module_enable didn=E2=80=99t check. (I modifies this function in = this patch to check the return value of ftrace_arch_code_modify_prepare = ).=20 Same for ftrace_arch_code_modify_post_process.=20 >> diff --git a/arch/arm64/kernel/ftrace.c b/arch/arm64/kernel/ftrace.c >> index 171773257974..97a89c38f6b9 100644 >> --- a/arch/arm64/kernel/ftrace.c >> +++ b/arch/arm64/kernel/ftrace.c >> @@ -115,9 +115,11 @@ int ftrace_make_call(struct dyn_ftrace *rec, = unsigned long addr) >> } >>=20 >> /* point the trampoline to our ftrace entry = point */ >> - module_disable_ro(mod); >> + if (module_disable_ro(mod)) >> + return -EINVAL; >> *dst =3D trampoline; >> - module_enable_ro(mod, true); >> + if (module_enable_ro(mod, true)) >> + return -EINVAL; >>=20 >> /* >> * Ensure updated trampoline is visible to = instruction >> diff --git a/arch/nds32/kernel/ftrace.c b/arch/nds32/kernel/ftrace.c >> index fd2a54b8cd57..e9e63e703a3e 100644 >> --- a/arch/nds32/kernel/ftrace.c >> +++ b/arch/nds32/kernel/ftrace.c >> @@ -91,14 +91,12 @@ int __init ftrace_dyn_arch_init(void) >>=20 >> int ftrace_arch_code_modify_prepare(void) >> { >> - set_all_modules_text_rw(); >> - return 0; >> + return set_all_modules_text_rw(); >> } >>=20 >> int ftrace_arch_code_modify_post_process(void) >> { >> - set_all_modules_text_ro(); >> - return 0; >> + return set_all_modules_text_ro(); >> } >>=20 >> static unsigned long gen_sethi_insn(unsigned long addr) >> diff --git a/arch/x86/kernel/ftrace.c b/arch/x86/kernel/ftrace.c >> index 024c3053dbba..7fdee06e2a76 100644 >> --- a/arch/x86/kernel/ftrace.c >> +++ b/arch/x86/kernel/ftrace.c >> @@ -42,19 +42,26 @@ int ftrace_arch_code_modify_prepare(void) >> * and live kernel patching from changing the text permissions = while >> * ftrace has it set to "read/write". >> */ >> + int ret; >> mutex_lock(&text_mutex); >> set_kernel_text_rw(); >> - set_all_modules_text_rw(); >> + ret =3D set_all_modules_text_rw(); >> + if (ret) { >> + set_kernel_text_ro(); >=20 > Is the set_kernel_text_ro() here is an atomicity fix? Also, won't a = future > patch need to check the result of that function call? (i.e. should it > be left out of this series and should atomicity fixes happen inside = the > set_memory_*() functions? Can they happen there?) set_kernel_text_tro() here was not for an atomicity fix in step 3, it = was just for reverting the status=20 in ftrace_arch_code_modify_prepare, because set_kernel_text_rw() is = called.=20 Thanks for pointing it out. I will check it.=20 >=20 >> + mutex_unlock(&text_mutex); >> + return ret; >> + } >> return 0; >> } >>=20 >> int ftrace_arch_code_modify_post_process(void) >> __releases(&text_mutex) >> { >> - set_all_modules_text_ro(); >> + int ret; >=20 > blank line needed... >=20 >> + ret =3D set_all_modules_text_ro(); >> set_kernel_text_ro(); >> mutex_unlock(&text_mutex); >> - return 0; >> + return ret; >> } >>=20 >> union ftrace_code_union { >> diff --git a/include/linux/module.h b/include/linux/module.h >> index 1455812dd325..e6c7f3b719a3 100644 >> --- a/include/linux/module.h >> +++ b/include/linux/module.h >> @@ -847,15 +847,15 @@ extern int module_sysfs_initialized; >> #define __MODULE_STRING(x) __stringify(x) >>=20 >> #ifdef CONFIG_STRICT_MODULE_RWX >> -extern void set_all_modules_text_rw(void); >> -extern void set_all_modules_text_ro(void); >> -extern void module_enable_ro(const struct module *mod, bool = after_init); >> -extern void module_disable_ro(const struct module *mod); >> +extern int set_all_modules_text_rw(void); >> +extern int set_all_modules_text_ro(void); >> +extern int module_enable_ro(const struct module *mod, bool = after_init); >> +extern int module_disable_ro(const struct module *mod); >> #else >> -static inline void set_all_modules_text_rw(void) { } >> -static inline void set_all_modules_text_ro(void) { } >> -static inline void module_enable_ro(const struct module *mod, bool = after_init) { } >> -static inline void module_disable_ro(const struct module *mod) { } >> +static inline int set_all_modules_text_rw(void) { return 0; } >> +static inline int set_all_modules_text_ro(void) { return 0; } >> +static inline int module_enable_ro(const struct module *mod, bool = after_init) { return 0; } >=20 > Please wrap this line (yes, the old one wasn't...) >=20 >> +static inline int module_disable_ro(const struct module *mod) { = return 0; } >> #endif >>=20 >> #ifdef CONFIG_GENERIC_BUG >> diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c >> index c4ce08f43bd6..39bfc0685854 100644 >> --- a/kernel/livepatch/core.c >> +++ b/kernel/livepatch/core.c >> @@ -721,16 +721,25 @@ static int klp_init_object_loaded(struct = klp_patch *patch, >>=20 >> mutex_lock(&text_mutex); >>=20 >> - module_disable_ro(patch->mod); >> + ret =3D module_disable_ro(patch->mod); >> + if (ret) { >> + mutex_unlock(&text_mutex); >> + return ret; >> + } >> ret =3D klp_write_object_relocations(patch->mod, obj); >> if (ret) { >> - module_enable_ro(patch->mod, true); >> + if (module_enable_ro(patch->mod, true)); >> + pr_err("module_enable_ro failed.\n"); >=20 > Why the pr_err() here and not in other failure cases? (Also, should it > instead be a WARN_ONCE() inside the set_memory_*() functions instead?) Because module_enable_ro() is called in the checking of another return = value. I can not return the value, so I print the error.=20 Similar to the previous question, should it be fixed by atomicity = patches later?=20 >=20 >> mutex_unlock(&text_mutex); >> return ret; >> } >>=20 >> arch_klp_init_object_loaded(patch, obj); >> - module_enable_ro(patch->mod, true); >> + ret =3D module_enable_ro(patch->mod, true); >> + if (ret) { >> + mutex_unlock(&text_mutex); >> + return ret; >> + } >>=20 >> mutex_unlock(&text_mutex); >>=20 >> diff --git a/kernel/module.c b/kernel/module.c >> index 9ee93421269c..87125b5e315c 100644 >> --- a/kernel/module.c >> +++ b/kernel/module.c >> @@ -1900,111 +1900,162 @@ static void mod_sysfs_teardown(struct = module *mod) >> * >> * These values are always page-aligned (as is base) >> */ >> -static void frob_text(const struct module_layout *layout, >> +static int frob_text(const struct module_layout *layout, >> int (*set_memory)(unsigned long start, int = num_pages)) >> { >> BUG_ON((unsigned long)layout->base & (PAGE_SIZE-1)); >> BUG_ON((unsigned long)layout->text_size & (PAGE_SIZE-1)); >> - set_memory((unsigned long)layout->base, >> + return set_memory((unsigned long)layout->base, >> layout->text_size >> PAGE_SHIFT); >> } >>=20 >> #ifdef CONFIG_STRICT_MODULE_RWX >> -static void frob_rodata(const struct module_layout *layout, >> +static int frob_rodata(const struct module_layout *layout, >> int (*set_memory)(unsigned long start, int = num_pages)) >> { >> BUG_ON((unsigned long)layout->base & (PAGE_SIZE-1)); >> BUG_ON((unsigned long)layout->text_size & (PAGE_SIZE-1)); >> BUG_ON((unsigned long)layout->ro_size & (PAGE_SIZE-1)); >> - set_memory((unsigned long)layout->base + layout->text_size, >> + return set_memory((unsigned long)layout->base + = layout->text_size, >> (layout->ro_size - layout->text_size) >> PAGE_SHIFT); >> } >>=20 >> -static void frob_ro_after_init(const struct module_layout *layout, >> +static int frob_ro_after_init(const struct module_layout *layout, >> int (*set_memory)(unsigned long start, = int num_pages)) >> { >> BUG_ON((unsigned long)layout->base & (PAGE_SIZE-1)); >> BUG_ON((unsigned long)layout->ro_size & (PAGE_SIZE-1)); >> BUG_ON((unsigned long)layout->ro_after_init_size & = (PAGE_SIZE-1)); >> - set_memory((unsigned long)layout->base + layout->ro_size, >> + return set_memory((unsigned long)layout->base + layout->ro_size, >> (layout->ro_after_init_size - layout->ro_size) >> = PAGE_SHIFT); >> } >>=20 >> -static void frob_writable_data(const struct module_layout *layout, >> +static int frob_writable_data(const struct module_layout *layout, >> int (*set_memory)(unsigned long start, = int num_pages)) >> { >> BUG_ON((unsigned long)layout->base & (PAGE_SIZE-1)); >> BUG_ON((unsigned long)layout->ro_after_init_size & = (PAGE_SIZE-1)); >> BUG_ON((unsigned long)layout->size & (PAGE_SIZE-1)); >> - set_memory((unsigned long)layout->base + = layout->ro_after_init_size, >> + return set_memory((unsigned long)layout->base + = layout->ro_after_init_size, >> (layout->size - layout->ro_after_init_size) >> = PAGE_SHIFT); >> } >>=20 >> /* livepatching wants to disable read-only so it can frob module. */ >> -void module_disable_ro(const struct module *mod) >> +int module_disable_ro(const struct module *mod) >> { >> + int ret; >> if (!rodata_enabled) >> - return; >> + return 0; >>=20 >> - frob_text(&mod->core_layout, set_memory_rw); >> - frob_rodata(&mod->core_layout, set_memory_rw); >> - frob_ro_after_init(&mod->core_layout, set_memory_rw); >> - frob_text(&mod->init_layout, set_memory_rw); >> - frob_rodata(&mod->init_layout, set_memory_rw); >> + ret =3D frob_text(&mod->core_layout, set_memory_rw); >> + if (ret) >> + return ret; >> + ret =3D frob_rodata(&mod->core_layout, set_memory_rw); >> + if (ret) >> + return ret; >> + ret =3D frob_ro_after_init(&mod->core_layout, set_memory_rw); >> + if (ret) >> + return ret; >> + ret =3D frob_text(&mod->init_layout, set_memory_rw); >> + if (ret) >> + return ret; >> + ret =3D frob_rodata(&mod->init_layout, set_memory_rw); >> + if (ret) >> + return ret; >> + >> + return 0; >> } >>=20 >> -void module_enable_ro(const struct module *mod, bool after_init) >> +int module_enable_ro(const struct module *mod, bool after_init) >> { >> + int ret; >> if (!rodata_enabled) >> - return; >> + return 0; >>=20 >> set_vm_flush_reset_perms(mod->core_layout.base); >> set_vm_flush_reset_perms(mod->init_layout.base); >> - frob_text(&mod->core_layout, set_memory_ro); >> + ret =3D frob_text(&mod->core_layout, set_memory_ro); >> + if (ret) >> + return ret; >>=20 >> - frob_rodata(&mod->core_layout, set_memory_ro); >> - frob_text(&mod->init_layout, set_memory_ro); >> - frob_rodata(&mod->init_layout, set_memory_ro); >> + ret =3D frob_rodata(&mod->core_layout, set_memory_ro); >> + if (ret) >> + return ret; >> + ret =3D frob_text(&mod->init_layout, set_memory_ro); >> + if (ret) >> + return ret; >> + ret =3D frob_rodata(&mod->init_layout, set_memory_ro); >> + if (ret) >> + return ret; >>=20 >> - if (after_init) >> - frob_ro_after_init(&mod->core_layout, set_memory_ro); >> + if (after_init) { >> + ret =3D frob_ro_after_init(&mod->core_layout, = set_memory_ro); >> + if (ret) >> + return ret; >> + } >> + return 0; >> } >>=20 >> -static void module_enable_nx(const struct module *mod) >> +static int module_enable_nx(const struct module *mod) >> { >> - frob_rodata(&mod->core_layout, set_memory_nx); >> - frob_ro_after_init(&mod->core_layout, set_memory_nx); >> - frob_writable_data(&mod->core_layout, set_memory_nx); >> - frob_rodata(&mod->init_layout, set_memory_nx); >> - frob_writable_data(&mod->init_layout, set_memory_nx); >> + int ret; >> + >> + ret =3D frob_rodata(&mod->core_layout, set_memory_nx); >> + if (ret) >> + return ret; >> + ret =3D frob_ro_after_init(&mod->core_layout, set_memory_nx); >> + if (ret) >> + return ret; >> + ret =3D frob_writable_data(&mod->core_layout, set_memory_nx); >> + if (ret) >> + return ret; >> + ret =3D frob_rodata(&mod->init_layout, set_memory_nx); >> + if (ret) >> + return ret; >> + ret =3D frob_writable_data(&mod->init_layout, set_memory_nx); >> + if (ret) >> + return ret; >> + >> + return 0; >> } >>=20 >> /* Iterate through all modules and set each module's text as RW */ >> -void set_all_modules_text_rw(void) >> +int set_all_modules_text_rw(void) >> { >> struct module *mod; >> + int ret; >>=20 >> if (!rodata_enabled) >> - return; >> + return 0; >>=20 >> mutex_lock(&module_mutex); >> list_for_each_entry_rcu(mod, &modules, list) { >> if (mod->state =3D=3D MODULE_STATE_UNFORMED) >> continue; >>=20 >> - frob_text(&mod->core_layout, set_memory_rw); >> - frob_text(&mod->init_layout, set_memory_rw); >> + ret =3D frob_text(&mod->core_layout, set_memory_rw); >> + if (ret) { >> + mutex_unlock(&module_mutex); >> + return ret; >> + } >> + ret =3D frob_text(&mod->init_layout, set_memory_rw); >> + if (ret) { >> + mutex_unlock(&module_mutex); >> + return ret; >> + } >=20 > This pattern feels like it might be better with a "goto out" style: >=20 > mutex_lock... > list_for_each... { > ret =3D frob_... > if (ret) > goto out; > ret =3D frob_... > if (ret) > goto out; > } > ret =3D 0; > out: > mutex_unlock... > return ret; Thanks! Will fix it in v2.=20 >=20 >> } >> mutex_unlock(&module_mutex); >> + return 0; >> } >>=20 >> /* Iterate through all modules and set each module's text as RO */ >> -void set_all_modules_text_ro(void) >> +int set_all_modules_text_ro(void) >> { >> struct module *mod; >> + int ret; >>=20 >> if (!rodata_enabled) >> - return; >> + return 0; >>=20 >> mutex_lock(&module_mutex); >> list_for_each_entry_rcu(mod, &modules, list) { >> @@ -2017,22 +2068,37 @@ void set_all_modules_text_ro(void) >> mod->state =3D=3D MODULE_STATE_GOING) >> continue; >>=20 >> - frob_text(&mod->core_layout, set_memory_ro); >> - frob_text(&mod->init_layout, set_memory_ro); >> + ret =3D frob_text(&mod->core_layout, set_memory_ro); >> + if (ret) { >> + mutex_unlock(&module_mutex); >> + return ret; >> + } >> + ret =3D frob_text(&mod->init_layout, set_memory_ro); >> + if (ret) { >> + mutex_unlock(&module_mutex); >> + return ret; >> + } >> } >> mutex_unlock(&module_mutex); >> + return 0; >> } >> #else /* !CONFIG_STRICT_MODULE_RWX */ >> -static void module_enable_nx(const struct module *mod) { } >> +static int module_enable_nx(const struct module *mod) { return 0; } >> #endif /* CONFIG_STRICT_MODULE_RWX */ >> -static void module_enable_x(const struct module *mod) >> +static int module_enable_x(const struct module *mod) >> { >> - frob_text(&mod->core_layout, set_memory_x); >> - frob_text(&mod->init_layout, set_memory_x); >> + int ret; >> + ret =3D frob_text(&mod->core_layout, set_memory_x); >> + if (ret) >> + return ret; >> + ret =3D frob_text(&mod->init_layout, set_memory_x); >> + if (ret) >> + return ret; >> + return 0; >> } >> #else /* !CONFIG_ARCH_HAS_STRICT_MODULE_RWX */ >> -static void module_enable_nx(const struct module *mod) { } >> -static void module_enable_x(const struct module *mod) { } >> +static int module_enable_nx(const struct module *mod) { return 0; } >> +static int module_enable_x(const struct module *mod) { return 0; } >> #endif /* CONFIG_ARCH_HAS_STRICT_MODULE_RWX */ >>=20 >>=20 >> @@ -3534,7 +3600,11 @@ static noinline int do_init_module(struct = module *mod) >> /* Switch to core kallsyms now init is done: kallsyms may be = walking! */ >> rcu_assign_pointer(mod->kallsyms, &mod->core_kallsyms); >> #endif >> - module_enable_ro(mod, true); >> + ret =3D module_enable_ro(mod, true); >> + if (ret) { >> + mutex_unlock(&module_mutex); >> + goto fail_free_freeinit; >> + } >> mod_tree_remove_init(mod); >> module_arch_freeing_init(mod); >> mod->init_layout.base =3D NULL; >> @@ -3640,9 +3710,15 @@ static int complete_formation(struct module = *mod, struct load_info *info) >> /* This relies on module_mutex for list integrity. */ >> module_bug_finalize(info->hdr, info->sechdrs, mod); >>=20 >> - module_enable_ro(mod, false); >> - module_enable_nx(mod); >> - module_enable_x(mod); >> + err =3D module_enable_ro(mod, false); >> + if (err) >> + goto out; >> + err =3D module_enable_nx(mod); >> + if (err) >> + goto out; >> + err =3D module_enable_x(mod); >> + if (err) >> + goto out; >>=20 >> /* Mark state as coming so strong_try_module_get() ignores us, >> * but kallsyms etc. can see us. */ >> diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c >> index f9821a3374e9..d4532bb65d1b 100644 >> --- a/kernel/trace/ftrace.c >> +++ b/kernel/trace/ftrace.c >> @@ -5814,8 +5814,13 @@ void ftrace_module_enable(struct module *mod) >> * text to read-only, as we now need to set it back to = read-write >> * so that we can modify the text. >> */ >> - if (ftrace_start_up) >> - ftrace_arch_code_modify_prepare(); >> + if (ftrace_start_up) { >> + int ret =3D ftrace_arch_code_modify_prepare(); >> + if (ret) { >> + FTRACE_WARN_ON(ret); >> + goto out_unlock; >> + } >=20 > Can FTRACE_WARN_ON() be used in an "if" like WARN_ON? This could maybe > look like: >=20 > ret =3D ftrace_arch... > if (FTRACE_WARN_ON(ret)) > goto out_unlock >=20 > Though I not this doesn't result in an error getting passed up? i.e. > ftrace_module_enable() is still "void". Does that matter here? Thanks. I will fix it in v2.=20 >> + } >>=20 >> do_for_each_ftrace_rec(pg, rec) { >> int cnt; >> @@ -5854,8 +5859,10 @@ void ftrace_module_enable(struct module *mod) >> } while_for_each_ftrace_rec(); >>=20 >> out_loop: >> - if (ftrace_start_up) >> - ftrace_arch_code_modify_post_process(); >> + if (ftrace_start_up) { >> + int ret =3D ftrace_arch_code_modify_post_process(); >> + FTRACE_WARN_ON(ret); >> + } >>=20 >> out_unlock: >> mutex_unlock(&ftrace_lock); >> --=20 >> 2.17.1 >>=20 >=20 > Thanks, Thanks for all detail suggestions!=20 > --=20 > Kees Cook --Apple-Mail=_96CC2F9E-A2F9-4977-908B-8F533F03F8D0 Content-Transfer-Encoding: quoted-printable Content-Type: text/html; charset=utf-8
On = Nov 19, 2019, at 11:07 AM, Kees Cook <keescook@chromium.org> wrote:

On Tue, Nov 19, 2019 at = 09:51:49AM -0600, Tianlin Li wrote:
Right now several architectures allow = their set_memory_*() family of 
functions to = fail, but callers may not be checking the return values. We 
need to fix = the callers and add the __must_check attribute. They also may
not provide any level of atomicity, in the sense that the = memory 
protections may be left incomplete on failure. This issue = likely has a few 
steps on effects architectures[1]:

Awesome; thanks for working on this!

A few suggestions on this patch, = which will help reviewers, below...

1)Have all callers of set_memory_*() = helpers check the return value.
2)Add __much_check to all = set_memory_*() helpers so that new uses do not 
ignore the = return value.
3)Add atomicity to the calls so that the = memory protections aren't left in 
a partial = state.

Maybe clarify to say something like "this series is step = 1=E2=80=9D?
ok. Will add the = clarification. Thanks!


Ideally, the failure of set_memory_*() should be passed up = the call stack, 
and callers should examine the failure and deal with it. But = currently, 
some callers just have void return type.

We need to fix the callers to handle the return all the way = to the top of 
stack, and it will require a large series of patches to = finish all the three steps mentioned above. I start with kernel/module, and will = move onto other 
subsystems. I am not entirely sure about the failure modes = for each caller. 
So I would like to get some comments before I move forward. = This single 
patch is just for fixing the return value of set_memory_*() = function in 
kernel/module, and also the related callers. Any feedback = would be greatly 
appreciated.

[1]:https://github.com/KSPP/linux/issues/7

Signed-off-by: Tianlin Li <tli@digitalocean.com>
---
arch/arm/kernel/ftrace.c   |   8 +-
arch/arm64/kernel/ftrace.c |   6 +-
arch/nds32/kernel/ftrace.c |   6 +-
arch/x86/kernel/ftrace.c   |  13 ++-
include/linux/module.h     |  16 = ++--
kernel/livepatch/core.c    |  15 = +++-
kernel/module.c =            | 170 = +++++++++++++++++++++++++++----------
kernel/trace/ftrace.c =      |  15 +++-
8 files = changed, 175 insertions(+), 74 deletions(-)
- Can you break this patch into = 3 separate patches, by "subsystem":
= - = ftrace
- module
= - livepatch = (unless Jessica thinks maybe livepatch should go
  with = module?)
- Please run = patches through scripts/checkpatch.pl to catch common
 coding style issues (e.g. = blank line between variable declarations
 and function body).
- These changes look pretty straight forward, so I think it'd = be fine
 to drop = the "RFC" tag and include linux-kernel@vger.kernel.org in = the
 Cc list = for the v2. For lots of detail, see:
 https://www.kernel.org/doc/html/latest/process/submitting-patch= es.html

ok. = I will fix them in v2. Thanks a lot! 
More below...


diff --git = a/arch/arm/kernel/ftrace.c b/arch/arm/kernel/ftrace.c
index = bda949fd84e8..7ea1338821d6 100644
--- = a/arch/arm/kernel/ftrace.c
+++ = b/arch/arm/kernel/ftrace.c
@@ -59,13 +59,15 @@ static = unsigned long adjust_address(struct dyn_ftrace *rec, unsigned long = addr)

int = ftrace_arch_code_modify_prepare(void)
{
- = set_all_modules_text_rw();
- return = 0;
+ return set_all_modules_text_rw();
}

int = ftrace_arch_code_modify_post_process(void)
{
- = set_all_modules_text_ro();
+ int = ret;

Blank line here...

+ ret =3D = set_all_modules_text_ro();
+ if (ret)
+ return = ret;
/* Make sure any TLB misses during machine stop are = cleared. */
flush_tlb_all();
= return 0;

Are callers of these ftrace functions checking return values = too?
ftrace_arch_code_modify_prepare = is called in ftrace_run_update_code and = ftrace_module_enable. 
ftrace_run_update_code is checking = the return value of ftrace_arch_code_modify_prepare = already. 
ftrace_module_enable didn=E2=80=99t check. (I = modifies this function in this patch to check the return value of = ftrace_arch_code_modify_prepare ). 
Same for = ftrace_arch_code_modify_post_process. 

diff = --git a/arch/arm64/kernel/ftrace.c b/arch/arm64/kernel/ftrace.c
index 171773257974..97a89c38f6b9 100644
--- = a/arch/arm64/kernel/ftrace.c
+++ = b/arch/arm64/kernel/ftrace.c
@@ -115,9 +115,11 @@ int = ftrace_make_call(struct dyn_ftrace *rec, unsigned long addr)
= = = }

/* point the trampoline to our = ftrace entry point */
- module_disable_ro(mod);
+ = = = if (module_disable_ro(mod))
+ return = -EINVAL;
*dst =3D trampoline;
- = = = module_enable_ro(mod, true);
+ if = (module_enable_ro(mod, true))
+ return = -EINVAL;

/*
 * Ensure updated trampoline = is visible to instruction
diff --git = a/arch/nds32/kernel/ftrace.c b/arch/nds32/kernel/ftrace.c
index fd2a54b8cd57..e9e63e703a3e 100644
--- = a/arch/nds32/kernel/ftrace.c
+++ = b/arch/nds32/kernel/ftrace.c
@@ -91,14 +91,12 @@ int = __init ftrace_dyn_arch_init(void)

int = ftrace_arch_code_modify_prepare(void)
{
- = set_all_modules_text_rw();
- return = 0;
+ return set_all_modules_text_rw();
}

int = ftrace_arch_code_modify_post_process(void)
{
- = set_all_modules_text_ro();
- return = 0;
+ return set_all_modules_text_ro();
}

static unsigned long gen_sethi_insn(unsigned = long addr)
diff --git a/arch/x86/kernel/ftrace.c = b/arch/x86/kernel/ftrace.c
index = 024c3053dbba..7fdee06e2a76 100644
--- = a/arch/x86/kernel/ftrace.c
+++ = b/arch/x86/kernel/ftrace.c
@@ -42,19 +42,26 @@ int = ftrace_arch_code_modify_prepare(void)
 * and live kernel patching = from changing the text permissions while
 * ftrace has it set to = "read/write".
 */
+ int = ret;
mutex_lock(&text_mutex);
= set_kernel_text_rw();
- set_all_modules_text_rw();
+ = ret =3D set_all_modules_text_rw();
+ if (ret) = {
+ set_kernel_text_ro();

Is the set_kernel_text_ro() here = is an atomicity fix? Also, won't a future
patch need to check the result of that function call? (i.e. = should it
be left out = of this series and should atomicity fixes happen inside the
set_memory_*() functions? Can = they happen there?)
set_kernel_text_tro() = here was not for an atomicity fix in step 3, it was just for reverting = the status 
in ftrace_arch_code_modify_prepare, because = set_kernel_text_rw() is called. 
Thanks for pointing it = out.  I will check it. 

+ mutex_unlock(&text_mutex);
+ = = return ret;
+ }
return = 0;
}

int = ftrace_arch_code_modify_post_process(void)
    __releases(&text_mutex)
{
- set_all_modules_text_ro();
+ = int ret;

blank line needed...

+ ret =3D = set_all_modules_text_ro();
set_kernel_text_ro();
= mutex_unlock(&text_mutex);
- return = 0;
+ return ret;
}

union ftrace_code_union {
diff --git = a/include/linux/module.h b/include/linux/module.h
index = 1455812dd325..e6c7f3b719a3 100644
--- = a/include/linux/module.h
+++ b/include/linux/module.h
@@ -847,15 +847,15 @@ extern int module_sysfs_initialized;
#define __MODULE_STRING(x) __stringify(x)

#ifdef CONFIG_STRICT_MODULE_RWX
-extern void = set_all_modules_text_rw(void);
-extern void = set_all_modules_text_ro(void);
-extern void = module_enable_ro(const struct module *mod, bool after_init);
-extern void module_disable_ro(const struct module *mod);
+extern int set_all_modules_text_rw(void);
+extern int set_all_modules_text_ro(void);
+extern int module_enable_ro(const struct module *mod, bool = after_init);
+extern int module_disable_ro(const struct = module *mod);
#else
-static inline void = set_all_modules_text_rw(void) { }
-static inline void = set_all_modules_text_ro(void) { }
-static inline void = module_enable_ro(const struct module *mod, bool after_init) { }
-static inline void module_disable_ro(const struct module = *mod) { }
+static inline int set_all_modules_text_rw(void) = { return 0; }
+static inline int = set_all_modules_text_ro(void) { return 0; }
+static inline = int module_enable_ro(const struct module *mod, bool after_init) { return = 0; }

Please wrap this line (yes, the old one wasn't...)

+static= inline int module_disable_ro(const struct module *mod) { return 0; }
#endif

#ifdef = CONFIG_GENERIC_BUG
diff --git a/kernel/livepatch/core.c = b/kernel/livepatch/core.c
index c4ce08f43bd6..39bfc0685854 = 100644
--- a/kernel/livepatch/core.c
+++ = b/kernel/livepatch/core.c
@@ -721,16 +721,25 @@ static int = klp_init_object_loaded(struct klp_patch *patch,

= mutex_lock(&text_mutex);

- = module_disable_ro(patch->mod);
+ ret =3D = module_disable_ro(patch->mod);
+ if (ret) = {
+ mutex_unlock(&text_mutex);
+ return = ret;
+ }
ret =3D = klp_write_object_relocations(patch->mod, obj);
if (ret) = {
- module_enable_ro(patch->mod, true);
+ if = (module_enable_ro(patch->mod, true));
+ = pr_err("module_enable_ro failed.\n");

Why the pr_err() here and not in other failure cases? (Also, = should it
instead be a = WARN_ONCE() inside the set_memory_*() functions instead?)
Because module_enable_ro() is called in = the checking of another return value. I can not return the value, so I = print the error. 
Similar to the previous question, = should it be fixed by atomicity patches = later? 

= mutex_unlock(&text_mutex);
return = ret;
}

= arch_klp_init_object_loaded(patch, obj);
- = module_enable_ro(patch->mod, true);
+ ret =3D = module_enable_ro(patch->mod, true);
+ if (ret) = {
+ mutex_unlock(&text_mutex);
+ return = ret;
+ }

= mutex_unlock(&text_mutex);

diff = --git a/kernel/module.c b/kernel/module.c
index = 9ee93421269c..87125b5e315c 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -1900,111 +1900,162 @@ = static void mod_sysfs_teardown(struct module *mod)
 *
 * These values are always = page-aligned (as is base)
 */
-static = void frob_text(const struct module_layout *layout,
+static = int frob_text(const struct module_layout *layout,
      = ;int (*set_memory)(unsigned long start, int num_pages))
{ = BUG_ON((unsigned long)layout->base & (PAGE_SIZE-1));
= BUG_ON((unsigned long)layout->text_size & = (PAGE_SIZE-1));
- set_memory((unsigned = long)layout->base,
+ return set_memory((unsigned = long)layout->base,
   layout->text_s= ize >> PAGE_SHIFT);
}

#ifdef CONFIG_STRICT_MODULE_RWX
-static void = frob_rodata(const struct module_layout *layout,
+static = int frob_rodata(const struct module_layout *layout,
int = (*set_memory)(unsigned long start, int num_pages))
{
= BUG_ON((unsigned long)layout->base & (PAGE_SIZE-1));
= BUG_ON((unsigned long)layout->text_size & = (PAGE_SIZE-1));
BUG_ON((unsigned = long)layout->ro_size & (PAGE_SIZE-1));
- = set_memory((unsigned long)layout->base + = layout->text_size,
+ return set_memory((unsigned = long)layout->base + layout->text_size,
   (layout->ro_si= ze - layout->text_size) >> PAGE_SHIFT);
}

-static void frob_ro_after_init(const struct = module_layout *layout,
+static int = frob_ro_after_init(const struct module_layout *layout,
int = (*set_memory)(unsigned long start, int num_pages))
{
= BUG_ON((unsigned long)layout->base & (PAGE_SIZE-1));
= BUG_ON((unsigned long)layout->ro_size & (PAGE_SIZE-1));
= BUG_ON((unsigned long)layout->ro_after_init_size & = (PAGE_SIZE-1));
- set_memory((unsigned = long)layout->base + layout->ro_size,
+ return = set_memory((unsigned long)layout->base + layout->ro_size,
= =    (layout->ro_af= ter_init_size - layout->ro_size) >> PAGE_SHIFT);
}

-static void = frob_writable_data(const struct module_layout *layout,
+static int frob_writable_data(const struct module_layout = *layout,
      = ; int (*set_memory)(unsigned long start, int num_pages))
{
BUG_ON((unsigned = long)layout->base & (PAGE_SIZE-1));
= BUG_ON((unsigned long)layout->ro_after_init_size & = (PAGE_SIZE-1));
BUG_ON((unsigned = long)layout->size & (PAGE_SIZE-1));
- = set_memory((unsigned long)layout->base + = layout->ro_after_init_size,
+ return = set_memory((unsigned long)layout->base + = layout->ro_after_init_size,
   (layout->size = - layout->ro_after_init_size) >> PAGE_SHIFT);
}

/* livepatching wants to disable read-only so = it can frob module. */
-void module_disable_ro(const = struct module *mod)
+int module_disable_ro(const struct = module *mod)
{
+ int ret;
if = (!rodata_enabled)
- return;
+ return = 0;

- = frob_text(&mod->core_layout, set_memory_rw);
- = frob_rodata(&mod->core_layout, set_memory_rw);
- = frob_ro_after_init(&mod->core_layout, set_memory_rw);
- = frob_text(&mod->init_layout, set_memory_rw);
- = frob_rodata(&mod->init_layout, set_memory_rw);
+ = ret =3D frob_text(&mod->core_layout, set_memory_rw);
+ = if (ret)
+ return ret;
+ ret =3D = frob_rodata(&mod->core_layout, set_memory_rw);
+ if = (ret)
+ return ret;
+ ret =3D = frob_ro_after_init(&mod->core_layout, set_memory_rw);
+ = if (ret)
+ return ret;
+ ret =3D = frob_text(&mod->init_layout, set_memory_rw);
+ if = (ret)
+ return ret;
+ ret =3D = frob_rodata(&mod->init_layout, set_memory_rw);
+ if = (ret)
+ return ret;
+
+ return = 0;
}

-void = module_enable_ro(const struct module *mod, bool after_init)
+int module_enable_ro(const struct module *mod, bool = after_init)
{
+ int ret;
if = (!rodata_enabled)
- return;
+ return = 0;

= set_vm_flush_reset_perms(mod->core_layout.base);
= set_vm_flush_reset_perms(mod->init_layout.base);
- = frob_text(&mod->core_layout, set_memory_ro);
+ = ret =3D frob_text(&mod->core_layout, set_memory_ro);
+ = if (ret)
+ return ret;

- = frob_rodata(&mod->core_layout, set_memory_ro);
- = frob_text(&mod->init_layout, set_memory_ro);
- = frob_rodata(&mod->init_layout, set_memory_ro);
+ = ret =3D frob_rodata(&mod->core_layout, set_memory_ro);
+ = if (ret)
+ return ret;
+ ret =3D = frob_text(&mod->init_layout, set_memory_ro);
+ if = (ret)
+ return ret;
+ ret =3D = frob_rodata(&mod->init_layout, set_memory_ro);
+ if = (ret)
+ return ret;

- if = (after_init)
- = frob_ro_after_init(&mod->core_layout, set_memory_ro);
+ = if (after_init) {
+ ret =3D = frob_ro_after_init(&mod->core_layout, set_memory_ro);
+ = = if (ret)
+ return ret;
+ }
+ = return 0;
}

-static = void module_enable_nx(const struct module *mod)
+static = int module_enable_nx(const struct module *mod)
{
- = frob_rodata(&mod->core_layout, set_memory_nx);
- = frob_ro_after_init(&mod->core_layout, set_memory_nx);
- = frob_writable_data(&mod->core_layout, set_memory_nx);
- = frob_rodata(&mod->init_layout, set_memory_nx);
- = frob_writable_data(&mod->init_layout, set_memory_nx);
+ = int ret;
+
+ ret =3D = frob_rodata(&mod->core_layout, set_memory_nx);
+ if = (ret)
+ return ret;
+ ret =3D = frob_ro_after_init(&mod->core_layout, set_memory_nx);
+ = if (ret)
+ return ret;
+ ret =3D = frob_writable_data(&mod->core_layout, set_memory_nx);
+ = if (ret)
+ return ret;
+ ret =3D = frob_rodata(&mod->init_layout, set_memory_nx);
+ if = (ret)
+ return ret;
+ ret =3D = frob_writable_data(&mod->init_layout, set_memory_nx);
+ = if (ret)
+ return ret;
+
+ = return 0;
}

/* Iterate = through all modules and set each module's text as RW */
-void set_all_modules_text_rw(void)
+int = set_all_modules_text_rw(void)
{
struct = module *mod;
+ int ret;

= if (!rodata_enabled)
- return;
+ return = 0;

mutex_lock(&module_mutex);
= list_for_each_entry_rcu(mod, &modules, list) {
= = if (mod->state =3D=3D MODULE_STATE_UNFORMED)
= continue;

- = frob_text(&mod->core_layout, set_memory_rw);
- = = frob_text(&mod->init_layout, set_memory_rw);
+ = = ret =3D frob_text(&mod->core_layout, set_memory_rw);
+ = = if (ret) {
+ = mutex_unlock(&module_mutex);
+ return = ret;
+ }
+ ret =3D = frob_text(&mod->init_layout, set_memory_rw);
+ if (ret) = {
+ mutex_unlock(&module_mutex);
+ return = ret;
+ }

This pattern feels like it might be better with a "goto out" = style:

mutex_lock...
= list_for_each... {
= ret =3D = frob_...
= if = (ret)
= goto = out;
= ret =3D = frob_...
= if = (ret)
= goto = out;
}
ret =3D 0;
out:
= mutex_unlock...
= return = ret;

Thanks! = Will fix it in v2. 

}
= mutex_unlock(&module_mutex);
+ return = 0;
}

/* Iterate through all = modules and set each module's text as RO */
-void = set_all_modules_text_ro(void)
+int = set_all_modules_text_ro(void)
{
struct = module *mod;
+ int ret;

= if (!rodata_enabled)
- return;
+ return = 0;

mutex_lock(&module_mutex);
= list_for_each_entry_rcu(mod, &modules, list) {
@@= -2017,22 +2068,37 @@ void set_all_modules_text_ro(void)
= = = mod->state =3D=3D MODULE_STATE_GOING)
= continue;

- = frob_text(&mod->core_layout, set_memory_ro);
- = = frob_text(&mod->init_layout, set_memory_ro);
+ = = ret =3D frob_text(&mod->core_layout, set_memory_ro);
+ = = if (ret) {
+ = mutex_unlock(&module_mutex);
+ return = ret;
+ }
+ ret =3D = frob_text(&mod->init_layout, set_memory_ro);
+ if (ret) = {
+ mutex_unlock(&module_mutex);
+ return = ret;
+ }
}
= mutex_unlock(&module_mutex);
+ return = 0;
}
#else /* !CONFIG_STRICT_MODULE_RWX = */
-static void module_enable_nx(const struct module *mod) = { }
+static int module_enable_nx(const struct module *mod) = { return 0; }
#endif /*  CONFIG_STRICT_MODULE_RWX = */
-static void module_enable_x(const struct module = *mod)
+static int module_enable_x(const struct module = *mod)
{
- = frob_text(&mod->core_layout, set_memory_x);
- = frob_text(&mod->init_layout, set_memory_x);
+ = int ret;
+ ret =3D = frob_text(&mod->core_layout, set_memory_x);
+ if = (ret)
+ return ret;
+ ret =3D = frob_text(&mod->init_layout, set_memory_x);
+ if = (ret)
+ return ret;
+ return 0;
}
#else /* !CONFIG_ARCH_HAS_STRICT_MODULE_RWX */
-static void module_enable_nx(const struct module *mod) { = }
-static void module_enable_x(const struct module *mod) { = }
+static int module_enable_nx(const struct module *mod) { = return 0; }
+static int module_enable_x(const struct = module *mod) { return 0; }
#endif /* = CONFIG_ARCH_HAS_STRICT_MODULE_RWX */


@@ -3534,7 +3600,11 @@ static noinline int = do_init_module(struct module *mod)
/* Switch = to core kallsyms now init is done: kallsyms may be walking! */
= rcu_assign_pointer(mod->kallsyms, = &mod->core_kallsyms);
#endif
- = module_enable_ro(mod, true);
+ ret =3D = module_enable_ro(mod, true);
+ if (ret) {
+ = mutex_unlock(&module_mutex);
+ goto = fail_free_freeinit;
+ }
= mod_tree_remove_init(mod);
= module_arch_freeing_init(mod);
= mod->init_layout.base =3D NULL;
@@ -3640,9 = +3710,15 @@ static int complete_formation(struct module *mod, struct = load_info *info)
/* This relies on module_mutex = for list integrity. */
module_bug_finalize(info->hdr, = info->sechdrs, mod);

- = module_enable_ro(mod, false);
- = module_enable_nx(mod);
- = module_enable_x(mod);
+ err =3D module_enable_ro(mod, = false);
+ if (err)
+ goto = out;
+ err =3D module_enable_nx(mod);
+ if = (err)
+ goto out;
+ err =3D module_enable_x(mod);
+ = if (err)
+ goto out;

= /* Mark state as coming so strong_try_module_get() ignores us,
=  * but kallsyms = etc. can see us. */
diff --git a/kernel/trace/ftrace.c = b/kernel/trace/ftrace.c
index f9821a3374e9..d4532bb65d1b = 100644
--- a/kernel/trace/ftrace.c
+++ = b/kernel/trace/ftrace.c
@@ -5814,8 +5814,13 @@ void = ftrace_module_enable(struct module *mod)
 * text to read-only, as we = now need to set it back to read-write
 * so that we can modify the = text.
 */
- = if (ftrace_start_up)
- = ftrace_arch_code_modify_prepare();
+ if = (ftrace_start_up) {
+ int ret =3D = ftrace_arch_code_modify_prepare();
+ if (ret) = {
+ FTRACE_WARN_ON(ret);
+ goto = out_unlock;
+ }

Can FTRACE_WARN_ON() be used in = an "if" like WARN_ON? This could maybe
look like:

= ret =3D = ftrace_arch...
= if = (FTRACE_WARN_ON(ret))
= goto = out_unlock

Though I not = this doesn't result in an error getting passed up? i.e.
ftrace_module_enable() is still = "void". Does that matter here?
Thanks. I = will fix it in v2. 

+ }

= do_for_each_ftrace_rec(pg, rec) {
int = cnt;
@@ -5854,8 +5859,10 @@ void = ftrace_module_enable(struct module *mod)
} = while_for_each_ftrace_rec();

 out_loop:
- if (ftrace_start_up)
- = = ftrace_arch_code_modify_post_process();
+ if = (ftrace_start_up) {
+ int ret =3D = ftrace_arch_code_modify_post_process();
+ = FTRACE_WARN_ON(ret);
+ }

 out_unlock:
= mutex_unlock(&ftrace_lock);
-- 
2.17.1


Thanks,
Thanks for all detail = suggestions! 
-- 
Kees = Cook

= --Apple-Mail=_96CC2F9E-A2F9-4977-908B-8F533F03F8D0--