[v10,04/11] x86/entry/64: Adapt assembly for PIE support
diff mbox series

Message ID 20191205000957.112719-5-thgarnie@chromium.org
State New
Headers show
Series
  • x86: PIE support to extend KASLR randomization
Related show

Commit Message

Thomas Garnier Dec. 5, 2019, 12:09 a.m. UTC
Change the assembly code to use only relative references of symbols for the
kernel to be PIE compatible.

Position Independent Executable (PIE) support will allow to extend the
KASLR randomization range below 0xffffffff80000000.

Signed-off-by: Thomas Garnier <thgarnie@chromium.org>
Reviewed-by: Kees Cook <keescook@chromium.org>
---
 arch/x86/entry/entry_64.S | 16 +++++++++++-----
 1 file changed, 11 insertions(+), 5 deletions(-)

Comments

Peter Zijlstra Dec. 5, 2019, 9:03 a.m. UTC | #1
On Wed, Dec 04, 2019 at 04:09:41PM -0800, Thomas Garnier wrote:

> @@ -1625,7 +1627,11 @@ first_nmi:
>  	addq	$8, (%rsp)	/* Fix up RSP */
>  	pushfq			/* RFLAGS */
>  	pushq	$__KERNEL_CS	/* CS */
> -	pushq	$1f		/* RIP */
> +	pushq	$0		/* Future return address */

We're building an IRET frame, the IRET frame does not have a 'future
return address' field.

> +	pushq	%rdx		/* Save RAX */

fail..

> +	leaq	1f(%rip), %rdx	/* RIP */

nonsensical comment

> +	movq    %rdx, 8(%rsp)   /* Put 1f on return address */
> +	popq	%rdx		/* Restore RAX */

fail..

>  	iretq			/* continues at repeat_nmi below */
>  	UNWIND_HINT_IRET_REGS
>  1:
> -- 
> 2.24.0.393.g34dc348eaf-goog
>
Thomas Garnier Dec. 5, 2019, 5:01 p.m. UTC | #2
On Thu, Dec 5, 2019 at 1:04 AM Peter Zijlstra <peterz@infradead.org> wrote:
>
> On Wed, Dec 04, 2019 at 04:09:41PM -0800, Thomas Garnier wrote:
>
> > @@ -1625,7 +1627,11 @@ first_nmi:
> >       addq    $8, (%rsp)      /* Fix up RSP */
> >       pushfq                  /* RFLAGS */
> >       pushq   $__KERNEL_CS    /* CS */
> > -     pushq   $1f             /* RIP */
> > +     pushq   $0              /* Future return address */
>
> We're building an IRET frame, the IRET frame does not have a 'future
> return address' field.

I assumed that's the target RIP after iretq.

>
> > +     pushq   %rdx            /* Save RAX */
>
> fail..

Yes, sorry. I was asked to switch from RAX to RDX and missed the comment.

>
> > +     leaq    1f(%rip), %rdx  /* RIP */
>
> nonsensical comment

That was the same comment from the push $1f that I changed.

>
> > +     movq    %rdx, 8(%rsp)   /* Put 1f on return address */
> > +     popq    %rdx            /* Restore RAX */
>
> fail..

I will change in next iteration.

>
> >       iretq                   /* continues at repeat_nmi below */
> >       UNWIND_HINT_IRET_REGS
> >  1:
> > --
> > 2.24.0.393.g34dc348eaf-goog
> >
Peter Zijlstra Dec. 6, 2019, 10:26 a.m. UTC | #3
On Thu, Dec 05, 2019 at 09:01:50AM -0800, Thomas Garnier wrote:
> On Thu, Dec 5, 2019 at 1:04 AM Peter Zijlstra <peterz@infradead.org> wrote:
> > On Wed, Dec 04, 2019 at 04:09:41PM -0800, Thomas Garnier wrote:
> >
> > > @@ -1625,7 +1627,11 @@ first_nmi:
> > >       addq    $8, (%rsp)      /* Fix up RSP */
> > >       pushfq                  /* RFLAGS */
> > >       pushq   $__KERNEL_CS    /* CS */
> > > -     pushq   $1f             /* RIP */
> > > +     pushq   $0              /* Future return address */
> >
> > We're building an IRET frame, the IRET frame does not have a 'future
> > return address' field.
> 
> I assumed that's the target RIP after iretq.

It is. But it's still the (R)IP field of the IRET frame. Calling it
anything else is just confusing. The frame is 5 words: SS, (R)SP, (R)FLAGS,
CS, (R)IP.

> > > +     pushq   %rdx            /* Save RAX */
> > > +     leaq    1f(%rip), %rdx  /* RIP */
> >
> > nonsensical comment
> 
> That was the same comment from the push $1f that I changed.

Yes, but there it made sense since the PUSH actually created that field
of the frame, here it is nonsensical. What this instruction does is put
the address of the '1f' label into RDX, which is then stuck into the
(R)IP field on the next instruction.

> > > +     movq    %rdx, 8(%rsp)   /* Put 1f on return address */
> > > +     popq    %rdx            /* Restore RAX */
Thomas Garnier Dec. 6, 2019, 4:35 p.m. UTC | #4
On Fri, Dec 6, 2019 at 2:27 AM Peter Zijlstra <peterz@infradead.org> wrote:
>
> On Thu, Dec 05, 2019 at 09:01:50AM -0800, Thomas Garnier wrote:
> > On Thu, Dec 5, 2019 at 1:04 AM Peter Zijlstra <peterz@infradead.org> wrote:
> > > On Wed, Dec 04, 2019 at 04:09:41PM -0800, Thomas Garnier wrote:
> > >
> > > > @@ -1625,7 +1627,11 @@ first_nmi:
> > > >       addq    $8, (%rsp)      /* Fix up RSP */
> > > >       pushfq                  /* RFLAGS */
> > > >       pushq   $__KERNEL_CS    /* CS */
> > > > -     pushq   $1f             /* RIP */
> > > > +     pushq   $0              /* Future return address */
> > >
> > > We're building an IRET frame, the IRET frame does not have a 'future
> > > return address' field.
> >
> > I assumed that's the target RIP after iretq.
>
> It is. But it's still the (R)IP field of the IRET frame. Calling it
> anything else is just confusing. The frame is 5 words: SS, (R)SP, (R)FLAGS,
> CS, (R)IP.
>
> > > > +     pushq   %rdx            /* Save RAX */
> > > > +     leaq    1f(%rip), %rdx  /* RIP */
> > >
> > > nonsensical comment
> >
> > That was the same comment from the push $1f that I changed.
>
> Yes, but there it made sense since the PUSH actually created that field
> of the frame, here it is nonsensical. What this instruction does is put
> the address of the '1f' label into RDX, which is then stuck into the
> (R)IP field on the next instruction.

Got it, make sense. Thanks.

>
> > > > +     movq    %rdx, 8(%rsp)   /* Put 1f on return address */
> > > > +     popq    %rdx            /* Restore RAX */
Borislav Petkov Dec. 20, 2019, 4:05 p.m. UTC | #5
On Fri, Dec 06, 2019 at 08:35:09AM -0800, Thomas Garnier wrote:
> > Yes, but there it made sense since the PUSH actually created that field
> > of the frame, here it is nonsensical. What this instruction does is put
> > the address of the '1f' label into RDX, which is then stuck into the
> > (R)IP field on the next instruction.
> 
> Got it, make sense. Thanks.
> 
> >
> > > > > +     movq    %rdx, 8(%rsp)   /* Put 1f on return address */

And pls write it out as "put the address of the '1f' label into RDX"
instead of "Put 1f on return address" which could be misunderstood.

Thx.

Patch
diff mbox series

diff --git a/arch/x86/entry/entry_64.S b/arch/x86/entry/entry_64.S
index 76942cbd95a1..f14363625f4b 100644
--- a/arch/x86/entry/entry_64.S
+++ b/arch/x86/entry/entry_64.S
@@ -1329,7 +1329,8 @@  SYM_CODE_START_LOCAL(error_entry)
 	movl	%ecx, %eax			/* zero extend */
 	cmpq	%rax, RIP+8(%rsp)
 	je	.Lbstep_iret
-	cmpq	$.Lgs_change, RIP+8(%rsp)
+	leaq	.Lgs_change(%rip), %rcx
+	cmpq	%rcx, RIP+8(%rsp)
 	jne	.Lerror_entry_done_lfence
 
 	/*
@@ -1529,10 +1530,10 @@  SYM_CODE_START(nmi)
 	 * resume the outer NMI.
 	 */
 
-	movq	$repeat_nmi, %rdx
+	leaq	repeat_nmi(%rip), %rdx
 	cmpq	8(%rsp), %rdx
 	ja	1f
-	movq	$end_repeat_nmi, %rdx
+	leaq	end_repeat_nmi(%rip), %rdx
 	cmpq	8(%rsp), %rdx
 	ja	nested_nmi_out
 1:
@@ -1586,7 +1587,8 @@  nested_nmi:
 	pushq	%rdx
 	pushfq
 	pushq	$__KERNEL_CS
-	pushq	$repeat_nmi
+	leaq	repeat_nmi(%rip), %rdx
+	pushq	%rdx
 
 	/* Put stack back */
 	addq	$(6*8), %rsp
@@ -1625,7 +1627,11 @@  first_nmi:
 	addq	$8, (%rsp)	/* Fix up RSP */
 	pushfq			/* RFLAGS */
 	pushq	$__KERNEL_CS	/* CS */
-	pushq	$1f		/* RIP */
+	pushq	$0		/* Future return address */
+	pushq	%rdx		/* Save RAX */
+	leaq	1f(%rip), %rdx	/* RIP */
+	movq    %rdx, 8(%rsp)   /* Put 1f on return address */
+	popq	%rdx		/* Restore RAX */
 	iretq			/* continues at repeat_nmi below */
 	UNWIND_HINT_IRET_REGS
 1: