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) <[email protected]>
---
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
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) <[email protected]>
> ---
> 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
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
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/[email protected]
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.
On Wed, Apr 20, 2022 at 4:53 AM Peter Zijlstra <[email protected]> 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
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
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.