On 04/02/2015 09:10 PM, Brian Gerst wrote: > On Thu, Apr 2, 2015 at 11:20 AM, Denys Vlasenko <dvlas...@redhat.com> wrote: >> On 04/02/2015 05:01 PM, Brian Gerst wrote: >>> On Thu, Apr 2, 2015 at 10:36 AM, Denys Vlasenko <dvlas...@redhat.com> 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 <dvlas...@redhat.com> >>>> CC: Linus Torvalds <torva...@linux-foundation.org> >>>> CC: Steven Rostedt <rost...@goodmis.org> >>>> CC: Ingo Molnar <mi...@kernel.org> >>>> CC: Borislav Petkov <b...@alien8.de> >>>> CC: "H. Peter Anvin" <h...@zytor.com> >>>> CC: Andy Lutomirski <l...@amacapital.net> >>>> CC: Oleg Nesterov <o...@redhat.com> >>>> CC: Frederic Weisbecker <fweis...@gmail.com> >>>> CC: Alexei Starovoitov <a...@plumgrid.com> >>>> CC: Will Drewry <w...@chromium.org> >>>> CC: Kees Cook <keesc...@chromium.org> >>>> CC: x...@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 majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/