Re: [Xen-devel] [PATCH v3 3/4] x86/pvh: Handle hypercalls for 32b PVH guests

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 24.07.15 at 20:35, <boris.ostrovsky@xxxxxxxxxx> wrote:
> On 07/23/2015 10:21 AM, Jan Beulich wrote:
>>>>> On 11.07.15 at 00:20, <boris.ostrovsky@xxxxxxxxxx> wrote:
>>> Signed-off-by: Boris Ostrovsky <boris.ostrovsky@xxxxxxxxxx>
>>> ---
>>> Changes in v3:
>>> * Defined compat_mmuext_op(). (XEN_GUEST_HANDLE_PARAM(mmuext_op_compat_t)
>>>    is not defined in header files so I used 'void' type.
>> How is it not? It's in compat/xen.h (which is a generated header).
> 
> compat/xen.h has DEFINE_COMPAT_HANDLE(mmuext_op_compat_t) (which is 
> __compat_handle_mmuext_op_compat_t).
> 
> We need XEN_GUEST_HANDLE(mmuext_op_compat_t), which is 
> __guest_handle_mmuext_op_compat_t. And I wasn't sure it's worth 
> explicitly adding it to a header file (like I think what we do for 
> vcpu_runstate_info_compat_t in sched.h);

Hmm, indeed all other compat_..._op()-s use void handles (albeit in
most if not all of the cases their native counterparts do too). So I
guess using void here is fine then, or using COMPAT_HANDLE()
instead. It's not really relevant anyway since COMPAT_CALL()
casts the function pointer to the intended type anyway.

>>> @@ -4981,7 +5003,7 @@ int hvm_do_hypercall(struct cpu_user_regs *regs)
>>>           return viridian_hypercall(regs);
>>>   
>>>       if ( (eax >= NR_hypercalls) ||
>>> -         (is_pvh_domain(currd) ? !pvh_hypercall64_table[eax]
>>> +         (is_pvh_domain(currd) ? !pvh_hypercall32_table[eax]
>>>                                  : !hvm_hypercall32_table[eax]) )
>> ... this will break (as we're assuming 32- and 64-bit tables to be fully
>> in sync here; there's still the pending work item of constructing these
>> tables so that this has a better chance of not getting broken).
> 
> So you prefer to have full check --- explicitly for both 32- and 64-bit, 
> right?

No. Just adding the missing operation to the table will deal with it.
I wouldn't like to see more conditionals to be added to this code
path when we can avoid doing so. What we could do is add a
respective ASSERT() to the 64-bit path, albeit the NULL deref
would be observable as a fault without the ASSERT() too (and
adding one wouldn't help release builds [and their security]).

Jan


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