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

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

[Andrew Cooper]

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.

While tweaking this area, fix one piece of trailing whitespace.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>

RFC because this might manifest as an ABI change for guests making hypercalls
via an unsupported mechanism.  However, I feel that fixing the bug is the less
bad option.
---
 xen/arch/x86/hvm/hvm.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
index 3c90ecd..563f029 100644
--- 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;
+
     if ( (eax & 0x80000000) && is_viridian_domain(currd) )
         return viridian_hypercall(regs);
 
@@ -4307,7 +4309,7 @@ int hvm_do_hypercall(struct cpu_user_regs *regs)
         unsigned long r8 = regs->r8;
         unsigned long r9 = regs->r9;
 
-        HVM_DBG_LOG(DBG_LEVEL_HCALL, "hcall%u(%lx, %lx, %lx, %lx, %lx, %lx)",
+        HVM_DBG_LOG(DBG_LEVEL_HCALL, "hcall%lu(%lx, %lx, %lx, %lx, %lx, %lx)",
                     eax, rdi, rsi, rdx, r10, r8, r9);
 
 #ifndef NDEBUG
@@ -4354,7 +4356,7 @@ int hvm_do_hypercall(struct cpu_user_regs *regs)
         unsigned int edi = regs->_edi;
         unsigned int ebp = regs->_ebp;
 
-        HVM_DBG_LOG(DBG_LEVEL_HCALL, "hcall%u(%x, %x, %x, %x, %x, %x)", eax,
+        HVM_DBG_LOG(DBG_LEVEL_HCALL, "hcall%lu(%x, %x, %x, %x, %x, %x)", eax,
                     ebx, ecx, edx, esi, edi, ebp);
 
 #ifndef NDEBUG
@@ -4390,7 +4392,7 @@ int hvm_do_hypercall(struct cpu_user_regs *regs)
 #endif
     }
 
-    HVM_DBG_LOG(DBG_LEVEL_HCALL, "hcall%u -> %lx",
+    HVM_DBG_LOG(DBG_LEVEL_HCALL, "hcall%lu -> %lx",
                 eax, (unsigned long)regs->eax);
 
     if ( curr->arch.hvm_vcpu.hcall_preempted )
-- 
2.1.4


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

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

[Jan Beulich]

>>> On 14.10.16 at 17:51, <andrew.cooper3@citrix.com> 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);

I'm not convinced we should change current behavior, the more that
the change has at least theoretical potential of breaking existing guests.

> @@ -4283,6 +4283,8 @@ int hvm_do_hypercall(struct cpu_user_regs *regs)
>          break;
>      }
>  
> +    eax = (mode == 8) ? regs->eax : regs->_eax;

But if we indeed want to make the adjustment, please use regs->rax
here (slightly helping the register field renaming series I have pending
for 4.9).

Jan

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

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

[Andrew Cooper]

On 24/10/16 10:33, Jan Beulich wrote:
>>>> On 14.10.16 at 17:51, <andrew.cooper3@citrix.com> 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?  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.

>
> 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.

>
>> @@ -4283,6 +4283,8 @@ int hvm_do_hypercall(struct cpu_user_regs *regs)
>>          break;
>>      }
>>  
>> +    eax = (mode == 8) ? regs->eax : regs->_eax;
> But if we indeed want to make the adjustment, please use regs->rax
> here (slightly helping the register field renaming series I have pending
> for 4.9).

Can do.

~Andrew

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

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

[Jan Beulich]

>>> On 24.10.16 at 11:55, <andrew.cooper3@citrix.com> wrote:
> On 24/10/16 10:33, Jan Beulich wrote:
>>>>> On 14.10.16 at 17:51, <andrew.cooper3@citrix.com> 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.

>  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.

>> 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.

Jan

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

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

[Andrew Cooper]

On 24/10/16 11:09, Jan Beulich wrote:
>>>> On 24.10.16 at 11:55, <andrew.cooper3@citrix.com> wrote:
>> On 24/10/16 10:33, Jan Beulich wrote:
>>>>>> On 14.10.16 at 17:51, <andrew.cooper3@citrix.com> 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@lists.xen.org
https://lists.xen.org/xen-devel

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

[Jan Beulich]

>>> On 24.10.16 at 12:25, <andrew.cooper3@citrix.com> wrote:
> On 24/10/16 11:09, Jan Beulich wrote:
>>>>> On 24.10.16 at 11:55, <andrew.cooper3@citrix.com> wrote:
>>> On 24/10/16 10:33, Jan Beulich wrote:
>>>>>>> On 14.10.16 at 17:51, <andrew.cooper3@citrix.com> 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.

Well, okay for that part, albeit it makes clear that we can't ever
have hypercall numbers in the range 0x80000000...0xffffffff.

>>>  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.

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).

>>>> 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.

This is something we can hope for, but can't expect.

Jan

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

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

[Andrew Cooper]

On 24/10/16 12:13, Jan Beulich wrote:
>>>> On 24.10.16 at 12:25, <andrew.cooper3@citrix.com> wrote:
>> On 24/10/16 11:09, Jan Beulich wrote:
>>>>>> On 24.10.16 at 11:55, <andrew.cooper3@citrix.com> wrote:
>>>> On 24/10/16 10:33, Jan Beulich wrote:
>>>>>>>> On 14.10.16 at 17:51, <andrew.cooper3@citrix.com> 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.
> Well, okay for that part, albeit it makes clear that we can't ever
> have hypercall numbers in the range 0x80000000...0xffffffff.

Not with the current Viridian scheme, but this can be fixed if we need to.

>
>>>>  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.
> 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.

>
>>>>> 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.
> This is something we can hope for, but can't expect.

This isn't relevant here.  There has never been a supported way of doing
this, and given the need for hypercall pages in HVM guests, I am very
willing to believe that there is no code or binary at all which ever
ends up in this truncation case.

As we do not have stability of unsupported scenarios, this bug should be
fixed while it is still a theoretical issue, not a real issue.

~Andrew

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

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

[Jan Beulich]

>>> On 26.10.16 at 20:19, <andrew.cooper3@citrix.com> wrote:
> On 24/10/16 12:13, Jan Beulich wrote:
>>>>> On 24.10.16 at 12:25, <andrew.cooper3@citrix.com> 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@suse.com>

Jan

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

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

[Andrew Cooper]

On 27/10/16 08:12, Jan Beulich wrote:
>>>> On 26.10.16 at 20:19, <andrew.cooper3@citrix.com> wrote:
>> On 24/10/16 12:13, Jan Beulich wrote:
>>>>>> On 24.10.16 at 12:25, <andrew.cooper3@citrix.com> 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@suse.com>

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);

~Andrew

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

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

[Jan Beulich]

>>> On 27.10.16 at 14:55, <andrew.cooper3@citrix.com> wrote:
> On 27/10/16 08:12, Jan Beulich wrote:
>>>>> On 26.10.16 at 20:19, <andrew.cooper3@citrix.com> wrote:
>>> On 24/10/16 12:13, Jan Beulich wrote:
>>>>>>> On 24.10.16 at 12:25, <andrew.cooper3@citrix.com> 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@suse.com>
> 
> 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@lists.xen.org
https://lists.xen.org/xen-devel