Re: [tip:x86/asm] x86/asm/entry/64: Always allocate a complete " struct pt_regs" on the kernel stack

[Brian Gerst]

On Wed, Mar 4, 2015 at 5:53 PM, tip-bot for Denys Vlasenko
<tipbot@xxxxxxxxx> wrote:
> Commit-ID:  76f5df43cab5e765c0bd42289103e8f625813ae1
> Gitweb:     http://git.kernel.org/tip/76f5df43cab5e765c0bd42289103e8f625813ae1
> Author:     Denys Vlasenko <dvlasenk@xxxxxxxxxx>
> AuthorDate: Thu, 26 Feb 2015 14:40:27 -0800
> Committer:  Ingo Molnar <mingo@xxxxxxxxxx>
> CommitDate: Wed, 4 Mar 2015 22:50:49 +0100
>
> x86/asm/entry/64: Always allocate a complete "struct pt_regs" on the kernel stack
>
> The 64-bit entry code was using six stack slots less by not
> saving/restoring registers which are callee-preserved according
> to the C ABI, and was not allocating space for them.
>
> Only when syscalls needed a complete "struct pt_regs" was
> the complete area allocated and filled in.
>
> As an additional twist, on interrupt entry a "slightly less
> truncated pt_regs" trick is used, to make nested interrupt
> stacks easier to unwind.
>
> This proved to be a source of significant obfuscation and subtle
> bugs. For example, 'stub_fork' had to pop the return address,
> extend the struct, save registers, and push return address back.
> Ugly. 'ia32_ptregs_common' pops return address and "returns" via
> jmp insn, throwing a wrench into CPU return stack cache.
>
> This patch changes the code to always allocate a complete
> "struct pt_regs" on the kernel stack. The saving of registers
> is still done lazily.
>
> "Partial pt_regs" trick on interrupt stack is retained.
>
> Macros which manipulate "struct pt_regs" on stack are reworked:
>
>  - ALLOC_PT_GPREGS_ON_STACK allocates the structure.
>
>  - SAVE_C_REGS saves to it those registers which are clobbered
>    by C code.
>
>  - SAVE_EXTRA_REGS saves to it all other registers.
>
>  - Corresponding RESTORE_* and REMOVE_PT_GPREGS_FROM_STACK macros
>    reverse it.
>
> 'ia32_ptregs_common', 'stub_fork' and friends lost their ugly dance
> with the return pointer.
>
> LOAD_ARGS32 in ia32entry.S now uses symbolic stack offsets
> instead of magic numbers.
>
> 'error_entry' and 'save_paranoid' now use SAVE_C_REGS +
> SAVE_EXTRA_REGS instead of having it open-coded yet again.
>
> Patch was run-tested: 64-bit executables, 32-bit executables,
> strace works.
>
> Timing tests did not show measurable difference in 32-bit
> and 64-bit syscalls.
>
> Signed-off-by: Denys Vlasenko <dvlasenk@xxxxxxxxxx>
> Signed-off-by: Andy Lutomirski <luto@xxxxxxxxxxxxxx>
> Cc: Alexei Starovoitov <ast@xxxxxxxxxxxx>
> Cc: Borislav Petkov <bp@xxxxxxxxx>
> Cc: Frederic Weisbecker <fweisbec@xxxxxxxxx>
> Cc: H. Peter Anvin <hpa@xxxxxxxxx>
> Cc: Kees Cook <keescook@xxxxxxxxxxxx>
> Cc: Linus Torvalds <torvalds@xxxxxxxxxxxxxxxxxxxx>
> Cc: Oleg Nesterov <oleg@xxxxxxxxxx>
> Cc: Will Drewry <wad@xxxxxxxxxxxx>
> Link: http://lkml.kernel.org/r/1423778052-21038-2-git-send-email-dvlasenk@xxxxxxxxxx
> Link: http://lkml.kernel.org/r/b89763d354aa23e670b9bdf3a40ae320320a7c2e.1424989793.git.luto@xxxxxxxxxxxxxx
> Signed-off-by: Ingo Molnar <mingo@xxxxxxxxxx>


> @@ -492,7 +496,6 @@ GLOBAL(stub32_clone)
>
>         ALIGN
>  ia32_ptregs_common:
> -       popq %r11
>         CFI_ENDPROC
>         CFI_STARTPROC32 simple
>         CFI_SIGNAL_FRAME
> @@ -507,9 +510,9 @@ ia32_ptregs_common:
>  /*     CFI_REL_OFFSET  rflags,EFLAGS-ARGOFFSET*/
>         CFI_REL_OFFSET  rsp,RSP-ARGOFFSET
>  /*     CFI_REL_OFFSET  ss,SS-ARGOFFSET*/
> -       SAVE_REST
> +       SAVE_EXTRA_REGS 8
>         call *%rax
> -       RESTORE_REST
> -       jmp  ia32_sysret        /* misbalances the return cache */
> +       RESTORE_EXTRA_REGS 8
> +       ret
>         CFI_ENDPROC
>  END(ia32_ptregs_common)

This change is causing Wine to fail when it's built to support both
32-bit and 64-bit code:

[  523.657406] wineserver[1605]: segfault at 453a7c90 ip
00000000453a7c90 sp 00000000b3c6ad10 error 14 in
ld-2.21.so[7fad453a7000+21000]

It tries to exec the 64-bit wineserver from 32-bit code and faults
because %rip is truncated.  This is because simply returning to the
caller will exit via sysretl which doesn't change %cs to 64-bit mode.
We need to set a thread flag to enter the slow path and return via
iret instead.

Patch coming to fix it.

--
Brian Gerst
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/