| Message ID | 20191205000957.112719-5-thgarnie@chromium.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | x86: PIE support to extend KASLR randomization | expand |
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 >
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 > >
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 */
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 */
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.
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: