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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 24/10/16 11:09, Jan Beulich wrote:
>>>> On 24.10.16 at 11:55, <andrew.cooper3@xxxxxxxxxx> wrote:
>> On 24/10/16 10:33, Jan Beulich wrote:
>>>>>> On 14.10.16 at 17:51, <andrew.cooper3@xxxxxxxxxx> wrote:
>>>> When the compat hypercall ABI was added for HVM guests (i.e. supporting 
>>>> 32bit
>>>> operating systems making hypercalls against a 64bit Xen), an ABI breakage 
>>>> was
>>>> introduced for non-compat guests, as the 64bit hypercall index became
>>>> truncated to 32 bits.
>>>>
>>>> This has been the case for a very long time, but is not very obvious from 
>>>> the
>>>> code, and definitely counterintuitive, seeing as all other 64bit parameters
>>>> are passed without truncation.
>>>>
>>>> However, the only supported method of making hypercalls is to call into the
>>>> hypercall page, which in practice means that only hypercall index up to 63 
>>>> are
>>>> supported.
>>>>
>>>> Therefore, take the opportunity to fix the ABI before it becomes 
>>>> impossible to
>>>> fix.
>>> Considering
>>>
>>>     if ( (eax & 0x80000000) && is_viridian_domain(currd) )
>>>         return viridian_hypercall(regs);
>> Why is this a problem?
> Because the use of bit 31 (instead of <guest-address-width> - 1)
> pretty clearly indicates the expectation of a 32-bit value here.

It is specifically a bit which is specifically reserved in the Viridian
ABI.  It isn't related to guest address width.

>
>>  Truncation or not has no effect of this
>> calculation, and the exact value comes from the Xen-provided Viridian
>> hypercall page (see enable_hypercall_page() in viridian.c); we are at
>> liberty to change the semantics if we wish.
> No, we aren't. No-one is required to use the hypercall page. We
> only provide it as a courtesy.

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.

>
>>> I'm not convinced we should change current behavior, the more that
>>> the change has at least theoretical potential of breaking existing guests.
>> My argument is that no existing guests can be relying on this behaviour.
> Why not? People tend to rely on anything they find works.

There is no supported or expected way of getting into this situation to
start with, and the only entities making hypercalls will be using some
variation of our public headers.

~Andrew

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