Received: by 2002:ad5:474a:0:0:0:0:0 with SMTP id i10csp890020imu; Thu, 13 Dec 2018 06:12:41 -0800 (PST) X-Google-Smtp-Source: AFSGD/UUIVaj8xGFJ5gK3DRK55NQq64dyZLv99oPCt0q1fntwTcP4o3Ei0OQnG4vSC+pd/Bjz0ut X-Received: by 2002:a62:b80a:: with SMTP id p10mr24157152pfe.32.1544710361320; Thu, 13 Dec 2018 06:12:41 -0800 (PST) ARC-Seal: i=1; a=rsa-sha256; t=1544710361; cv=none; d=google.com; s=arc-20160816; b=QaBxXffhAQhS7j7WBegGJsje+6QadfozJTji9fZac9Iss1mc4jT1+/FuB5VCDG49it Y+vfisV7eDDzkerfJjQ1a9ZDqD0fvtUjzvaeubBwXta/jUN6pRp/x68BFkkqRIL+6NP7 V/LWBKSLz2g7l+yQgydZIHSAMcvdzbnfdwWKkNKr9/u5sx3evgULJfFuWwsD3x1KoUfn 3GF00xjsv5b2o7J40fxAdnuWAGjtgX+kppIJNpdaXa9dgeaBvo/QeOrZfkuwtxwsIsVh 2He2LT7L5khjdLB/9gFp1JpjVDYpRN+YjEyDYP1OWrIqxoxyq8d7mhF5mMIoY3GSrh4Z Q3FQ== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:sender:user-agent:in-reply-to :content-disposition:mime-version:references:message-id:subject:cc :to:from:date:dkim-signature; bh=zWCnjUexUr293QcmSN9FuBYdpf6GmqYK2CfY1lArpBI=; b=O+pkBn2MW+icuPprGuyuzFaYJZexOLWv1dCC84v0ohKXAB4U9rJUkcG8Gr5+7yX4GM Ug9HHrWBRphDvu/BwAw6vw2SXKtdZYaAd3tUZfVYfpJqcbOE1r9m2o5K5Gmz4Nq9G6tu KdJVtZ8ABBkj1fCSzDgTJU8gFy9iRsbLVUnCtGUnxceLR4b5Kt+atCTbOE0itJcVL7TM jB++Js3N2CgYvvMRnzvtUuB7j2vae6+Wv6PhP5RbFp6e+3dI2RkhBhWK5ZiZXCl3GOnZ kiJCMVWiPA6KzZyvwHJ/rB87/HQBfDVR8COTxWvD4ln68yqYIqMrn4sknV1/9bDdg76t TyGw== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@kernel.org header.s=default header.b=ZOuhypgq; spf=pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=kernel.org Return-Path: Received: from vger.kernel.org (vger.kernel.org. [209.132.180.67]) by mx.google.com with ESMTP id m1si1634647pfi.286.2018.12.13.06.12.25; Thu, 13 Dec 2018 06:12:41 -0800 (PST) Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67; Authentication-Results: mx.google.com; dkim=pass header.i=@kernel.org header.s=default header.b=ZOuhypgq; spf=pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=kernel.org Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1728707AbeLMOKW (ORCPT + 99 others); Thu, 13 Dec 2018 09:10:22 -0500 Received: from mail.kernel.org ([198.145.29.99]:54102 "EHLO mail.kernel.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1727720AbeLMOKW (ORCPT ); Thu, 13 Dec 2018 09:10:22 -0500 Received: from linux-8ccs (p5DE45A67.dip0.t-ipconnect.de [93.228.90.103]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPSA id 91FDA2086D; Thu, 13 Dec 2018 14:10:16 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=default; t=1544710220; bh=SSWtJydgAT1VIU1V2HZUv33F7SJctJF2ncli7OfWFOQ=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=ZOuhypgqy+brjCnFoNs8MojU6a/Cdy++6vjYA4O+gsdTIVA3QVy+arpdsjE8Iv7fi IsMsa6H3WjdgvhGiUa2KDhzu7H65oNgpc0p4DaOkqr3gdTglYeaT889xP3tlydk5kD 8baQXkUowvIEmcLwHDGRpyie7Ui+h0GmJkHjMRAc= Date: Thu, 13 Dec 2018 15:10:13 +0100 From: Jessica Yu To: Nadav Amit Cc: Ingo Molnar , linux-kernel@vger.kernel.org, x86@kernel.org, "H. Peter Anvin" , Thomas Gleixner , Borislav Petkov , Andy Lutomirski , Nadav Amit , Dave Hansen , Peter Zijlstra , linux_dti@icloud.com, linux-integrity@vger.kernel.org, linux-security-module@vger.kernel.org, Rick P Edgecombe , Will Deacon , Andrea Parri Subject: Re: [PATCH v7 13/14] module: Do not set nx for module memory before freeing Message-ID: <20181213141013.GA16819@linux-8ccs> References: <20181205013408.47725-1-namit@vmware.com> <20181205013408.47725-14-namit@vmware.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii; format=flowed Content-Disposition: inline In-Reply-To: <20181205013408.47725-14-namit@vmware.com> X-OS: Linux linux-8ccs 4.12.14-lp150.12.22-default x86_64 User-Agent: Mutt/1.10.1 (2018-07-13) Sender: linux-kernel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org +++ Nadav Amit [04/12/18 17:34 -0800]: >When module memory is about to be freed, there is no apparent reason to >make it (and its data) executable, but that's exactly what is done >today. This is not efficient and not secure. > >There are various theories why it was done, but none of them seem as >something that really require it today. nios2 uses kmalloc for module >memory, but anyhow it does not change the PTEs of the module memory. In >x86, changing vmalloc'd memory mappings also modifies the direct mapping >alias, but the NX-bit is not modified in such way. > >So let's remove it. Andy suggested that the changes of the PTEs can be >avoided (excluding the direct-mapping alias), which is true. However, >in x86 it requires some cleanup of the contiguous page allocator, which >is outside of the scope of this patch-set. > >Cc: Rick P Edgecombe >Cc: Will Deacon >Cc: Andy Lutomirski >Signed-off-by: Nadav Amit [ Thanks Andrea Parri for the cc ] Regarding the patch subject, don't you mean "Do not make module memory executable" or "Do not unset nx" instead of "Do not set nx"? Hm, these double negatives are confusing :-) I think this also needs to be done in the load_module() error path. See the bug_cleanup label. There, module_disable_{ro,nx}() are called before module deallocation. I am not sure why all this was made executable before freeing in the first place. Tried to dig through the commit history and the first commit that introduced this behavior was 448694a1d50 ("module: undo module RONX protection correctly"). There, the behavior was changed from W+NX to W+X before releasing the module. But AFAIK from the changelog, there was no real technical reason behind it, it stemmed out of the complaint of code asymmetry :-/ Jessica >--- > kernel/module.c | 35 ++++++++++++++++++++++------------- > 1 file changed, 22 insertions(+), 13 deletions(-) > >diff --git a/kernel/module.c b/kernel/module.c >index 7cb207249437..57c5b23746e7 100644 >--- a/kernel/module.c >+++ b/kernel/module.c >@@ -2027,20 +2027,29 @@ void set_all_modules_text_ro(void) > mutex_unlock(&module_mutex); > } > >-static void disable_ro_nx(const struct module_layout *layout) >+static void module_restore_mappings(const struct module_layout *layout) > { >- if (rodata_enabled) { >- frob_text(layout, set_memory_rw); >- frob_rodata(layout, set_memory_rw); >- frob_ro_after_init(layout, set_memory_rw); >- } >- frob_rodata(layout, set_memory_x); >- frob_ro_after_init(layout, set_memory_x); >- frob_writable_data(layout, set_memory_x); >+ /* >+ * First, make the mappings of the code non-executable to prevent >+ * transient W+X mappings from being set when the text is set as RW. >+ */ >+ frob_text(layout, set_memory_nx); >+ >+ if (!rodata_enabled) >+ return; >+ >+ /* >+ * Second, set the memory as writable. Although the module memory is >+ * about to be freed, these calls are required (at least on x86) to >+ * restore the direct map to its "correct" state. >+ */ >+ frob_text(layout, set_memory_rw); >+ frob_rodata(layout, set_memory_rw); >+ frob_ro_after_init(layout, set_memory_rw); > } > > #else >-static void disable_ro_nx(const struct module_layout *layout) { } >+static void module_restore_mappings(const struct module_layout *layout) { } > static void module_enable_nx(const struct module *mod) { } > static void module_disable_nx(const struct module *mod) { } > #endif >@@ -2173,7 +2182,7 @@ static void free_module(struct module *mod) > mutex_unlock(&module_mutex); > > /* This may be empty, but that's OK */ >- disable_ro_nx(&mod->init_layout); >+ module_restore_mappings(&mod->init_layout); > module_arch_freeing_init(mod); > module_memfree(mod->init_layout.base); > kfree(mod->args); >@@ -2183,7 +2192,7 @@ static void free_module(struct module *mod) > lockdep_free_key_range(mod->core_layout.base, mod->core_layout.size); > > /* Finally, free the core (containing the module structure) */ >- disable_ro_nx(&mod->core_layout); >+ module_restore_mappings(&mod->core_layout); > module_memfree(mod->core_layout.base); > } > >@@ -3507,7 +3516,7 @@ static noinline int do_init_module(struct module *mod) > #endif > module_enable_ro(mod, true); > mod_tree_remove_init(mod); >- disable_ro_nx(&mod->init_layout); >+ module_restore_mappings(&mod->init_layout); > module_arch_freeing_init(mod); > mod->init_layout.base = NULL; > mod->init_layout.size = 0; >-- >2.17.1 >