Re: [PATCH v7 13/14] module: Do not set nx for module memory before freeing

From: Nadav Amit <namit@vmware.com>
To: Jessica Yu <jeyu@kernel.org>
Cc: Ingo Molnar <mingo@redhat.com>,
	LKML <linux-kernel@vger.kernel.org>, X86 ML <x86@kernel.org>,
	"H. Peter Anvin" <hpa@zytor.com>,
	Thomas Gleixner <tglx@linutronix.de>,
	Borislav Petkov <bp@alien8.de>, Andy Lutomirski <luto@kernel.org>,
	Dave Hansen <dave.hansen@linux.intel.com>,
	Peter Zijlstra <peterz@infradead.org>,
	Damian Tometzki <linux_dti@icloud.com>,
	linux-integrity <linux-integrity@vger.kernel.org>,
	LSM List <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 17:25:12 +0000	[thread overview]
Message-ID: <634A88B1-B41C-466C-A98A-DB85065A4BA9@vmware.com> (raw)
In-Reply-To: <20181213141013.GA16819@linux-8ccs>

> On Dec 13, 2018, at 6:10 AM, Jessica Yu <jeyu@kernel.org> wrote:
> 
> +++ 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 guess it is just plain wrong in this case… ;-)

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

Yes, I missed this one. I think Rick Edgecombe has a better version of this
patch that also takes care of this case (see
https://lkml.org/lkml/2018/12/11/1573 ). I think he will merge the rest of
this series (although I’m still waiting for Thomas/Ingo to tell me what’s it
going to be with the first patches).

> 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 :-/

Thanks for looking into it. I gave up after I saw it should have no
architectural reason (on x86) and could not think about such one (on any
arch., certainly for the data). Anyhow, that’s what automatic testing are
for. If this is wrong, things should crash and burn very fast.