From mboxrd@z Thu Jan 1 00:00:00 1970 From: David Vrabel Subject: Re: [PATCHv2] xen/x86: don't corrupt %eip when returning from a signal handler Date: Fri, 19 Oct 2012 16:44:00 +0100 Message-ID: <50817540.5070908@citrix.com> References: <1350480580-4844-1-git-send-email-david.vrabel@citrix.com> <50818DF502000078000A2A84@nat28.tlf.novell.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <50818DF502000078000A2A84@nat28.tlf.novell.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Jan Beulich Cc: Frediano Ziglio , Konrad Rzeszutek Wilk , "stable@vger.kernel.org" , "xen-devel@lists.xen.org" List-Id: xen-devel@lists.xenproject.org On 19/10/12 16:29, Jan Beulich wrote: >>>> On 17.10.12 at 15:29, David Vrabel wrote: >> From: David Vrabel >> >> In 32 bit guests, if a userspace process has %eax == -ERESTARTSYS >> (-512) or -ERESTARTNOINTR (-513) when it is interrupted by an event >> /and/ the process has a pending signal then %eip (and %eax) are >> corrupted when returning to the main process after handling the >> signal. The application may then crash with SIGSEGV or a SIGILL or it >> may have subtly incorrect behaviour (depending on what instruction it >> returned to). >> [...] >> diff --git a/arch/x86/kernel/entry_32.S b/arch/x86/kernel/entry_32.S >> index 2c63407..6a19e66 100644 >> --- a/arch/x86/kernel/entry_32.S >> +++ b/arch/x86/kernel/entry_32.S >> @@ -1042,7 +1042,7 @@ ENTRY(xen_sysenter_target) >> >> ENTRY(xen_hypervisor_callback) >> CFI_STARTPROC >> - pushl_cfi $0 >> + pushl_cfi $-1 /* orig_ax = -1 => not a system call */ >> SAVE_ALL >> TRACE_IRQS_OFF >> >> @@ -1078,7 +1078,7 @@ ENDPROC(xen_hypervisor_callback) >> # We distinguish between categories by maintaining a status value in EAX. >> ENTRY(xen_failsafe_callback) >> CFI_STARTPROC >> - pushl_cfi %eax >> + pushl_cfi $-1 /* orig_ax = -1 => not a system call */ > > While making this apply to the 2.6.18 tree, I noticed that you > replaced the wrong push here, thus causing register corruption. > Just like on the 64-bit side, the one that needs fixing is the one > right before the SAVE_ALL (and hence it's again not just for > consistency, as zero is being pushed there too). Oops. We would have liked to test this path but could not see how to. Do you have any ideas? David