linux-kernel - Re: PROBLEM: Resume form hibernate broken by setting NX on gap

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <3878383.bGDWSmEsze@vostro.rjw.lan>
Date:	Sun, 12 Jun 2016 03:05:04 +0200
From:	"Rafael J. Wysocki" <rjw@...ysocki.net>
To:	Logan Gunthorpe <logang@...tatee.com>
Cc:	Kees Cook <keescook@...omium.org>,
	"Rafael J. Wysocki" <rafael@...nel.org>,
	Stephen Smalley <sds@...ho.nsa.gov>,
	Ingo Molnar <mingo@...nel.org>, Ingo Molnar <mingo@...hat.com>,
	the arch/x86 maintainers <x86@...nel.org>,
	"linux-pm@...r.kernel.org" <linux-pm@...r.kernel.org>,
	Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
	Andy Lutomirski <luto@...nel.org>,
	Borislav Petkov <bp@...en8.de>,
	Denys Vlasenko <dvlasenk@...hat.com>,
	Brian Gerst <brgerst@...il.com>
Subject: Re: PROBLEM: Resume form hibernate broken by setting NX on gap

On Saturday, June 11, 2016 11:39:48 AM Logan Gunthorpe wrote:
> Hey Rafael,
> 
> I tried this patch as well and there was no change.
> 
> I have a couple tentative observations to make though. None of this is 
> 100% clear to me so please correct me if I'm wrong anywhere:
> 
> 1) Commit ab76f7b4ab only extends the NX bit between __ex_table and 
> rodata; which, by my understanding, shouldn't be used by anything. And 
> __ex_table and rodata are fixed by the kernel's binary so both symbols 
> should be the same in both the image kernel and the boot kernel given 
> that both are running from the same binary.

Well, what if the kernel is relocated?

> 2) When ab76f7b4ab is reverted, hibernation seems to work 100%. Though, 
> when it's in place, it only works some of the time. Given that commit is 
> only extending the NX region a bit, if there is some random mismatch, 
> why does it never reach rodata? In other words, why is rodata a magic 
> line that seems to work all the time -- why doesn't this random mismatch 
> ever extend into the rodata region? rodata isn't _that_ far away from 
> the end of ex_table.

That's a very good question. :-)

Overall, it looks like re-using the boot kernel text mapping in the temporary
page tables is a bad idea.  I guess a temporary kernel text mapping is needed
too or at least the existing one has to be modified to cover the trampoline
code properly.

> Anyway, thanks again for looking into this.

No problem.  I haven't helped much so far, though ...

Can you please check if the patch below makes any difference?

---
 arch/x86/power/hibernate_64.c |   19 +++++++++++++++----
 1 file changed, 15 insertions(+), 4 deletions(-)

Index: linux-pm/arch/x86/power/hibernate_64.c
===================================================================
--- linux-pm.orig/arch/x86/power/hibernate_64.c
+++ linux-pm/arch/x86/power/hibernate_64.c
@@ -12,6 +12,7 @@
 #include <linux/smp.h>
 #include <linux/suspend.h>
 
+#include <asm/cacheflush.h>
 #include <asm/init.h>
 #include <asm/proto.h>
 #include <asm/page.h>
@@ -27,7 +28,7 @@ extern asmlinkage __visible int restore_
  * Address to jump to in the last phase of restore in order to get to the image
  * kernel's text (this value is passed in the image header).
  */
-unsigned long restore_jump_address __visible;
+void *restore_jump_address __visible;
 
 /*
  * Value of the cr3 register from before the hibernation (this value is passed
@@ -108,7 +109,7 @@ int pfn_is_nosave(unsigned long pfn)
 }
 
 struct restore_data_record {
-	unsigned long jump_address;
+	void *jump_address;
 	unsigned long cr3;
 	unsigned long magic;
 };
@@ -126,7 +127,7 @@ int arch_hibernation_header_save(void *a
 
 	if (max_size < sizeof(struct restore_data_record))
 		return -EOVERFLOW;
-	rdr->jump_address = restore_jump_address;
+	rdr->jump_address = &restore_registers;
 	rdr->cr3 = restore_cr3;
 	rdr->magic = RESTORE_MAGIC;
 	return 0;
@@ -140,8 +141,18 @@ int arch_hibernation_header_save(void *a
 int arch_hibernation_header_restore(void *addr)
 {
 	struct restore_data_record *rdr = addr;
+	unsigned long text_end, all_end;
+
+	if (rdr->magic != RESTORE_MAGIC)
+		return -EINVAL;
 
 	restore_jump_address = rdr->jump_address;
 	restore_cr3 = rdr->cr3;
-	return (rdr->magic == RESTORE_MAGIC) ? 0 : -EINVAL;
+
+	text_end = PFN_ALIGN(&__stop___ex_table);
+	all_end = roundup((unsigned long)restore_jump_address, PMD_SIZE);
+	if (all_end > text_end)
+		set_memory_x(text_end, (all_end - text_end) >> PAGE_SHIFT);
+
+	return 0;
 }