linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 0/2] x86: A few random cleanups
@ 2022-04-19 20:41 Peter Zijlstra
  2022-04-19 20:41 ` [PATCH 1/2] x86: Simplify Retpoline thunk Peter Zijlstra
  2022-04-19 20:41 ` [PATCH 2/2] x86,entry: Use PUSH_AND_CLEAR_REGS for compat Peter Zijlstra
  0 siblings, 2 replies; 12+ messages in thread
From: Peter Zijlstra @ 2022-04-19 20:41 UTC (permalink / raw)
  To: x86, jpoimboe, brgerst, jiangshanlai, Andrew.Cooper3; +Cc: linux-kernel, peterz




^ permalink raw reply	[flat|nested] 12+ messages in thread

* [PATCH 1/2] x86: Simplify Retpoline thunk
  2022-04-19 20:41 [PATCH 0/2] x86: A few random cleanups Peter Zijlstra
@ 2022-04-19 20:41 ` Peter Zijlstra
  2022-04-20 15:27   ` Josh Poimboeuf
  2022-04-19 20:41 ` [PATCH 2/2] x86,entry: Use PUSH_AND_CLEAR_REGS for compat Peter Zijlstra
  1 sibling, 1 reply; 12+ messages in thread
From: Peter Zijlstra @ 2022-04-19 20:41 UTC (permalink / raw)
  To: x86, jpoimboe, brgerst, jiangshanlai, Andrew.Cooper3; +Cc: linux-kernel, peterz

Now that we rewrite all the indirect call sites, per commit:

  750850090081 ("x86/alternative: Implement .retpoline_sites support")

it doesn't make sense to have the retpoline thunks be an ALTERNATIVE_2
that still includes a 'naked' indirect jump.

(this accidentally 'defunnels' i386 by going back to full retpolines)

Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
 arch/x86/lib/retpoline.S |    6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

--- a/arch/x86/lib/retpoline.S
+++ b/arch/x86/lib/retpoline.S
@@ -32,9 +32,9 @@
 SYM_INNER_LABEL(__x86_indirect_thunk_\reg, SYM_L_GLOBAL)
 	UNWIND_HINT_EMPTY
 
-	ALTERNATIVE_2 __stringify(ANNOTATE_RETPOLINE_SAFE; jmp *%\reg), \
-		      __stringify(RETPOLINE \reg), X86_FEATURE_RETPOLINE, \
-		      __stringify(lfence; ANNOTATE_RETPOLINE_SAFE; jmp *%\reg; int3), X86_FEATURE_RETPOLINE_LFENCE
+	ALTERNATIVE __stringify(RETPOLINE \reg), \
+		    __stringify(lfence; ANNOTATE_RETPOLINE_SAFE; jmp *%\reg; int3), \
+		    X86_FEATURE_RETPOLINE_LFENCE
 
 .endm
 



^ permalink raw reply	[flat|nested] 12+ messages in thread

* [PATCH 2/2] x86,entry: Use PUSH_AND_CLEAR_REGS for compat
  2022-04-19 20:41 [PATCH 0/2] x86: A few random cleanups Peter Zijlstra
  2022-04-19 20:41 ` [PATCH 1/2] x86: Simplify Retpoline thunk Peter Zijlstra
@ 2022-04-19 20:41 ` Peter Zijlstra
  2022-04-20  3:21   ` Josh Poimboeuf
                     ` (2 more replies)
  1 sibling, 3 replies; 12+ messages in thread
From: Peter Zijlstra @ 2022-04-19 20:41 UTC (permalink / raw)
  To: x86, jpoimboe, brgerst, jiangshanlai, Andrew.Cooper3; +Cc: linux-kernel, peterz

Since the upper regs don't exist for ia32 code, preserving them
doesn't hurt and it simplifies the code.

Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
 arch/x86/entry/calling.h         |    9 ++--
 arch/x86/entry/entry_64_compat.S |   87 +--------------------------------------
 2 files changed, 8 insertions(+), 88 deletions(-)

--- a/arch/x86/entry/calling.h
+++ b/arch/x86/entry/calling.h
@@ -63,13 +63,15 @@ For 32-bit we have the following convent
  * for assembly code:
  */
 
-.macro PUSH_REGS rdx=%rdx rax=%rax save_ret=0
+.macro PUSH_REGS rdx=%rdx rax=%rax save_ret=0 save_rdi=1
 	.if \save_ret
 	pushq	%rsi		/* pt_regs->si */
 	movq	8(%rsp), %rsi	/* temporarily store the return address in %rsi */
 	movq	%rdi, 8(%rsp)	/* pt_regs->di (overwriting original return address) */
 	.else
+	.if \save_rdi
 	pushq   %rdi		/* pt_regs->di */
+	.endif
 	pushq   %rsi		/* pt_regs->si */
 	.endif
 	pushq	\rdx		/* pt_regs->dx */
@@ -111,11 +113,10 @@ For 32-bit we have the following convent
 	xorl	%r13d, %r13d	/* nospec r13 */
 	xorl	%r14d, %r14d	/* nospec r14 */
 	xorl	%r15d, %r15d	/* nospec r15 */
-
 .endm
 
-.macro PUSH_AND_CLEAR_REGS rdx=%rdx rax=%rax save_ret=0
-	PUSH_REGS rdx=\rdx, rax=\rax, save_ret=\save_ret
+.macro PUSH_AND_CLEAR_REGS rdx=%rdx rax=%rax save_ret=0 save_rdi=1
+	PUSH_REGS rdx=\rdx, rax=\rax, save_ret=\save_ret save_rdi=\save_rdi
 	CLEAR_REGS
 .endm
 
--- a/arch/x86/entry/entry_64_compat.S
+++ b/arch/x86/entry/entry_64_compat.S
@@ -83,32 +83,7 @@ SYM_INNER_LABEL(entry_SYSENTER_compat_af
 	movl	%eax, %eax
 
 	pushq	%rax			/* pt_regs->orig_ax */
-	pushq	%rdi			/* pt_regs->di */
-	pushq	%rsi			/* pt_regs->si */
-	pushq	%rdx			/* pt_regs->dx */
-	pushq	%rcx			/* pt_regs->cx */
-	pushq	$-ENOSYS		/* pt_regs->ax */
-	pushq   $0			/* pt_regs->r8  = 0 */
-	xorl	%r8d, %r8d		/* nospec   r8 */
-	pushq   $0			/* pt_regs->r9  = 0 */
-	xorl	%r9d, %r9d		/* nospec   r9 */
-	pushq   $0			/* pt_regs->r10 = 0 */
-	xorl	%r10d, %r10d		/* nospec   r10 */
-	pushq   $0			/* pt_regs->r11 = 0 */
-	xorl	%r11d, %r11d		/* nospec   r11 */
-	pushq   %rbx                    /* pt_regs->rbx */
-	xorl	%ebx, %ebx		/* nospec   rbx */
-	pushq   %rbp                    /* pt_regs->rbp (will be overwritten) */
-	xorl	%ebp, %ebp		/* nospec   rbp */
-	pushq   $0			/* pt_regs->r12 = 0 */
-	xorl	%r12d, %r12d		/* nospec   r12 */
-	pushq   $0			/* pt_regs->r13 = 0 */
-	xorl	%r13d, %r13d		/* nospec   r13 */
-	pushq   $0			/* pt_regs->r14 = 0 */
-	xorl	%r14d, %r14d		/* nospec   r14 */
-	pushq   $0			/* pt_regs->r15 = 0 */
-	xorl	%r15d, %r15d		/* nospec   r15 */
-
+	PUSH_AND_CLEAR_REGS rax=$-ENOSYS
 	UNWIND_HINT_REGS
 
 	cld
@@ -225,35 +200,7 @@ SYM_INNER_LABEL(entry_SYSCALL_compat_saf
 SYM_INNER_LABEL(entry_SYSCALL_compat_after_hwframe, SYM_L_GLOBAL)
 	movl	%eax, %eax		/* discard orig_ax high bits */
 	pushq	%rax			/* pt_regs->orig_ax */
-	pushq	%rdi			/* pt_regs->di */
-	pushq	%rsi			/* pt_regs->si */
-	xorl	%esi, %esi		/* nospec   si */
-	pushq	%rdx			/* pt_regs->dx */
-	xorl	%edx, %edx		/* nospec   dx */
-	pushq	%rbp			/* pt_regs->cx (stashed in bp) */
-	xorl	%ecx, %ecx		/* nospec   cx */
-	pushq	$-ENOSYS		/* pt_regs->ax */
-	pushq   $0			/* pt_regs->r8  = 0 */
-	xorl	%r8d, %r8d		/* nospec   r8 */
-	pushq   $0			/* pt_regs->r9  = 0 */
-	xorl	%r9d, %r9d		/* nospec   r9 */
-	pushq   $0			/* pt_regs->r10 = 0 */
-	xorl	%r10d, %r10d		/* nospec   r10 */
-	pushq   $0			/* pt_regs->r11 = 0 */
-	xorl	%r11d, %r11d		/* nospec   r11 */
-	pushq   %rbx                    /* pt_regs->rbx */
-	xorl	%ebx, %ebx		/* nospec   rbx */
-	pushq   %rbp                    /* pt_regs->rbp (will be overwritten) */
-	xorl	%ebp, %ebp		/* nospec   rbp */
-	pushq   $0			/* pt_regs->r12 = 0 */
-	xorl	%r12d, %r12d		/* nospec   r12 */
-	pushq   $0			/* pt_regs->r13 = 0 */
-	xorl	%r13d, %r13d		/* nospec   r13 */
-	pushq   $0			/* pt_regs->r14 = 0 */
-	xorl	%r14d, %r14d		/* nospec   r14 */
-	pushq   $0			/* pt_regs->r15 = 0 */
-	xorl	%r15d, %r15d		/* nospec   r15 */
-
+	PUSH_AND_CLEAR_REGS rax=$-ENOSYS
 	UNWIND_HINT_REGS
 
 	movq	%rsp, %rdi
@@ -381,35 +328,7 @@ SYM_CODE_START(entry_INT80_compat)
 	pushq	1*8(%rdi)		/* regs->orig_ax */
 	pushq	(%rdi)			/* pt_regs->di */
 .Lint80_keep_stack:
-
-	pushq	%rsi			/* pt_regs->si */
-	xorl	%esi, %esi		/* nospec   si */
-	pushq	%rdx			/* pt_regs->dx */
-	xorl	%edx, %edx		/* nospec   dx */
-	pushq	%rcx			/* pt_regs->cx */
-	xorl	%ecx, %ecx		/* nospec   cx */
-	pushq	$-ENOSYS		/* pt_regs->ax */
-	pushq   %r8			/* pt_regs->r8 */
-	xorl	%r8d, %r8d		/* nospec   r8 */
-	pushq   %r9			/* pt_regs->r9 */
-	xorl	%r9d, %r9d		/* nospec   r9 */
-	pushq   %r10			/* pt_regs->r10*/
-	xorl	%r10d, %r10d		/* nospec   r10 */
-	pushq   %r11			/* pt_regs->r11 */
-	xorl	%r11d, %r11d		/* nospec   r11 */
-	pushq   %rbx                    /* pt_regs->rbx */
-	xorl	%ebx, %ebx		/* nospec   rbx */
-	pushq   %rbp                    /* pt_regs->rbp */
-	xorl	%ebp, %ebp		/* nospec   rbp */
-	pushq   %r12                    /* pt_regs->r12 */
-	xorl	%r12d, %r12d		/* nospec   r12 */
-	pushq   %r13                    /* pt_regs->r13 */
-	xorl	%r13d, %r13d		/* nospec   r13 */
-	pushq   %r14                    /* pt_regs->r14 */
-	xorl	%r14d, %r14d		/* nospec   r14 */
-	pushq   %r15                    /* pt_regs->r15 */
-	xorl	%r15d, %r15d		/* nospec   r15 */
-
+	PUSH_AND_CLEAR_REGS save_rdi=0
 	UNWIND_HINT_REGS
 
 	cld



^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 2/2] x86,entry: Use PUSH_AND_CLEAR_REGS for compat
  2022-04-19 20:41 ` [PATCH 2/2] x86,entry: Use PUSH_AND_CLEAR_REGS for compat Peter Zijlstra
@ 2022-04-20  3:21   ` Josh Poimboeuf
  2022-04-20  7:07     ` Peter Zijlstra
  2022-04-20  4:29   ` Lai Jiangshan
  2022-04-20 14:26   ` Andy Lutomirski
  2 siblings, 1 reply; 12+ messages in thread
From: Josh Poimboeuf @ 2022-04-20  3:21 UTC (permalink / raw)
  To: Peter Zijlstra
  Cc: x86, brgerst, jiangshanlai, Andrew.Cooper3, linux-kernel,
	Andy Lutomirski

On Tue, Apr 19, 2022 at 10:41:11PM +0200, Peter Zijlstra wrote:
> Since the upper regs don't exist for ia32 code, preserving them
> doesn't hurt and it simplifies the code.

But an attacker can still control those registers, so clearing them on
the stack is better, as it reduces user control over the kernel stack.

64-bit syscalls *do* have to save those registers to the stack, so
whether it truly matters if compat mode is made equally insecure, I
can't say.  But without evidence to the contrary, my feeling is that we
should err on the side of caution.

-- 
Josh


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 2/2] x86,entry: Use PUSH_AND_CLEAR_REGS for compat
  2022-04-19 20:41 ` [PATCH 2/2] x86,entry: Use PUSH_AND_CLEAR_REGS for compat Peter Zijlstra
  2022-04-20  3:21   ` Josh Poimboeuf
@ 2022-04-20  4:29   ` Lai Jiangshan
  2022-04-20 14:26   ` Andy Lutomirski
  2 siblings, 0 replies; 12+ messages in thread
From: Lai Jiangshan @ 2022-04-20  4:29 UTC (permalink / raw)
  To: Peter Zijlstra; +Cc: X86 ML, Josh Poimboeuf, Brian Gerst, Andrew Cooper, LKML

On Wed, Apr 20, 2022 at 4:53 AM Peter Zijlstra <peterz@infradead.org> wrote:
>
> Since the upper regs don't exist for ia32 code, preserving them
> doesn't hurt and it simplifies the code.

In entry_INT80_compat(), the upper regs need to be preserved on the
stack which makes the code identical to the macro idtentry except the
special prolog.

So reusing idtentry simplifiers more.

I don't ask to remove the change to entry_INT80_compat().

But I don't think the change to entry_INT80_compat()
fit the changelog.

I think it is better to split this patch into two.

The first one contains the change to entry_SYSENTER_compat()
and entry_SYSCALL_compat() with current changelog.

The second one contains the change to entry_INT80_compat()
and PUSH_AND_CLEAR_REGS with changelog saying it simplifies
entry_SYSENTER_compat().

My patchset can do the simplification by using idtentry
and remove save_rdi from PUSH_AND_CLEAR_REGS on top of it.

Thanks
Lai

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 2/2] x86,entry: Use PUSH_AND_CLEAR_REGS for compat
  2022-04-20  3:21   ` Josh Poimboeuf
@ 2022-04-20  7:07     ` Peter Zijlstra
  2022-04-20 14:58       ` Josh Poimboeuf
  0 siblings, 1 reply; 12+ messages in thread
From: Peter Zijlstra @ 2022-04-20  7:07 UTC (permalink / raw)
  To: Josh Poimboeuf
  Cc: x86, brgerst, jiangshanlai, Andrew.Cooper3, linux-kernel,
	Andy Lutomirski

On Tue, Apr 19, 2022 at 08:21:23PM -0700, Josh Poimboeuf wrote:
> On Tue, Apr 19, 2022 at 10:41:11PM +0200, Peter Zijlstra wrote:
> > Since the upper regs don't exist for ia32 code, preserving them
> > doesn't hurt and it simplifies the code.
> 
> But an attacker can still control those registers, so clearing them on
> the stack is better, as it reduces user control over the kernel stack.
> 
> 64-bit syscalls *do* have to save those registers to the stack, so
> whether it truly matters if compat mode is made equally insecure, I
> can't say.  But without evidence to the contrary, my feeling is that we
> should err on the side of caution.

Right, so earlier Brian said simpler might be better, and I figured I'd
try to see if I could make that stick, because I too like simpler ;-)

Also, since int80 already has to do this, attackers already have their
attack surface.

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 2/2] x86,entry: Use PUSH_AND_CLEAR_REGS for compat
  2022-04-19 20:41 ` [PATCH 2/2] x86,entry: Use PUSH_AND_CLEAR_REGS for compat Peter Zijlstra
  2022-04-20  3:21   ` Josh Poimboeuf
  2022-04-20  4:29   ` Lai Jiangshan
@ 2022-04-20 14:26   ` Andy Lutomirski
  2022-04-20 14:51     ` Peter Zijlstra
  2 siblings, 1 reply; 12+ messages in thread
From: Andy Lutomirski @ 2022-04-20 14:26 UTC (permalink / raw)
  To: Peter Zijlstra (Intel),
	the arch/x86 maintainers, Josh Poimboeuf, Brian Gerst,
	Lai Jiangshan, Andrew Cooper
  Cc: Linux Kernel Mailing List



On Tue, Apr 19, 2022, at 1:41 PM, Peter Zijlstra wrote:
> Since the upper regs don't exist for ia32 code, preserving them
> doesn't hurt and it simplifies the code.

They exist for compat code, though, and should be preserved for ABI purposes. Programs that do int $0x80 in 64-bit code do exist.

>
> Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
> ---
>  arch/x86/entry/calling.h         |    9 ++--
>  arch/x86/entry/entry_64_compat.S |   87 +--------------------------------------
>  2 files changed, 8 insertions(+), 88 deletions(-)
>
> --- a/arch/x86/entry/calling.h
> +++ b/arch/x86/entry/calling.h
> @@ -63,13 +63,15 @@ For 32-bit we have the following convent
>   * for assembly code:
>   */
> 
> -.macro PUSH_REGS rdx=%rdx rax=%rax save_ret=0
> +.macro PUSH_REGS rdx=%rdx rax=%rax save_ret=0 save_rdi=1
>  	.if \save_ret
>  	pushq	%rsi		/* pt_regs->si */
>  	movq	8(%rsp), %rsi	/* temporarily store the return address in %rsi */
>  	movq	%rdi, 8(%rsp)	/* pt_regs->di (overwriting original return address) */
>  	.else
> +	.if \save_rdi
>  	pushq   %rdi		/* pt_regs->di */
> +	.endif
>  	pushq   %rsi		/* pt_regs->si */
>  	.endif
>  	pushq	\rdx		/* pt_regs->dx */
> @@ -111,11 +113,10 @@ For 32-bit we have the following convent
>  	xorl	%r13d, %r13d	/* nospec r13 */
>  	xorl	%r14d, %r14d	/* nospec r14 */
>  	xorl	%r15d, %r15d	/* nospec r15 */
> -
>  .endm
> 
> -.macro PUSH_AND_CLEAR_REGS rdx=%rdx rax=%rax save_ret=0
> -	PUSH_REGS rdx=\rdx, rax=\rax, save_ret=\save_ret
> +.macro PUSH_AND_CLEAR_REGS rdx=%rdx rax=%rax save_ret=0 save_rdi=1
> +	PUSH_REGS rdx=\rdx, rax=\rax, save_ret=\save_ret save_rdi=\save_rdi
>  	CLEAR_REGS
>  .endm
> 
> --- a/arch/x86/entry/entry_64_compat.S
> +++ b/arch/x86/entry/entry_64_compat.S
> @@ -83,32 +83,7 @@ SYM_INNER_LABEL(entry_SYSENTER_compat_af
>  	movl	%eax, %eax
> 
>  	pushq	%rax			/* pt_regs->orig_ax */
> -	pushq	%rdi			/* pt_regs->di */
> -	pushq	%rsi			/* pt_regs->si */
> -	pushq	%rdx			/* pt_regs->dx */
> -	pushq	%rcx			/* pt_regs->cx */
> -	pushq	$-ENOSYS		/* pt_regs->ax */
> -	pushq   $0			/* pt_regs->r8  = 0 */
> -	xorl	%r8d, %r8d		/* nospec   r8 */
> -	pushq   $0			/* pt_regs->r9  = 0 */
> -	xorl	%r9d, %r9d		/* nospec   r9 */
> -	pushq   $0			/* pt_regs->r10 = 0 */
> -	xorl	%r10d, %r10d		/* nospec   r10 */
> -	pushq   $0			/* pt_regs->r11 = 0 */
> -	xorl	%r11d, %r11d		/* nospec   r11 */
> -	pushq   %rbx                    /* pt_regs->rbx */
> -	xorl	%ebx, %ebx		/* nospec   rbx */
> -	pushq   %rbp                    /* pt_regs->rbp (will be overwritten) */
> -	xorl	%ebp, %ebp		/* nospec   rbp */
> -	pushq   $0			/* pt_regs->r12 = 0 */
> -	xorl	%r12d, %r12d		/* nospec   r12 */
> -	pushq   $0			/* pt_regs->r13 = 0 */
> -	xorl	%r13d, %r13d		/* nospec   r13 */
> -	pushq   $0			/* pt_regs->r14 = 0 */
> -	xorl	%r14d, %r14d		/* nospec   r14 */
> -	pushq   $0			/* pt_regs->r15 = 0 */
> -	xorl	%r15d, %r15d		/* nospec   r15 */
> -
> +	PUSH_AND_CLEAR_REGS rax=$-ENOSYS
>  	UNWIND_HINT_REGS
> 
>  	cld
> @@ -225,35 +200,7 @@ SYM_INNER_LABEL(entry_SYSCALL_compat_saf
>  SYM_INNER_LABEL(entry_SYSCALL_compat_after_hwframe, SYM_L_GLOBAL)
>  	movl	%eax, %eax		/* discard orig_ax high bits */
>  	pushq	%rax			/* pt_regs->orig_ax */
> -	pushq	%rdi			/* pt_regs->di */
> -	pushq	%rsi			/* pt_regs->si */
> -	xorl	%esi, %esi		/* nospec   si */
> -	pushq	%rdx			/* pt_regs->dx */
> -	xorl	%edx, %edx		/* nospec   dx */
> -	pushq	%rbp			/* pt_regs->cx (stashed in bp) */
> -	xorl	%ecx, %ecx		/* nospec   cx */
> -	pushq	$-ENOSYS		/* pt_regs->ax */
> -	pushq   $0			/* pt_regs->r8  = 0 */
> -	xorl	%r8d, %r8d		/* nospec   r8 */
> -	pushq   $0			/* pt_regs->r9  = 0 */
> -	xorl	%r9d, %r9d		/* nospec   r9 */
> -	pushq   $0			/* pt_regs->r10 = 0 */
> -	xorl	%r10d, %r10d		/* nospec   r10 */
> -	pushq   $0			/* pt_regs->r11 = 0 */
> -	xorl	%r11d, %r11d		/* nospec   r11 */
> -	pushq   %rbx                    /* pt_regs->rbx */
> -	xorl	%ebx, %ebx		/* nospec   rbx */
> -	pushq   %rbp                    /* pt_regs->rbp (will be overwritten) */
> -	xorl	%ebp, %ebp		/* nospec   rbp */
> -	pushq   $0			/* pt_regs->r12 = 0 */
> -	xorl	%r12d, %r12d		/* nospec   r12 */
> -	pushq   $0			/* pt_regs->r13 = 0 */
> -	xorl	%r13d, %r13d		/* nospec   r13 */
> -	pushq   $0			/* pt_regs->r14 = 0 */
> -	xorl	%r14d, %r14d		/* nospec   r14 */
> -	pushq   $0			/* pt_regs->r15 = 0 */
> -	xorl	%r15d, %r15d		/* nospec   r15 */
> -
> +	PUSH_AND_CLEAR_REGS rax=$-ENOSYS
>  	UNWIND_HINT_REGS
> 
>  	movq	%rsp, %rdi
> @@ -381,35 +328,7 @@ SYM_CODE_START(entry_INT80_compat)
>  	pushq	1*8(%rdi)		/* regs->orig_ax */
>  	pushq	(%rdi)			/* pt_regs->di */
>  .Lint80_keep_stack:
> -
> -	pushq	%rsi			/* pt_regs->si */
> -	xorl	%esi, %esi		/* nospec   si */
> -	pushq	%rdx			/* pt_regs->dx */
> -	xorl	%edx, %edx		/* nospec   dx */
> -	pushq	%rcx			/* pt_regs->cx */
> -	xorl	%ecx, %ecx		/* nospec   cx */
> -	pushq	$-ENOSYS		/* pt_regs->ax */
> -	pushq   %r8			/* pt_regs->r8 */
> -	xorl	%r8d, %r8d		/* nospec   r8 */
> -	pushq   %r9			/* pt_regs->r9 */
> -	xorl	%r9d, %r9d		/* nospec   r9 */
> -	pushq   %r10			/* pt_regs->r10*/
> -	xorl	%r10d, %r10d		/* nospec   r10 */
> -	pushq   %r11			/* pt_regs->r11 */
> -	xorl	%r11d, %r11d		/* nospec   r11 */
> -	pushq   %rbx                    /* pt_regs->rbx */
> -	xorl	%ebx, %ebx		/* nospec   rbx */
> -	pushq   %rbp                    /* pt_regs->rbp */
> -	xorl	%ebp, %ebp		/* nospec   rbp */
> -	pushq   %r12                    /* pt_regs->r12 */
> -	xorl	%r12d, %r12d		/* nospec   r12 */
> -	pushq   %r13                    /* pt_regs->r13 */
> -	xorl	%r13d, %r13d		/* nospec   r13 */
> -	pushq   %r14                    /* pt_regs->r14 */
> -	xorl	%r14d, %r14d		/* nospec   r14 */
> -	pushq   %r15                    /* pt_regs->r15 */
> -	xorl	%r15d, %r15d		/* nospec   r15 */
> -
> +	PUSH_AND_CLEAR_REGS save_rdi=0
>  	UNWIND_HINT_REGS
> 
>  	cld

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 2/2] x86,entry: Use PUSH_AND_CLEAR_REGS for compat
  2022-04-20 14:26   ` Andy Lutomirski
@ 2022-04-20 14:51     ` Peter Zijlstra
  0 siblings, 0 replies; 12+ messages in thread
From: Peter Zijlstra @ 2022-04-20 14:51 UTC (permalink / raw)
  To: Andy Lutomirski
  Cc: the arch/x86 maintainers, Josh Poimboeuf, Brian Gerst,
	Lai Jiangshan, Andrew Cooper, Linux Kernel Mailing List

On Wed, Apr 20, 2022 at 07:26:54AM -0700, Andy Lutomirski wrote:
> 
> 
> On Tue, Apr 19, 2022, at 1:41 PM, Peter Zijlstra wrote:
> > Since the upper regs don't exist for ia32 code, preserving them
> > doesn't hurt and it simplifies the code.
> 
> They exist for compat code, though, and should be preserved for ABI
> purposes. Programs that do int $0x80 in 64-bit code do exist.

So this patch preserves semantics for int80, it changes things for
sysenter/syscall, those currently clear the high registers, whereas
after this patch they behave identical to int80.

So the earlier patch:

  https://lkml.kernel.org/r/20220408223827.GR2731@worktop.programming.kicks-ass.net

preserves semantics across the board but is slightly more complicated.

And as argued elsewhere in thie thread; if preserving instead of
clearing the high regs is a valid attach surface, then int80 already
provides it, so I don't see how doing the same to sys* is any worse.


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 2/2] x86,entry: Use PUSH_AND_CLEAR_REGS for compat
  2022-04-20  7:07     ` Peter Zijlstra
@ 2022-04-20 14:58       ` Josh Poimboeuf
  0 siblings, 0 replies; 12+ messages in thread
From: Josh Poimboeuf @ 2022-04-20 14:58 UTC (permalink / raw)
  To: Peter Zijlstra
  Cc: x86, brgerst, jiangshanlai, Andrew.Cooper3, linux-kernel,
	Andy Lutomirski

On Wed, Apr 20, 2022 at 09:07:30AM +0200, Peter Zijlstra wrote:
> On Tue, Apr 19, 2022 at 08:21:23PM -0700, Josh Poimboeuf wrote:
> > On Tue, Apr 19, 2022 at 10:41:11PM +0200, Peter Zijlstra wrote:
> > > Since the upper regs don't exist for ia32 code, preserving them
> > > doesn't hurt and it simplifies the code.
> > 
> > But an attacker can still control those registers, so clearing them on
> > the stack is better, as it reduces user control over the kernel stack.
> > 
> > 64-bit syscalls *do* have to save those registers to the stack, so
> > whether it truly matters if compat mode is made equally insecure, I
> > can't say.  But without evidence to the contrary, my feeling is that we
> > should err on the side of caution.
> 
> Right, so earlier Brian said simpler might be better, and I figured I'd
> try to see if I could make that stick, because I too like simpler ;-)

Simple is good.

> Also, since int80 already has to do this, attackers already have their
> attack surface.

Hm, probably true...

-- 
Josh


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 1/2] x86: Simplify Retpoline thunk
  2022-04-19 20:41 ` [PATCH 1/2] x86: Simplify Retpoline thunk Peter Zijlstra
@ 2022-04-20 15:27   ` Josh Poimboeuf
  2022-04-20 15:51     ` Peter Zijlstra
  0 siblings, 1 reply; 12+ messages in thread
From: Josh Poimboeuf @ 2022-04-20 15:27 UTC (permalink / raw)
  To: Peter Zijlstra; +Cc: x86, brgerst, jiangshanlai, Andrew.Cooper3, linux-kernel

On Tue, Apr 19, 2022 at 10:41:10PM +0200, Peter Zijlstra wrote:
> Now that we rewrite all the indirect call sites, per commit:
> 
>   750850090081 ("x86/alternative: Implement .retpoline_sites support")
> 
> it doesn't make sense to have the retpoline thunks be an ALTERNATIVE_2
> that still includes a 'naked' indirect jump.
> 
> (this accidentally 'defunnels' i386 by going back to full retpolines)

So mitigations=off no longer works on i386?

Is funneling even a concern on i386?  I don't think it has eIBRS anyway,
or does it?

-- 
Josh


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 1/2] x86: Simplify Retpoline thunk
  2022-04-20 15:27   ` Josh Poimboeuf
@ 2022-04-20 15:51     ` Peter Zijlstra
  2022-04-20 16:01       ` Josh Poimboeuf
  0 siblings, 1 reply; 12+ messages in thread
From: Peter Zijlstra @ 2022-04-20 15:51 UTC (permalink / raw)
  To: Josh Poimboeuf; +Cc: x86, brgerst, jiangshanlai, Andrew.Cooper3, linux-kernel

On Wed, Apr 20, 2022 at 08:27:36AM -0700, Josh Poimboeuf wrote:
> On Tue, Apr 19, 2022 at 10:41:10PM +0200, Peter Zijlstra wrote:
> > Now that we rewrite all the indirect call sites, per commit:
> > 
> >   750850090081 ("x86/alternative: Implement .retpoline_sites support")
> > 
> > it doesn't make sense to have the retpoline thunks be an ALTERNATIVE_2
> > that still includes a 'naked' indirect jump.
> > 
> > (this accidentally 'defunnels' i386 by going back to full retpolines)
> 
> So mitigations=off no longer works on i386?

Also true I suppose... does anybody care? /me runs like heck.

I'd hate to make all this more complicated just because i386 tho :/

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 1/2] x86: Simplify Retpoline thunk
  2022-04-20 15:51     ` Peter Zijlstra
@ 2022-04-20 16:01       ` Josh Poimboeuf
  0 siblings, 0 replies; 12+ messages in thread
From: Josh Poimboeuf @ 2022-04-20 16:01 UTC (permalink / raw)
  To: Peter Zijlstra; +Cc: x86, brgerst, jiangshanlai, Andrew.Cooper3, linux-kernel

On Wed, Apr 20, 2022 at 05:51:16PM +0200, Peter Zijlstra wrote:
> On Wed, Apr 20, 2022 at 08:27:36AM -0700, Josh Poimboeuf wrote:
> > On Tue, Apr 19, 2022 at 10:41:10PM +0200, Peter Zijlstra wrote:
> > > Now that we rewrite all the indirect call sites, per commit:
> > > 
> > >   750850090081 ("x86/alternative: Implement .retpoline_sites support")
> > > 
> > > it doesn't make sense to have the retpoline thunks be an ALTERNATIVE_2
> > > that still includes a 'naked' indirect jump.
> > > 
> > > (this accidentally 'defunnels' i386 by going back to full retpolines)
> > 
> > So mitigations=off no longer works on i386?
> 
> Also true I suppose... does anybody care? /me runs like heck.
> 
> I'd hate to make all this more complicated just because i386 tho :/

Not that I care... but at least the commit log should own up to it :-)

-- 
Josh


^ permalink raw reply	[flat|nested] 12+ messages in thread

end of thread, other threads:[~2022-04-20 16:01 UTC | newest]

Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-04-19 20:41 [PATCH 0/2] x86: A few random cleanups Peter Zijlstra
2022-04-19 20:41 ` [PATCH 1/2] x86: Simplify Retpoline thunk Peter Zijlstra
2022-04-20 15:27   ` Josh Poimboeuf
2022-04-20 15:51     ` Peter Zijlstra
2022-04-20 16:01       ` Josh Poimboeuf
2022-04-19 20:41 ` [PATCH 2/2] x86,entry: Use PUSH_AND_CLEAR_REGS for compat Peter Zijlstra
2022-04-20  3:21   ` Josh Poimboeuf
2022-04-20  7:07     ` Peter Zijlstra
2022-04-20 14:58       ` Josh Poimboeuf
2022-04-20  4:29   ` Lai Jiangshan
2022-04-20 14:26   ` Andy Lutomirski
2022-04-20 14:51     ` Peter Zijlstra

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).