[PATCH v2 0/2] x86/entry: 32-bit facepalm fixes

[PATCH v2 0/2] x86/entry: 32-bit facepalm fixes

[Andy Lutomirski]

Fixes a (correct) warning and an oops.  Oddly, both seem to only
cause problems on some configurations.  The second one presumably
only survived my testing because I didn't test a threaded program.

The first patch is a bit ugly.  If that's a problem, suggestions
are welcome.

Changes from v1: No longer breaks 64-bit builds.

Andy Lutomirski (2):
  x86/entry/32: Fix entry_INT80_32 to expect interrupts to be on
  x86/entry/32: Fix FS and GS restore in opportunistic SYSEXIT

 arch/x86/entry/common.c          | 15 ++++++++++++---
 arch/x86/entry/entry_32.S        | 12 ++++++------
 arch/x86/entry/entry_64_compat.S |  2 +-
 3 files changed, 19 insertions(+), 10 deletions(-)

-- 
2.4.3

[PATCH v2 1/2] x86/entry/32: Fix entry_INT80_32 to expect interrupts to be on

[Andy Lutomirski]

When I rewrote entry_INT80_32, I thought that int80 was an interrupt
gate.  It's a trap gate.  *facepalm*

Thanks to Brian Gerst for pointing out that it's better to change
the entry code than to change the gate type.

Suggested-by: Brian Gerst <brgerst@gmail.com>
Reported-by: Borislav Petkov <bp@suse.de>
Fixes: 150ac78d63af ("x86/entry/32: Switch INT80 to the new C syscall path")
Signed-off-by: Andy Lutomirski <luto@kernel.org>
---
 arch/x86/entry/common.c          | 15 ++++++++++++---
 arch/x86/entry/entry_32.S        |  8 ++++----
 arch/x86/entry/entry_64_compat.S |  2 +-
 3 files changed, 17 insertions(+), 8 deletions(-)

diff --git a/arch/x86/entry/common.c b/arch/x86/entry/common.c
index b53e04d301a3..09afb3b6acbb 100644
--- a/arch/x86/entry/common.c
+++ b/arch/x86/entry/common.c
@@ -351,7 +351,14 @@ __visible inline void syscall_return_slowpath(struct pt_regs *regs)
  * in workloads that use it, and it's usually called from
  * do_fast_syscall_32, so forcibly inline it to improve performance.
  */
-static __always_inline void do_syscall_32_irqs_on(struct pt_regs *regs)
+#ifdef CONFIG_X86_32
+/* 32-bit kernels use a trap gate for int80, and the asm code calls here. */
+__visible
+#else
+/* 64-bit kernels use do_syscall_32_irqs_off instead. */
+static
+#endif
+__always_inline void do_syscall_32_irqs_on(struct pt_regs *regs)
 {
 	struct thread_info *ti = pt_regs_to_thread_info(regs);
 	unsigned int nr = (unsigned int)regs->orig_ax;
@@ -386,12 +393,14 @@ static __always_inline void do_syscall_32_irqs_on(struct pt_regs *regs)
 	syscall_return_slowpath(regs);
 }
 
-/* Handles int $0x80 */
-__visible void do_int80_syscall_32(struct pt_regs *regs)
+#ifdef CONFIG_X86_64
+/* Handles int $0x80 on 64-bit kernels */
+__visible void do_syscall_32_irqs_off(struct pt_regs *regs)
 {
 	local_irq_enable();
 	do_syscall_32_irqs_on(regs);
 }
+#endif
 
 /* Returns 0 to return using IRET or 1 to return using SYSEXIT/SYSRETL. */
 __visible long do_fast_syscall_32(struct pt_regs *regs)
diff --git a/arch/x86/entry/entry_32.S b/arch/x86/entry/entry_32.S
index 07a041b91af1..ba0dad937be9 100644
--- a/arch/x86/entry/entry_32.S
+++ b/arch/x86/entry/entry_32.S
@@ -346,13 +346,13 @@ ENTRY(entry_INT80_32)
 	SAVE_ALL pt_regs_ax=$-ENOSYS	/* save rest */
 
 	/*
-	 * User mode is traced as though IRQs are on, and the interrupt gate
-	 * turned them off.
+	 * User mode is traced as though IRQs are on.  Unlike the 64-bit
+	 * case, int80 is a trap gate on 32-bit kernels, so interrupts
+	 * are already on (unless user code is messing around with iopl).
 	 */
-	TRACE_IRQS_OFF
 
 	movl	%esp, %eax
-	call	do_int80_syscall_32
+	call	do_syscall_32_irqs_on
 .Lsyscall_32_done:
 
 restore_all:
diff --git a/arch/x86/entry/entry_64_compat.S b/arch/x86/entry/entry_64_compat.S
index 78becafe60d1..dd160e4e2ef5 100644
--- a/arch/x86/entry/entry_64_compat.S
+++ b/arch/x86/entry/entry_64_compat.S
@@ -303,7 +303,7 @@ ENTRY(entry_INT80_compat)
 	TRACE_IRQS_OFF
 
 	movq	%rsp, %rdi
-	call	do_int80_syscall_32
+	call	do_syscall_32_irqs_off
 .Lsyscall_32_done:
 
 	/* Go back to user mode. */
-- 
2.4.3

[PATCH v2 2/2] x86/entry/32: Fix FS and GS restore in opportunistic SYSEXIT

[Andy Lutomirski]

We either need to restore them before popping and thus changing ESP,
or we need to adjust the offsets.  The former is simpler.

Fixes: 5f310f739b4c x86/entry/32: ("Re-implement SYSENTER using the new C path")
Signed-off-by: Andy Lutomirski <luto@kernel.org>
---
 arch/x86/entry/entry_32.S | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/x86/entry/entry_32.S b/arch/x86/entry/entry_32.S
index ba0dad937be9..e3ede42d30e2 100644
--- a/arch/x86/entry/entry_32.S
+++ b/arch/x86/entry/entry_32.S
@@ -315,6 +315,8 @@ sysenter_past_esp:
 	TRACE_IRQS_ON			/* User mode traces as IRQs on. */
 	movl	PT_EIP(%esp), %edx	/* pt_regs->ip */
 	movl	PT_OLDESP(%esp), %ecx	/* pt_regs->sp */
+1:	mov	PT_FS(%esp), %fs
+	PTGS_TO_GS
 	popl	%ebx			/* pt_regs->bx */
 	popl	%eax			/* skip pt_regs->cx */
 	popl	%eax			/* skip pt_regs->dx */
@@ -322,8 +324,6 @@ sysenter_past_esp:
 	popl	%edi			/* pt_regs->di */
 	popl	%ebp			/* pt_regs->bp */
 	popl	%eax			/* pt_regs->ax */
-1:	mov	PT_FS(%esp), %fs
-	PTGS_TO_GS
 
 	/*
 	 * Return back to the vDSO, which will pop ecx and edx.
-- 
2.4.3

Re: [PATCH v2 1/2] x86/entry/32: Fix entry_INT80_32 to expect interrupts to be on

[Brian Gerst]

On Fri, Oct 16, 2015 at 6:42 PM, Andy Lutomirski <luto@kernel.org> wrote:
> When I rewrote entry_INT80_32, I thought that int80 was an interrupt
> gate.  It's a trap gate.  *facepalm*
>
> Thanks to Brian Gerst for pointing out that it's better to change
> the entry code than to change the gate type.
>
> Suggested-by: Brian Gerst <brgerst@gmail.com>
> Reported-by: Borislav Petkov <bp@suse.de>
> Fixes: 150ac78d63af ("x86/entry/32: Switch INT80 to the new C syscall path")
> Signed-off-by: Andy Lutomirski <luto@kernel.org>
> ---
>  arch/x86/entry/common.c          | 15 ++++++++++++---
>  arch/x86/entry/entry_32.S        |  8 ++++----
>  arch/x86/entry/entry_64_compat.S |  2 +-
>  3 files changed, 17 insertions(+), 8 deletions(-)
>
> diff --git a/arch/x86/entry/common.c b/arch/x86/entry/common.c
> index b53e04d301a3..09afb3b6acbb 100644
> --- a/arch/x86/entry/common.c
> +++ b/arch/x86/entry/common.c
> @@ -351,7 +351,14 @@ __visible inline void syscall_return_slowpath(struct pt_regs *regs)
>   * in workloads that use it, and it's usually called from
>   * do_fast_syscall_32, so forcibly inline it to improve performance.
>   */
> -static __always_inline void do_syscall_32_irqs_on(struct pt_regs *regs)
> +#ifdef CONFIG_X86_32
> +/* 32-bit kernels use a trap gate for int80, and the asm code calls here. */
> +__visible
> +#else
> +/* 64-bit kernels use do_syscall_32_irqs_off instead. */
> +static
> +#endif
> +__always_inline void do_syscall_32_irqs_on(struct pt_regs *regs)
>  {
>         struct thread_info *ti = pt_regs_to_thread_info(regs);
>         unsigned int nr = (unsigned int)regs->orig_ax;
> @@ -386,12 +393,14 @@ static __always_inline void do_syscall_32_irqs_on(struct pt_regs *regs)
>         syscall_return_slowpath(regs);
>  }
>
> -/* Handles int $0x80 */
> -__visible void do_int80_syscall_32(struct pt_regs *regs)
> +#ifdef CONFIG_X86_64
> +/* Handles int $0x80 on 64-bit kernels */
> +__visible void do_syscall_32_irqs_off(struct pt_regs *regs)
>  {
>         local_irq_enable();
>         do_syscall_32_irqs_on(regs);
>  }
> +#endif

This would be more readable if the STI were moved down into the asm
for 64-bit.  In fact, we should be re-enabling interrupts as early as
possible once the full kernel environment is set up (on the process
stack, NT clear, and after SWAPGS).  What was your reasoning for
moving it later?

--
Brian Gerst

Re: [PATCH v2 1/2] x86/entry/32: Fix entry_INT80_32 to expect interrupts to be on

[Andy Lutomirski]

On Oct 16, 2015 7:54 PM, "Brian Gerst" <brgerst@gmail.com> wrote:
>
> On Fri, Oct 16, 2015 at 6:42 PM, Andy Lutomirski <luto@kernel.org> wrote:
> > When I rewrote entry_INT80_32, I thought that int80 was an interrupt
> > gate.  It's a trap gate.  *facepalm*
> >
> > Thanks to Brian Gerst for pointing out that it's better to change
> > the entry code than to change the gate type.
> >
> > Suggested-by: Brian Gerst <brgerst@gmail.com>
> > Reported-by: Borislav Petkov <bp@suse.de>
> > Fixes: 150ac78d63af ("x86/entry/32: Switch INT80 to the new C syscall path")
> > Signed-off-by: Andy Lutomirski <luto@kernel.org>
> > ---
> >  arch/x86/entry/common.c          | 15 ++++++++++++---
> >  arch/x86/entry/entry_32.S        |  8 ++++----
> >  arch/x86/entry/entry_64_compat.S |  2 +-
> >  3 files changed, 17 insertions(+), 8 deletions(-)
> >
> > diff --git a/arch/x86/entry/common.c b/arch/x86/entry/common.c
> > index b53e04d301a3..09afb3b6acbb 100644
> > --- a/arch/x86/entry/common.c
> > +++ b/arch/x86/entry/common.c
> > @@ -351,7 +351,14 @@ __visible inline void syscall_return_slowpath(struct pt_regs *regs)
> >   * in workloads that use it, and it's usually called from
> >   * do_fast_syscall_32, so forcibly inline it to improve performance.
> >   */
> > -static __always_inline void do_syscall_32_irqs_on(struct pt_regs *regs)
> > +#ifdef CONFIG_X86_32
> > +/* 32-bit kernels use a trap gate for int80, and the asm code calls here. */
> > +__visible
> > +#else
> > +/* 64-bit kernels use do_syscall_32_irqs_off instead. */
> > +static
> > +#endif
> > +__always_inline void do_syscall_32_irqs_on(struct pt_regs *regs)
> >  {
> >         struct thread_info *ti = pt_regs_to_thread_info(regs);
> >         unsigned int nr = (unsigned int)regs->orig_ax;
> > @@ -386,12 +393,14 @@ static __always_inline void do_syscall_32_irqs_on(struct pt_regs *regs)
> >         syscall_return_slowpath(regs);
> >  }
> >
> > -/* Handles int $0x80 */
> > -__visible void do_int80_syscall_32(struct pt_regs *regs)
> > +#ifdef CONFIG_X86_64
> > +/* Handles int $0x80 on 64-bit kernels */
> > +__visible void do_syscall_32_irqs_off(struct pt_regs *regs)
> >  {
> >         local_irq_enable();
> >         do_syscall_32_irqs_on(regs);
> >  }
> > +#endif
>
> This would be more readable if the STI were moved down into the asm
> for 64-bit.  In fact, we should be re-enabling interrupts as early as
> possible once the full kernel environment is set up (on the process
> stack, NT clear, and after SWAPGS).  What was your reasoning for
> moving it later?

On x86_64, we have to use an interrupt gate because of swapgs, and for
context tracking, once I clean up the SYSCALL64 entry, I want to make
it all the way to user_exit with IRQs off.   There are nice
optimizations that become possible once user_exit is always called
with IRQs off, and there's another cleanup we can do when IRQs are no
longer possible in kernel mode with IRQs on.

For x86_32, we don't actually support context tracking, but we could.
So maybe we should suck up the ~3 cycles for int80 users and just use
an interrupt gate everywhere.

--Andy

Re: [PATCH v2 0/2] x86/entry: 32-bit facepalm fixes

[Borislav Petkov]

On Fri, Oct 16, 2015 at 03:42:53PM -0700, Andy Lutomirski wrote:
> Fixes a (correct) warning and an oops.  Oddly, both seem to only
> cause problems on some configurations.  The second one presumably
> only survived my testing because I didn't test a threaded program.
> 
> The first patch is a bit ugly.  If that's a problem, suggestions
> are welcome.
> 
> Changes from v1: No longer breaks 64-bit builds.
> 
> Andy Lutomirski (2):
>   x86/entry/32: Fix entry_INT80_32 to expect interrupts to be on
>   x86/entry/32: Fix FS and GS restore in opportunistic SYSEXIT
> 
>  arch/x86/entry/common.c          | 15 ++++++++++++---
>  arch/x86/entry/entry_32.S        | 12 ++++++------
>  arch/x86/entry/entry_64_compat.S |  2 +-
>  3 files changed, 19 insertions(+), 10 deletions(-)

Looks good.

Reported-and-tested-by: Borislav Petkov <bp@suse.de>

-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.

[tip:x86/asm] x86/entry/32: Fix entry_INT80_32() to expect interrupts to be on

[tip-bot for Andy Lutomirski]

Commit-ID:  657c1eea0019e80685a84cbb1919794243a187c9
Gitweb:     http://git.kernel.org/tip/657c1eea0019e80685a84cbb1919794243a187c9
Author:     Andy Lutomirski <luto@kernel.org>
AuthorDate: Fri, 16 Oct 2015 15:42:54 -0700
Committer:  Ingo Molnar <mingo@kernel.org>
CommitDate: Sun, 18 Oct 2015 12:11:16 +0200

x86/entry/32: Fix entry_INT80_32() to expect interrupts to be on

When I rewrote entry_INT80_32, I thought that int80 was an
interrupt gate.  It's a trap gate.  *facepalm*

Thanks to Brian Gerst for pointing out that it's better to
change the entry code than to change the gate type.

Suggested-by: Brian Gerst <brgerst@gmail.com>
Reported-and-tested-by: Borislav Petkov <bp@suse.de>
Signed-off-by: Andy Lutomirski <luto@kernel.org>
Cc: Andy Lutomirski <luto@amacapital.net>
Cc: Borislav Petkov <bp@alien8.de>
Cc: Denys Vlasenko <dvlasenk@redhat.com>
Cc: H. Peter Anvin <hpa@zytor.com>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
Fixes: 150ac78d63af ("x86/entry/32: Switch INT80 to the new C syscall path")
Link: http://lkml.kernel.org/r/dc09d9b574a5c1dcca996847875c73f8341ce0ad.1445035014.git.luto@kernel.org
Signed-off-by: Ingo Molnar <mingo@kernel.org>
---
 arch/x86/entry/common.c          | 15 ++++++++++++---
 arch/x86/entry/entry_32.S        |  8 ++++----
 arch/x86/entry/entry_64_compat.S |  2 +-
 3 files changed, 17 insertions(+), 8 deletions(-)

diff --git a/arch/x86/entry/common.c b/arch/x86/entry/common.c
index b53e04d..a89fdbc 100644
--- a/arch/x86/entry/common.c
+++ b/arch/x86/entry/common.c
@@ -351,7 +351,14 @@ __visible inline void syscall_return_slowpath(struct pt_regs *regs)
  * in workloads that use it, and it's usually called from
  * do_fast_syscall_32, so forcibly inline it to improve performance.
  */
-static __always_inline void do_syscall_32_irqs_on(struct pt_regs *regs)
+#ifdef CONFIG_X86_32
+/* 32-bit kernels use a trap gate for INT80, and the asm code calls here. */
+__visible
+#else
+/* 64-bit kernels use do_syscall_32_irqs_off() instead. */
+static
+#endif
+__always_inline void do_syscall_32_irqs_on(struct pt_regs *regs)
 {
 	struct thread_info *ti = pt_regs_to_thread_info(regs);
 	unsigned int nr = (unsigned int)regs->orig_ax;
@@ -386,12 +393,14 @@ static __always_inline void do_syscall_32_irqs_on(struct pt_regs *regs)
 	syscall_return_slowpath(regs);
 }
 
-/* Handles int $0x80 */
-__visible void do_int80_syscall_32(struct pt_regs *regs)
+#ifdef CONFIG_X86_64
+/* Handles INT80 on 64-bit kernels */
+__visible void do_syscall_32_irqs_off(struct pt_regs *regs)
 {
 	local_irq_enable();
 	do_syscall_32_irqs_on(regs);
 }
+#endif
 
 /* Returns 0 to return using IRET or 1 to return using SYSEXIT/SYSRETL. */
 __visible long do_fast_syscall_32(struct pt_regs *regs)
diff --git a/arch/x86/entry/entry_32.S b/arch/x86/entry/entry_32.S
index c1c7c63..4f97f49 100644
--- a/arch/x86/entry/entry_32.S
+++ b/arch/x86/entry/entry_32.S
@@ -345,13 +345,13 @@ ENTRY(entry_INT80_32)
 	SAVE_ALL pt_regs_ax=$-ENOSYS	/* save rest */
 
 	/*
-	 * User mode is traced as though IRQs are on, and the interrupt gate
-	 * turned them off.
+	 * User mode is traced as though IRQs are on.  Unlike the 64-bit
+	 * case, INT80 is a trap gate on 32-bit kernels, so interrupts
+	 * are already on (unless user code is messing around with iopl).
 	 */
-	TRACE_IRQS_OFF
 
 	movl	%esp, %eax
-	call	do_int80_syscall_32
+	call	do_syscall_32_irqs_on
 .Lsyscall_32_done:
 
 restore_all:
diff --git a/arch/x86/entry/entry_64_compat.S b/arch/x86/entry/entry_64_compat.S
index 92b0b27..c320183 100644
--- a/arch/x86/entry/entry_64_compat.S
+++ b/arch/x86/entry/entry_64_compat.S
@@ -303,7 +303,7 @@ ENTRY(entry_INT80_compat)
 	TRACE_IRQS_OFF
 
 	movq	%rsp, %rdi
-	call	do_int80_syscall_32
+	call	do_syscall_32_irqs_off
 .Lsyscall_32_done:
 
 	/* Go back to user mode. */

[tip:x86/asm] x86/entry/32: Fix FS and GS restore in opportunistic SYSEXIT

[tip-bot for Andy Lutomirski]

Commit-ID:  3bd29515d1cad26fa85a1a9b442de8816c1f5c54
Gitweb:     http://git.kernel.org/tip/3bd29515d1cad26fa85a1a9b442de8816c1f5c54
Author:     Andy Lutomirski <luto@kernel.org>
AuthorDate: Fri, 16 Oct 2015 15:42:55 -0700
Committer:  Ingo Molnar <mingo@kernel.org>
CommitDate: Sun, 18 Oct 2015 12:11:16 +0200

x86/entry/32: Fix FS and GS restore in opportunistic SYSEXIT

We either need to restore them before popping and thus changing
ESP, or we need to adjust the offsets.  The former is simpler.

Reported-and-tested-by: Borislav Petkov <bp@suse.de>
Signed-off-by: Andy Lutomirski <luto@kernel.org>
Cc: Andy Lutomirski <luto@amacapital.net>
Cc: Borislav Petkov <bp@alien8.de>
Cc: Brian Gerst <brgerst@gmail.com>
Cc: Denys Vlasenko <dvlasenk@redhat.com>
Cc: H. Peter Anvin <hpa@zytor.com>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
Fixes: 5f310f739b4c x86/entry/32: ("Re-implement SYSENTER using the new C path")
Link: http://lkml.kernel.org/r/461e5c7d8fa3821529893a4893ac9c4bc37f9e17.1445035014.git.luto@kernel.org
Signed-off-by: Ingo Molnar <mingo@kernel.org>
---
 arch/x86/entry/entry_32.S | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/x86/entry/entry_32.S b/arch/x86/entry/entry_32.S
index 4f97f49..3eb572e 100644
--- a/arch/x86/entry/entry_32.S
+++ b/arch/x86/entry/entry_32.S
@@ -315,14 +315,14 @@ sysenter_past_esp:
 	TRACE_IRQS_ON			/* User mode traces as IRQs on. */
 	movl	PT_EIP(%esp), %edx	/* pt_regs->ip */
 	movl	PT_OLDESP(%esp), %ecx	/* pt_regs->sp */
+1:	mov	PT_FS(%esp), %fs
+	PTGS_TO_GS
 	popl	%ebx			/* pt_regs->bx */
 	addl	$2*4, %esp		/* skip pt_regs->cx and pt_regs->dx */
 	popl	%esi			/* pt_regs->si */
 	popl	%edi			/* pt_regs->di */
 	popl	%ebp			/* pt_regs->bp */
 	popl	%eax			/* pt_regs->ax */
-1:	mov	PT_FS(%esp), %fs
-	PTGS_TO_GS
 
 	/*
 	 * Return back to the vDSO, which will pop ecx and edx.

Re: [tip:x86/asm] x86/entry/32: Fix entry_INT80_32() to expect interrupts to be on

[Andy Lutomirski]

On Oct 18, 2015 3:16 AM, "tip-bot for Andy Lutomirski" <tipbot@zytor.com> wrote:
>
> Commit-ID:  657c1eea0019e80685a84cbb1919794243a187c9
> Gitweb:     http://git.kernel.org/tip/657c1eea0019e80685a84cbb1919794243a187c9
> Author:     Andy Lutomirski <luto@kernel.org>
> AuthorDate: Fri, 16 Oct 2015 15:42:54 -0700
> Committer:  Ingo Molnar <mingo@kernel.org>
> CommitDate: Sun, 18 Oct 2015 12:11:16 +0200
>
> x86/entry/32: Fix entry_INT80_32() to expect interrupts to be on

FWIW, I may want to revert this down the road, but it's fine for now.

--Andy