Linux-Security-Module Archive on lore.kernel.org
 help / Atom feed
From: Jessica Yu <jeyu@kernel.org>
To: Nadav Amit <namit@vmware.com>
Cc: Ingo Molnar <mingo@redhat.com>,
	linux-kernel@vger.kernel.org, x86@kernel.org,
	"H. Peter Anvin" <hpa@zytor.com>,
	Thomas Gleixner <tglx@linutronix.de>,
	Borislav Petkov <bp@alien8.de>, Andy Lutomirski <luto@kernel.org>,
	Nadav Amit <nadav.amit@gmail.com>,
	Dave Hansen <dave.hansen@linux.intel.com>,
	Peter Zijlstra <peterz@infradead.org>,
	linux_dti@icloud.com, linux-integrity@vger.kernel.org,
	linux-security-module@vger.kernel.org,
	Rick P Edgecombe <rick.p.edgecombe@intel.com>,
	Will Deacon <will.deacon@arm.com>,
	Andrea Parri <andrea.parri@amarulasolutions.com>
Subject: Re: [PATCH v7 13/14] module: Do not set nx for module memory before freeing
Date: Thu, 13 Dec 2018 15:10:13 +0100
Message-ID: <20181213141013.GA16819@linux-8ccs> (raw)
In-Reply-To: <20181205013408.47725-14-namit@vmware.com>

+++ 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 <rick.p.edgecombe@intel.com>
>Cc: Will Deacon <will.deacon@arm.com>
>Cc: Andy Lutomirski <luto@kernel.org>
>Signed-off-by: Nadav Amit <namit@vmware.com>

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

  parent reply index

Thread overview: 30+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-12-05  1:33 [PATCH v7 00/14] x86/alternative: text_poke() enhancements Nadav Amit
2018-12-05  1:33 ` [PATCH v7 01/14] Fix "x86/alternatives: Lockdep-enforce text_mutex in text_poke*()" Nadav Amit
2018-12-05  1:33 ` [PATCH v7 02/14] x86/jump_label: Use text_poke_early() during early init Nadav Amit
2018-12-05  1:33 ` [PATCH v7 03/14] x86/mm: temporary mm struct Nadav Amit
2018-12-05  1:33 ` [PATCH v7 04/14] fork: provide a function for copying init_mm Nadav Amit
2018-12-05  1:33 ` [PATCH v7 05/14] x86/alternative: initializing temporary mm for patching Nadav Amit
2018-12-05  1:34 ` [PATCH v7 06/14] x86/alternative: use temporary mm for text poking Nadav Amit
2018-12-05  1:34 ` [PATCH v7 07/14] x86/kgdb: avoid redundant comparison of patched code Nadav Amit
2018-12-05  1:34 ` [PATCH v7 08/14] x86/ftrace: Use text_poke_*() infrastructure Nadav Amit
2018-12-06  0:06   ` Nadav Amit
2018-12-06 16:28     ` Ingo Molnar
2018-12-05  1:34 ` [PATCH v7 09/14] x86/kprobes: Instruction pages initialization enhancements Nadav Amit
2018-12-06 13:09   ` Masami Hiramatsu
2018-12-05  1:34 ` [PATCH v7 10/14] x86: avoid W^X being broken during modules loading Nadav Amit
2018-12-05  1:34 ` [PATCH v7 11/14] x86/jump-label: remove support for custom poker Nadav Amit
2018-12-05  1:34 ` [PATCH v7 12/14] x86/alternative: Remove the return value of text_poke_*() Nadav Amit
2018-12-05  1:34 ` [PATCH v7 13/14] module: Do not set nx for module memory before freeing Nadav Amit
2018-12-06  9:57   ` Peter Zijlstra
2018-12-06 17:28     ` Nadav Amit
2018-12-06 11:13   ` Andrea Parri
2018-12-06 18:52   ` Andy Lutomirski
2018-12-06 18:56     ` Nadav Amit
2018-12-06 20:21     ` Edgecombe, Rick P
2018-12-06 20:29       ` Nadav Amit
2018-12-13 14:10   ` Jessica Yu [this message]
2018-12-13 17:25     ` Nadav Amit
2018-12-05  1:34 ` [PATCH v7 14/14] module: Prevent module removal racing with text_poke() Nadav Amit
2018-12-06 10:01   ` Peter Zijlstra
2018-12-06 10:03 ` [PATCH v7 00/14] x86/alternative: text_poke() enhancements Peter Zijlstra
2018-12-10  1:06   ` Nadav Amit

Reply instructions:

You may reply publically to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20181213141013.GA16819@linux-8ccs \
    --to=jeyu@kernel.org \
    --cc=andrea.parri@amarulasolutions.com \
    --cc=bp@alien8.de \
    --cc=dave.hansen@linux.intel.com \
    --cc=hpa@zytor.com \
    --cc=linux-integrity@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-security-module@vger.kernel.org \
    --cc=linux_dti@icloud.com \
    --cc=luto@kernel.org \
    --cc=mingo@redhat.com \
    --cc=nadav.amit@gmail.com \
    --cc=namit@vmware.com \
    --cc=peterz@infradead.org \
    --cc=rick.p.edgecombe@intel.com \
    --cc=tglx@linutronix.de \
    --cc=will.deacon@arm.com \
    --cc=x86@kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Linux-Security-Module Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-security-module/0 linux-security-module/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-security-module linux-security-module/ https://lore.kernel.org/linux-security-module \
		linux-security-module@vger.kernel.org linux-security-module@archiver.kernel.org
	public-inbox-index linux-security-module


Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-security-module


AGPL code for this site: git clone https://public-inbox.org/ public-inbox