Re: [Xen-devel] [PATCH] x86: fold code in load_segments()

2016-09-15 Thread Andrew Cooper 
On 15/09/2016 07:28, Jan Beulich wrote:
 On 14.09.16 at 19:12,  wrote:
>> On 14/09/16 16:24, Jan Beulich wrote:
>>> --- a/xen/arch/x86/domain.c
>>> +++ b/xen/arch/x86/domain.c
>>> @@ -1745,22 +1745,22 @@ static void load_segments(struct vcpu *n
>>>  (unsigned long *)pv->kernel_sp;
>>>  unsigned long cs_and_mask, rflags;
>>>  
>>> +/* Fold upcall mask and architectural IOPL into RFLAGS.IF. */
>>> +rflags  = regs->rflags & ~(X86_EFLAGS_IF|X86_EFLAGS_IOPL);
>>> +rflags |= !vcpu_info(n, evtchn_upcall_mask) << 9;
>>> +if ( VM_ASSIST(n->domain, architectural_iopl) )
>>> +rflags |= n->arch.pv_vcpu.iopl;
>>> +
>>>  if ( is_pv_32bit_vcpu(n) )
>>>  {
>>>  unsigned int *esp = ring_1(regs) ?
>>>  (unsigned int *)regs->rsp :
>>>  (unsigned int *)pv->kernel_sp;
>>> -unsigned int cs_and_mask, eflags;
>> The unshadowed cs_and_mask is unsigned long, not int, which means the
>> put_user() below will clobber a 32bit PV guests stack frame.
> No, put_user() determines the access size from its second (pointer)
> argument.

Oh - so it does.  Mind putting at least note to that effect in the
commit message?  My first thought upon seeing it was wondering whether
you had a stale patch which didn't compile.

With that, Reviewed-by: Andrew Cooper 

___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH] x86: fold code in load_segments()

2016-09-15 Thread Jan Beulich 
>>> On 14.09.16 at 19:12,  wrote:
> On 14/09/16 16:24, Jan Beulich wrote:
>> --- a/xen/arch/x86/domain.c
>> +++ b/xen/arch/x86/domain.c
>> @@ -1745,22 +1745,22 @@ static void load_segments(struct vcpu *n
>>  (unsigned long *)pv->kernel_sp;
>>  unsigned long cs_and_mask, rflags;
>>  
>> +/* Fold upcall mask and architectural IOPL into RFLAGS.IF. */
>> +rflags  = regs->rflags & ~(X86_EFLAGS_IF|X86_EFLAGS_IOPL);
>> +rflags |= !vcpu_info(n, evtchn_upcall_mask) << 9;
>> +if ( VM_ASSIST(n->domain, architectural_iopl) )
>> +rflags |= n->arch.pv_vcpu.iopl;
>> +
>>  if ( is_pv_32bit_vcpu(n) )
>>  {
>>  unsigned int *esp = ring_1(regs) ?
>>  (unsigned int *)regs->rsp :
>>  (unsigned int *)pv->kernel_sp;
>> -unsigned int cs_and_mask, eflags;
> 
> The unshadowed cs_and_mask is unsigned long, not int, which means the
> put_user() below will clobber a 32bit PV guests stack frame.

No, put_user() determines the access size from its second (pointer)
argument.

> Other than that, Reviewed-by: Andrew Cooper 
> for the intended change.

Well, with the above (it not being clear what change you would have
expected, should one be needed in the first place) I'll have to wait
for clarification.

Jan


___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH] x86: fold code in load_segments()

2016-09-14 Thread Andrew Cooper 
On 14/09/16 16:24, Jan Beulich wrote:
> No need to have the same logic twice.
>
> Signed-off-by: Jan Beulich 
>
> --- a/xen/arch/x86/domain.c
> +++ b/xen/arch/x86/domain.c
> @@ -1745,22 +1745,22 @@ static void load_segments(struct vcpu *n
>  (unsigned long *)pv->kernel_sp;
>  unsigned long cs_and_mask, rflags;
>  
> +/* Fold upcall mask and architectural IOPL into RFLAGS.IF. */
> +rflags  = regs->rflags & ~(X86_EFLAGS_IF|X86_EFLAGS_IOPL);
> +rflags |= !vcpu_info(n, evtchn_upcall_mask) << 9;
> +if ( VM_ASSIST(n->domain, architectural_iopl) )
> +rflags |= n->arch.pv_vcpu.iopl;
> +
>  if ( is_pv_32bit_vcpu(n) )
>  {
>  unsigned int *esp = ring_1(regs) ?
>  (unsigned int *)regs->rsp :
>  (unsigned int *)pv->kernel_sp;
> -unsigned int cs_and_mask, eflags;

The unshadowed cs_and_mask is unsigned long, not int, which means the
put_user() below will clobber a 32bit PV guests stack frame.

Other than that, Reviewed-by: Andrew Cooper 
for the intended change.

~Andrew

>  int ret = 0;
>  
>  /* CS longword also contains full evtchn_upcall_mask. */
>  cs_and_mask = (unsigned short)regs->cs |
>  ((unsigned int)vcpu_info(n, evtchn_upcall_mask) << 16);
> -/* Fold upcall mask into RFLAGS.IF. */
> -eflags  = regs->_eflags & ~(X86_EFLAGS_IF|X86_EFLAGS_IOPL);
> -eflags |= !vcpu_info(n, evtchn_upcall_mask) << 9;
> -if ( VM_ASSIST(n->domain, architectural_iopl) )
> -eflags |= n->arch.pv_vcpu.iopl;
>  
>  if ( !ring_1(regs) )
>  {
> @@ -1770,7 +1770,7 @@ static void load_segments(struct vcpu *n
>  }
>  
>  if ( ret |
> - put_user(eflags,  esp-1) |
> + put_user(rflags,  esp-1) |
>   put_user(cs_and_mask, esp-2) |
>   put_user(regs->_eip,  esp-3) |
>   put_user(uregs->gs,   esp-4) |
> @@ -1805,12 +1805,6 @@ static void load_segments(struct vcpu *n
>  cs_and_mask = (unsigned long)regs->cs |
>  ((unsigned long)vcpu_info(n, evtchn_upcall_mask) << 32);
>  
> -/* Fold upcall mask into RFLAGS.IF. */
> -rflags  = regs->rflags & ~(X86_EFLAGS_IF|X86_EFLAGS_IOPL);
> -rflags |= !vcpu_info(n, evtchn_upcall_mask) << 9;
> -if ( VM_ASSIST(n->domain, architectural_iopl) )
> -rflags |= n->arch.pv_vcpu.iopl;
> -
>  if ( put_user(regs->ss,rsp- 1) |
>   put_user(regs->rsp,   rsp- 2) |
>   put_user(rflags,  rsp- 3) |
>
>
>
>
>
> ___
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> https://lists.xen.org/xen-devel

___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel