linux-kernel - Re: [PATCH v2 01/24] x86/xen: Mark cpu_bringup_and_idle() as dead_end

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: <20210820193107.omvshmsqbpxufzkc@treble>
Date:   Fri, 20 Aug 2021 12:31:07 -0700
From:   Josh Poimboeuf <jpoimboe@...hat.com>
To:     Peter Zijlstra <peterz@...radead.org>
Cc:     tglx@...utronix.de, linux-kernel@...r.kernel.org, joro@...tes.org,
        boris.ostrovsky@...cle.com, jgross@...e.com, x86@...nel.org,
        mbenes@...e.com, rostedt@...dmis.org, dvyukov@...gle.com,
        elver@...gle.com
Subject: Re: [PATCH v2 01/24] x86/xen: Mark cpu_bringup_and_idle() as
 dead_end_function

On Fri, Aug 20, 2021 at 12:22:28PM -0700, Josh Poimboeuf wrote:
> On Thu, Jun 24, 2021 at 11:41:00AM +0200, Peter Zijlstra wrote:
> > The asm_cpu_bringup_and_idle() function is required to push the return
> > value on the stack in order to make ORC happy, but the only reason
> > objtool doesn't complain is because of a happy accident.
> > 
> > The thing is that asm_cpu_bringup_and_idle() doesn't return, so
> > validate_branch() never terminates and falls through to the next
> > function, which in the normal case is the hypercall_page. And that, as
> > it happens, is 4095 NOPs and a RET.
> > 
> > Make asm_cpu_bringup_and_idle() terminate on it's own, by making the
> > function it calls as a dead-end. This way we no longer rely on what
> > code happens to come after.
> > 
> > Fixes: c3881eb58d56 ("x86/xen: Make the secondary CPU idle tasks reliable")
> > Signed-off-by: Peter Zijlstra (Intel) <peterz@...radead.org>
> 
> Looks right.  Only problem is, with my assembler I get this:
> 
>   arch/x86/kernel/head_64.o: warning: objtool: .text+0x5: unreachable instruction
> 
> Because gas insists on jumping over the page of nops...
> 
> 0000000000000000 <asm_cpu_bringup_and_idle>:
>        0:	e8 00 00 00 00       	callq  5 <asm_cpu_bringup_and_idle+0x5>
> 			1: R_X86_64_PLT32	cpu_bringup_and_idle-0x4
>        5:	e9 f6 0f 00 00       	jmpq   1000 <xen_hypercall_set_trap_table>
>        a:	66 66 2e 0f 1f 84 00 	data16 nopw %cs:0x0(%rax,%rax,1)
>       11:	00 00 00 00 
>       15:	66 66 2e 0f 1f 84 00 	data16 nopw %cs:0x0(%rax,%rax,1)
>       1c:	00 00 00 00 
>       20:	66 66 2e 0f 1f 84 00 	data16 nopw %cs:0x0(%rax,%rax,1)
>       27:	00 00 00 00 
>       2b:	66 66 2e 0f 1f 84 00 	data16 nopw %cs:0x0(%rax,%rax,1)
>       32:	00 00 00 00 
>       36:	66 66 2e 0f 1f 84 00 	data16 nopw %cs:0x0(%rax,%rax,1)
>       3d:	00 00 00 00 

Here's a fix:

From: Josh Poimboeuf <jpoimboe@...hat.com>
Subject: [PATCH] x86/xen: Move hypercall_page to top of the file

Because hypercall_page is page-aligned, the assembler inexplicably adds
an unreachable jump from after the end of the previous code to the
beginning of hypercall_page.

That confuses objtool, understandably.  It also creates significant text
fragmentation.  As a result, much of the object file is wasted text
(nops).

Move hypercall_page to the beginning of the file to both prevent the
text fragmentation and avoid the dead jump instruction.

$ size /tmp/head_64.before.o /tmp/head_64.after.o
   text	   data	    bss	    dec	    hex	filename
  10924	 307252	   4096	 322272	  4eae0	/tmp/head_64.before.o
   6823	 307252	   4096	 318171	  4dadb	/tmp/head_64.after.o

Signed-off-by: Josh Poimboeuf <jpoimboe@...hat.com>
---
 arch/x86/xen/xen-head.S | 34 +++++++++++++++++-----------------
 1 file changed, 17 insertions(+), 17 deletions(-)

diff --git a/arch/x86/xen/xen-head.S b/arch/x86/xen/xen-head.S
index cb6538ae2fe0..488944d6d430 100644
--- a/arch/x86/xen/xen-head.S
+++ b/arch/x86/xen/xen-head.S
@@ -20,6 +20,23 @@
 #include <xen/interface/xen-mca.h>
 #include <asm/xen/interface.h>
 
+.pushsection .text
+	.balign PAGE_SIZE
+SYM_CODE_START(hypercall_page)
+	.rept (PAGE_SIZE / 32)
+		UNWIND_HINT_FUNC
+		.skip 31, 0x90
+		ret
+	.endr
+
+#define HYPERCALL(n) \
+	.equ xen_hypercall_##n, hypercall_page + __HYPERVISOR_##n * 32; \
+	.type xen_hypercall_##n, @function; .size xen_hypercall_##n, 32
+#include <asm/xen-hypercalls.h>
+#undef HYPERCALL
+SYM_CODE_END(hypercall_page)
+.popsection
+
 #ifdef CONFIG_XEN_PV
 	__INIT
 SYM_CODE_START(startup_xen)
@@ -64,23 +81,6 @@ SYM_CODE_END(asm_cpu_bringup_and_idle)
 #endif
 #endif
 
-.pushsection .text
-	.balign PAGE_SIZE
-SYM_CODE_START(hypercall_page)
-	.rept (PAGE_SIZE / 32)
-		UNWIND_HINT_FUNC
-		.skip 31, 0x90
-		ret
-	.endr
-
-#define HYPERCALL(n) \
-	.equ xen_hypercall_##n, hypercall_page + __HYPERVISOR_##n * 32; \
-	.type xen_hypercall_##n, @function; .size xen_hypercall_##n, 32
-#include <asm/xen-hypercalls.h>
-#undef HYPERCALL
-SYM_CODE_END(hypercall_page)
-.popsection
-
 	ELFNOTE(Xen, XEN_ELFNOTE_GUEST_OS,       .asciz "linux")
 	ELFNOTE(Xen, XEN_ELFNOTE_GUEST_VERSION,  .asciz "2.6")
 	ELFNOTE(Xen, XEN_ELFNOTE_XEN_VERSION,    .asciz "xen-3.0")
-- 
2.31.1