Re: [Xen-devel] [PATCH] RFC x86/hvm: Don't truncate the hvm hypercall index before range checking it

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 27.10.16 at 14:55, <andrew.cooper3@xxxxxxxxxx> wrote:
> On 27/10/16 08:12, Jan Beulich wrote:
>>>>> On 26.10.16 at 20:19, <andrew.cooper3@xxxxxxxxxx> wrote:
>>> On 24/10/16 12:13, Jan Beulich wrote:
>>>>>>> On 24.10.16 at 12:25, <andrew.cooper3@xxxxxxxxxx> wrote:
>>>>> Yes we very much are at liberty to change things.  Viridian would not
>>>>> function without using that page (as the hypercalls would be confused
>>>>> with Xen hypercalls), and the spec is very clear that the hypercall page
>>>>> will be used.
>>>>>
>>>>> As for the Xen hypercall page, the ABI is clearly stated as:
>>>>>
>>>>>     call hypercall_page + hypercall-number * 32
>>>>>
>>>>> in include/public/arch-x86/xen-x86_{32,64}.h, meaning that we are
>>>>> perfectly at liberty to alter the layout and inner-workings of our
>>>>> hypercall page as well.
>>>> This, iirc, is not something that has been this way from the beginning;
>>>> I think the page has got introduced as a courtesy for 64-bit PV guests,
>>>> where the hypercall sequence involves multiple instructions (I can't
>>>> tell whether perhaps for HVM guests it has always been there, to
>>>> abstract out the vendor differences in what instruction to use).
>>>>
>>>> In fact even current upstream Linux still has a remnant of it being
>>>> different, by way of the (now unused) TRAP_INSTR definition. If the
>>>> presence of a hypercall page (as an obvious prerequisite of its use)
>>>> was a requirement, we shouldn't boot guests not having one (and we
>>>> probably should go as far as refusing calls originating from outside,
>>>> which would break many if not all SUSE 32-bit PV kernels, which do a
>>>> few early calls without going through hypercall_page).
>>> PV guests aren't a problem.  Even now, they don't truncate %rax.
>>>
>>> HVM guests have always had hypercall pages.  Having gone through the
>>> history again, it appears that the 64bit HVM ABI was introduced broken,
>>> by c/s 5eeca68f, despite the fact that the mov $imm32, %eax in the
>>> hypercall page provides the expected truncation.
>> Okay, you've convinced me. I'd like to slightly refine my earlier minor
>> adjustment request though:
>>
>>> --- a/xen/arch/x86/hvm/hvm.c
>>> +++ b/xen/arch/x86/hvm/hvm.c
>>> @@ -4265,11 +4265,11 @@ int hvm_do_hypercall(struct cpu_user_regs *regs)
>>>      struct domain *currd = curr->domain;
>>>      struct segment_register sreg;
>>>      int mode = hvm_guest_x86_mode(curr);
>>> -    uint32_t eax = regs->eax;
>>> +    unsigned long eax;
>>>  
>>>      switch ( mode )
>>>      {
>>> -    case 8:        
>>> +    case 8:
>>>      case 4:
>>>      case 2:
>>>          hvm_get_segment_register(curr, x86_seg_ss, &sreg);
>>> @@ -4283,6 +4283,8 @@ int hvm_do_hypercall(struct cpu_user_regs *regs)
>>>          break;
>>>      }
>>>  
>>> +    eax = (mode == 8) ? regs->eax : regs->_eax;
>> I think to avoid another conditional here, regs->_eax could remain to
>> be the initializer of eax, and the use of regs->rax could be but into the
>> "case 8:" which you touch anyway. I'm not insisting on this though, so
>> no matter with just the originally requested adjustment of this one:
>> Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>
> 
> Something like this?
> 
> diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
> index 11e2b82..69b740d 100644
> --- a/xen/arch/x86/hvm/hvm.c
> +++ b/xen/arch/x86/hvm/hvm.c
> @@ -4279,11 +4279,12 @@ int hvm_do_hypercall(struct cpu_user_regs *regs)
>      struct domain *currd = curr->domain;
>      struct segment_register sreg;
>      int mode = hvm_guest_x86_mode(curr);
> -    uint32_t eax = regs->eax;
> +    unsigned long eax = regs->_eax;
>  
>      switch ( mode )
>      {
> -    case 8:       
> +    case 8:
> +        eax = regs->rax;
>      case 4:
>      case 2:
>          hvm_get_segment_register(curr, x86_seg_ss, &sreg);

Yes, with some fall-through comment.

Jan

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