Re: [PATCHv2] xen/x86: don't corrupt %eip when returning from a signal handler

[David Vrabel <david.vrabel@citrix.com>]

On 19/10/12 16:29, Jan Beulich wrote:
>>>> On 17.10.12 at 15:29, David Vrabel <david.vrabel@citrix.com> wrote:
>> From: David Vrabel <david.vrabel@citrix.com>
>>
>> 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