From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753436AbbDBTKf (ORCPT ); Thu, 2 Apr 2015 15:10:35 -0400 Received: from mail-wg0-f51.google.com ([74.125.82.51]:34993 "EHLO mail-wg0-f51.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751856AbbDBTKd (ORCPT ); Thu, 2 Apr 2015 15:10:33 -0400 MIME-Version: 1.0 In-Reply-To: <551D5E23.2050002@redhat.com> References: <1427985378-4287-1-git-send-email-dvlasenk@redhat.com> <1427985378-4287-3-git-send-email-dvlasenk@redhat.com> <551D5E23.2050002@redhat.com> Date: Thu, 2 Apr 2015 15:10:31 -0400 Message-ID: Subject: Re: [PATCH 3/9] x86/asm/entry/64: do not SAVE_EXTRA_REGS in stub_sigreturn From: Brian Gerst To: Denys Vlasenko Cc: Ingo Molnar , Linus Torvalds , Steven Rostedt , Borislav Petkov , "H. Peter Anvin" , Andy Lutomirski , Oleg Nesterov , Frederic Weisbecker , Alexei Starovoitov , Will Drewry , Kees Cook , "the arch/x86 maintainers" , Linux Kernel Mailing List Content-Type: text/plain; charset=UTF-8 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Thu, Apr 2, 2015 at 11:20 AM, Denys Vlasenko wrote: > On 04/02/2015 05:01 PM, Brian Gerst wrote: >> On Thu, Apr 2, 2015 at 10:36 AM, Denys Vlasenko wrote: >>> stub_sigreturn ignores old values of pt_regs->REG for all general-purpose >>> registers, it sets them to values saved on userspace >>> signal stack. >>> >>> Which is hardly surprising - it would be a bug if it would use pt_regs->REG. >>> sigreturn must restore all registers. >>> >>> Therefore, SAVE_EXTRA_REGS in it ought to be redundant. >>> >>> It is a leftover from the time SAVE_EXTRA_REGS wasn't only saving registers, >>> but it also was extending stack to "full" pt_regs. >>> >>> Delete this SAVE_EXTRA_REGS. >>> >>> Run-tested. >>> >>> Signed-off-by: Denys Vlasenko >>> CC: Linus Torvalds >>> CC: Steven Rostedt >>> CC: Ingo Molnar >>> CC: Borislav Petkov >>> CC: "H. Peter Anvin" >>> CC: Andy Lutomirski >>> CC: Oleg Nesterov >>> CC: Frederic Weisbecker >>> CC: Alexei Starovoitov >>> CC: Will Drewry >>> CC: Kees Cook >>> CC: x86@kernel.org >>> CC: linux-kernel@vger.kernel.org >>> --- >>> arch/x86/kernel/entry_64.S | 9 +++++++-- >>> 1 file changed, 7 insertions(+), 2 deletions(-) >>> >>> diff --git a/arch/x86/kernel/entry_64.S b/arch/x86/kernel/entry_64.S >>> index ec51598..1cf245d 100644 >>> --- a/arch/x86/kernel/entry_64.S >>> +++ b/arch/x86/kernel/entry_64.S >>> @@ -447,7 +447,12 @@ ENTRY(stub_rt_sigreturn) >>> CFI_STARTPROC >>> addq $8, %rsp >>> DEFAULT_FRAME 0 >>> - SAVE_EXTRA_REGS >>> + /* >>> + * Despite RESTORE_EXTRA_REGS in return_from_stub, >>> + * no need to SAVE_EXTRA_REGS here: >>> + * sys_rt_sigreturn overwrites all general purpose pt_regs->REGs >>> + * on stack, for RESTORE_{EXTRA,C}_REGS to pick them up. >>> + */ >>> call sys_rt_sigreturn >>> jmp return_from_stub >>> CFI_ENDPROC >>> @@ -458,7 +463,7 @@ ENTRY(stub_x32_rt_sigreturn) >>> CFI_STARTPROC >>> addq $8, %rsp >>> DEFAULT_FRAME 0 >>> - SAVE_EXTRA_REGS >>> + /* No need to SAVE_EXTRA_REGS */ >>> call sys32_x32_rt_sigreturn >>> jmp return_from_stub >>> CFI_ENDPROC >> >> I had the same idea, but determined sigreturn can fault and return an >> error code without modifying all the registers. This would leak junk >> from the stack. To clarify, I remembered looking at sigreturn possibly faulting from the 32-bit perspective, where the 6th arg is read from the user stack and a fault there would return -EFAULT, for any syscall. > This still can be made to work by not RESTORE'ing EXTRA_REGS either, > if there is a way to detect the failure: > > call sys_rt_sigreturn > - jmp return_from_stub > + testl ??????????? > + jz return_from_stub > + ret > CFI_ENDPROC > > But this is not a normal syscall, off-hand I don't see an easy way > to do the test. sys_rt_sigreturn() on failure runs this code: > > ... > segfault: > force_sig(SIGSEGV, current); > return 0; > } > > Help? I don't think you can test the return value, because in the success case it can be any value (the restored RAX value). -- Brian Gerst