Re: [PATCH v2 07/10] x86/entry: Vastly simplify SYSENTER TF handling

[Borislav Petkov <bp@alien8.de>]

On Sat, Mar 05, 2016 at 09:52:20PM -0800, Andy Lutomirski wrote:
> Due to a blatant design error, SYSENTER doesn't clear TF.  As a result,
> if a user does SYSENTER with TF set, we will single-step through the
> kernel until something clears TF.  There is absolutely nothing we can
> do to prevent this short of turning off SYSENTER [1].
> 
> Simplify the handling considerably with two changes:
> 
> 1. We already sanitize EFLAGS in SYSENTER to clear NT and AC.  We can
>    add TF to that list of flags to sanitize with no overhead whatsoever.
> 
> 2. Teach do_debug to ignore single-step traps in the SYSENTER prologue.
> 
> That's all we need to do.
> 
> Don't get too excited -- our handling is still buggy on 32-bit
> kernels.  There's nothing wrong with the SYSENTER code itself, but
> the #DB prologue has a clever fixup for traps on the very first
> instruction of entry_SYSENTER_32, and the fixup doesn't work quite
> correctly.  The next two patches will fix that.
> 
> [1] We could probably prevent it by forcing BTF on at all times and
>     making sure we clear TF before any branches in the SYSENTER
>     code.  Needless to say, this is a bad idea.
> 
> Signed-off-by: Andy Lutomirski <luto@kernel.org>
> ---
>  arch/x86/entry/entry_32.S        | 42 ++++++++++++++++++++++----------
>  arch/x86/entry/entry_64_compat.S |  9 ++++++-
>  arch/x86/include/asm/proto.h     | 15 ++++++++++--
>  arch/x86/kernel/traps.c          | 52 +++++++++++++++++++++++++++++++++-------
>  4 files changed, 94 insertions(+), 24 deletions(-)
> 
> diff --git a/arch/x86/entry/entry_32.S b/arch/x86/entry/entry_32.S
> index a8c3424c3392..7700cf695d8c 100644
> --- a/arch/x86/entry/entry_32.S
> +++ b/arch/x86/entry/entry_32.S
> @@ -287,7 +287,26 @@ need_resched:
>  END(resume_kernel)
>  #endif
>  
> -	# SYSENTER  call handler stub
> +GLOBAL(__begin_SYSENTER_singlestep_region)
> +/*
> + * All code from here through __end_SYSENTER_singlestep_region is subject
> + * to being single-stepped if a user program sets TF and executes SYSENTER.
> + * There is absolutely nothing that we can do to prevent this from happening
> + * (thanks Intel!).  To keep our handling of this situation as simple as
> + * possible, we handle TF just like AC and NT, except that our #DB handler
> + * will ignore all of the single-step traps generated in this range.
> + */
> +
> +#ifdef CONFIG_XEN
> +/*
> + * Xen doesn't set %esp to be precisely what the normal SYSENTER
> + * entry point expects, so fix it up before using the normal path.
> + */
> +ENTRY(xen_sysenter_target)
> +	addl	$5*4, %esp			/* remove xen-provided frame */
> +	jmp	sysenter_past_esp
> +#endif
> +
>  ENTRY(entry_SYSENTER_32)
>  	movl	TSS_sysenter_sp0(%esp), %esp
>  sysenter_past_esp:

Can you do this below (ontop of your diff) and get rid of those
__{begin,end}_SYSENTER_singlestep_region and __end_entry_SYSENTER_compat
globals and use the entry_SYSENTER_{32|compat} symbols instead?

>From a quick scan, kallsyms can give you the symbol size too so that you
can compute where it ends:

readelf -a vmlinux | grep entry_SYSENTER
 55454: ffffffff8170aff0    99 FUNC    GLOBAL DEFAULT    1 entry_SYSENTER_compat
 62596: ffffffff8170b053     0 NOTYPE  GLOBAL DEFAULT    1 __end_entry_SYSENTER_comp

0xffffffff8170aff0 + 99 = 0xffffffff8170b053

---
Index: b/arch/x86/entry/entry_32.S
===================================================================
--- a/arch/x86/entry/entry_32.S	2016-03-06 17:47:10.059733163 +0100
+++ b/arch/x86/entry/entry_32.S	2016-03-06 17:46:52.235733410 +0100
@@ -297,18 +297,13 @@ GLOBAL(__begin_SYSENTER_singlestep_regio
  * will ignore all of the single-step traps generated in this range.
  */
 
+ENTRY(entry_SYSENTER_32)
 #ifdef CONFIG_XEN
-/*
- * Xen doesn't set %esp to be precisely what the normal SYSENTER
- * entry point expects, so fix it up before using the normal path.
- */
-ENTRY(xen_sysenter_target)
 	addl	$5*4, %esp			/* remove xen-provided frame */
 	jmp	sysenter_past_esp
-#endif
-
-ENTRY(entry_SYSENTER_32)
+#else
 	movl	TSS_sysenter_sp0(%esp), %esp
+#endif
 sysenter_past_esp:
 	pushl	$__USER_DS		/* pt_regs->ss */
 	pushl	%ebp			/* pt_regs->sp (stashed in bp) */

-- 
Regards/Gruss,
    Boris.

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