Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751187AbbDBTkh (ORCPT ); Thu, 2 Apr 2015 15:40:37 -0400 Received: from mx1.redhat.com ([209.132.183.28]:58130 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750797AbbDBTkg (ORCPT ); Thu, 2 Apr 2015 15:40:36 -0400 Message-ID: <551D9B16.2030404@redhat.com> Date: Thu, 02 Apr 2015 21:40:06 +0200 From: Denys Vlasenko User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:24.0) Gecko/20100101 Thunderbird/24.2.0 MIME-Version: 1.0 To: Brian Gerst 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 Subject: Re: [PATCH 3/9] x86/asm/entry/64: do not SAVE_EXTRA_REGS in stub_sigreturn References: <1427985378-4287-1-git-send-email-dvlasenk@redhat.com> <1427985378-4287-3-git-send-email-dvlasenk@redhat.com> <551D5E23.2050002@redhat.com> In-Reply-To: Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 3948 Lines: 106 On 04/02/2015 09:10 PM, Brian Gerst wrote: > 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). Yeah. I think the "optimize out SAVE_EXTRA_REGS on sigreturn" idea didn't play out. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/