| Message ID | 947880c41ade688ff4836f665d0c9fcaa9bd1201.1593191971.git.luto@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | None | expand |
On 6/26/20 1:21 PM, Andy Lutomirski wrote: > The SYSENTER frame setup was nonsense. It worked by accident > because the normal code into which the Xen asm jumped > (entry_SYSENTER_32/compat) threw away SP without touching the stack. > entry_SYSENTER_compat was recently modified such that it relied on > having a valid stack pointer, so now the Xen asm needs to invoke it > with a valid stack. > > Fix it up like SYSCALL: use the Xen-provided frame and skip the bare > metal prologue. > > Cc: Boris Ostrovsky <boris.ostrovsky@oracle.com> > Cc: Juergen Gross <jgross@suse.com> > Cc: Stefano Stabellini <sstabellini@kernel.org> > Cc: xen-devel@lists.xenproject.org > Fixes: 1c3e5d3f60e2 ("x86/entry: Make entry_64_compat.S objtool clean") > Signed-off-by: Andy Lutomirski <luto@kernel.org> Reviewed-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>
On Fri, Jun 26, 2020 at 1:30 PM Andy Lutomirski <luto@kernel.org> wrote: > > The SYSENTER frame setup was nonsense. It worked by accident > because the normal code into which the Xen asm jumped > (entry_SYSENTER_32/compat) threw away SP without touching the stack. > entry_SYSENTER_compat was recently modified such that it relied on > having a valid stack pointer, so now the Xen asm needs to invoke it > with a valid stack. > > Fix it up like SYSCALL: use the Xen-provided frame and skip the bare > metal prologue. > > Cc: Boris Ostrovsky <boris.ostrovsky@oracle.com> > Cc: Juergen Gross <jgross@suse.com> > Cc: Stefano Stabellini <sstabellini@kernel.org> > Cc: xen-devel@lists.xenproject.org > Fixes: 1c3e5d3f60e2 ("x86/entry: Make entry_64_compat.S objtool clean") > Signed-off-by: Andy Lutomirski <luto@kernel.org> > --- > arch/x86/entry/entry_64_compat.S | 1 + > arch/x86/xen/xen-asm_64.S | 20 ++++++++++++++++---- > 2 files changed, 17 insertions(+), 4 deletions(-) > > diff --git a/arch/x86/entry/entry_64_compat.S b/arch/x86/entry/entry_64_compat.S > index 7b9d8150f652..381a6de7de9c 100644 > --- a/arch/x86/entry/entry_64_compat.S > +++ b/arch/x86/entry/entry_64_compat.S > @@ -79,6 +79,7 @@ SYM_CODE_START(entry_SYSENTER_compat) > pushfq /* pt_regs->flags (except IF = 0) */ > pushq $__USER32_CS /* pt_regs->cs */ > pushq $0 /* pt_regs->ip = 0 (placeholder) */ > +SYM_INNER_LABEL(entry_SYSENTER_compat_after_hwframe, SYM_L_GLOBAL) This skips over the section that truncates the syscall number to 32-bits. The comments present some doubt that it is actually necessary, but the Xen path shouldn't differ from native. That code should be moved after this new label. -- Brian Gerst
On Wed, Jul 1, 2020 at 8:42 AM Brian Gerst <brgerst@gmail.com> wrote: > > On Fri, Jun 26, 2020 at 1:30 PM Andy Lutomirski <luto@kernel.org> wrote: > > > > The SYSENTER frame setup was nonsense. It worked by accident > > because the normal code into which the Xen asm jumped > > (entry_SYSENTER_32/compat) threw away SP without touching the stack. > > entry_SYSENTER_compat was recently modified such that it relied on > > having a valid stack pointer, so now the Xen asm needs to invoke it > > with a valid stack. > > > > Fix it up like SYSCALL: use the Xen-provided frame and skip the bare > > metal prologue. > > > > Cc: Boris Ostrovsky <boris.ostrovsky@oracle.com> > > Cc: Juergen Gross <jgross@suse.com> > > Cc: Stefano Stabellini <sstabellini@kernel.org> > > Cc: xen-devel@lists.xenproject.org > > Fixes: 1c3e5d3f60e2 ("x86/entry: Make entry_64_compat.S objtool clean") > > Signed-off-by: Andy Lutomirski <luto@kernel.org> > > --- > > arch/x86/entry/entry_64_compat.S | 1 + > > arch/x86/xen/xen-asm_64.S | 20 ++++++++++++++++---- > > 2 files changed, 17 insertions(+), 4 deletions(-) > > > > diff --git a/arch/x86/entry/entry_64_compat.S b/arch/x86/entry/entry_64_compat.S > > index 7b9d8150f652..381a6de7de9c 100644 > > --- a/arch/x86/entry/entry_64_compat.S > > +++ b/arch/x86/entry/entry_64_compat.S > > @@ -79,6 +79,7 @@ SYM_CODE_START(entry_SYSENTER_compat) > > pushfq /* pt_regs->flags (except IF = 0) */ > > pushq $__USER32_CS /* pt_regs->cs */ > > pushq $0 /* pt_regs->ip = 0 (placeholder) */ > > +SYM_INNER_LABEL(entry_SYSENTER_compat_after_hwframe, SYM_L_GLOBAL) > > This skips over the section that truncates the syscall number to > 32-bits. The comments present some doubt that it is actually > necessary, but the Xen path shouldn't differ from native. That code > should be moved after this new label. Whoops. I thought I caught that myself, but apparently not. I'll fix it. > > -- > Brian Gerst
Andy Lutomirski <luto@kernel.org> writes: > On Wed, Jul 1, 2020 at 8:42 AM Brian Gerst <brgerst@gmail.com> wrote: > > On Fri, Jun 26, 2020 at 1:30 PM Andy Lutomirski <luto@kernel.org> wrote: >> > >> > The SYSENTER frame setup was nonsense. It worked by accident >> > because the normal code into which the Xen asm jumped >> > (entry_SYSENTER_32/compat) threw away SP without touching the stack. >> > entry_SYSENTER_compat was recently modified such that it relied on >> > having a valid stack pointer, so now the Xen asm needs to invoke it >> > with a valid stack. >> > >> > Fix it up like SYSCALL: use the Xen-provided frame and skip the bare >> > metal prologue. >> > >> > Cc: Boris Ostrovsky <boris.ostrovsky@oracle.com> >> > Cc: Juergen Gross <jgross@suse.com> >> > Cc: Stefano Stabellini <sstabellini@kernel.org> >> > Cc: xen-devel@lists.xenproject.org >> > Fixes: 1c3e5d3f60e2 ("x86/entry: Make entry_64_compat.S objtool clean") >> > Signed-off-by: Andy Lutomirski <luto@kernel.org> >> > --- >> > arch/x86/entry/entry_64_compat.S | 1 + >> > arch/x86/xen/xen-asm_64.S | 20 ++++++++++++++++---- >> > 2 files changed, 17 insertions(+), 4 deletions(-) >> > >> > diff --git a/arch/x86/entry/entry_64_compat.S b/arch/x86/entry/entry_64_compat.S >> > index 7b9d8150f652..381a6de7de9c 100644 >> > --- a/arch/x86/entry/entry_64_compat.S >> > +++ b/arch/x86/entry/entry_64_compat.S >> > @@ -79,6 +79,7 @@ SYM_CODE_START(entry_SYSENTER_compat) >> > pushfq /* pt_regs->flags (except IF = 0) */ >> > pushq $__USER32_CS /* pt_regs->cs */ >> > pushq $0 /* pt_regs->ip = 0 (placeholder) */ >> > +SYM_INNER_LABEL(entry_SYSENTER_compat_after_hwframe, SYM_L_GLOBAL) >> >> This skips over the section that truncates the syscall number to >> 32-bits. The comments present some doubt that it is actually >> necessary, but the Xen path shouldn't differ from native. That code >> should be moved after this new label. > > Whoops. I thought I caught that myself, but apparently not. I'll fix it. Darn. I already applied that lot. Can you please send a delta fix? Thanks, tglx
diff --git a/arch/x86/entry/entry_64_compat.S b/arch/x86/entry/entry_64_compat.S index 7b9d8150f652..381a6de7de9c 100644 --- a/arch/x86/entry/entry_64_compat.S +++ b/arch/x86/entry/entry_64_compat.S @@ -79,6 +79,7 @@ SYM_CODE_START(entry_SYSENTER_compat) pushfq /* pt_regs->flags (except IF = 0) */ pushq $__USER32_CS /* pt_regs->cs */ pushq $0 /* pt_regs->ip = 0 (placeholder) */ +SYM_INNER_LABEL(entry_SYSENTER_compat_after_hwframe, SYM_L_GLOBAL) pushq %rax /* pt_regs->orig_ax */ pushq %rdi /* pt_regs->di */ pushq %rsi /* pt_regs->si */ diff --git a/arch/x86/xen/xen-asm_64.S b/arch/x86/xen/xen-asm_64.S index 5d252aaeade8..e1e1c7eafa60 100644 --- a/arch/x86/xen/xen-asm_64.S +++ b/arch/x86/xen/xen-asm_64.S @@ -161,10 +161,22 @@ SYM_FUNC_END(xen_syscall32_target) /* 32-bit compat sysenter target */ SYM_FUNC_START(xen_sysenter_target) - mov 0*8(%rsp), %rcx - mov 1*8(%rsp), %r11 - mov 5*8(%rsp), %rsp - jmp entry_SYSENTER_compat + /* + * NB: Xen is polite and clears TF from EFLAGS for us. This means + * that we don't need to guard against single step exceptions here. + */ + popq %rcx + popq %r11 + + /* + * Neither Xen nor the kernel really knows what the old SS and + * CS were. The kernel expects __USER32_DS and __USER32_CS, so + * report those values even though Xen will guess its own values. + */ + movq $__USER32_DS, 4*8(%rsp) + movq $__USER32_CS, 1*8(%rsp) + + jmp entry_SYSENTER_compat_after_hwframe SYM_FUNC_END(xen_sysenter_target) #else /* !CONFIG_IA32_EMULATION */
The SYSENTER frame setup was nonsense. It worked by accident because the normal code into which the Xen asm jumped (entry_SYSENTER_32/compat) threw away SP without touching the stack. entry_SYSENTER_compat was recently modified such that it relied on having a valid stack pointer, so now the Xen asm needs to invoke it with a valid stack. Fix it up like SYSCALL: use the Xen-provided frame and skip the bare metal prologue. Cc: Boris Ostrovsky <boris.ostrovsky@oracle.com> Cc: Juergen Gross <jgross@suse.com> Cc: Stefano Stabellini <sstabellini@kernel.org> Cc: xen-devel@lists.xenproject.org Fixes: 1c3e5d3f60e2 ("x86/entry: Make entry_64_compat.S objtool clean") Signed-off-by: Andy Lutomirski <luto@kernel.org> --- arch/x86/entry/entry_64_compat.S | 1 + arch/x86/xen/xen-asm_64.S | 20 ++++++++++++++++---- 2 files changed, 17 insertions(+), 4 deletions(-)