Re: [Xen-devel] [PATCH v3] x86/HVM: Merge HVM and PVH hypercall tables

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 18/12/15 17:33, Boris Ostrovsky wrote:
> On 12/18/2015 12:16 PM, Andrew Cooper wrote:
>> On 18/12/15 17:10, Jan Beulich wrote:
>>>>>> On 18.12.15 at 17:59, <andrew.cooper3@xxxxxxxxxx> wrote:
>>>> On 18/12/15 16:37, Jan Beulich wrote:
>>>>>>>> On 18.12.15 at 17:28, <andrew.cooper3@xxxxxxxxxx> wrote:
>>>>>> On 17/12/15 23:00, Boris Ostrovsky wrote:
>>>>>>> diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c
>>>>>>> index a7767f8..871aca0 100644
>>>>>>> --- a/xen/arch/x86/mm.c
>>>>>>> +++ b/xen/arch/x86/mm.c
>>>>>>> @@ -3019,6 +3019,25 @@ long do_mmuext_op(
>>>>>>>               break;
>>>>>>>           }
>>>>>>>   +        if ( has_hvm_container_domain(d) )
>>>>>>> +        {
>>>>>>> +            switch ( op.cmd )
>>>>>>> +            {
>>>>>>> +            case MMUEXT_PIN_L1_TABLE:
>>>>>>> +            case MMUEXT_PIN_L2_TABLE:
>>>>>>> +            case MMUEXT_PIN_L3_TABLE:
>>>>>>> +            case MMUEXT_PIN_L4_TABLE:
>>>>>>> +            case MMUEXT_UNPIN_TABLE:
>>>>>>> +                if ( is_control_domain(d) )
>>>>>>> +                    break;
>>>>>> This needs to be an XSM check, rather than a dom0 check. 
>>>>>> Consider the
>>>>>> usecase of a PVH/DMLite domain builder stubdomain.
>>>>> But wouldn't that be the control domain then? Afaict by making this
>>>>> an XSM check we'd also permit the hardware domain access to these,
>>>>> for no reason. In fact we should probably further restrict this to
>>>>> d != pg_owner.
>>>> Any domain needing to construct PV domains needs to be able to make
>>>> these hypercalls against the target domain.
>>>>
>>>> Therefore, the only valid check is whether XSM will permit
>>>> 'current' to
>>>> issue the hypercall against 'd', irrespective of whether current is
>>>> the
>>>> control domain, the hardware domain, or something else.
>>>>
>>>> I think all that is needed is xsm_mmuext_op(XSM_TARGET, d, pg_owner)
>>> Which, as Boris has just pointed out, is already there.
>> So it is.  That is good.
>>
>>> But which also allows the d to issue such operations on itself.
>
> Won't get_pg_owner() fail in that case? (domid == curr->domain_id)  test?
>
>> For safely sake, it is probably having either do_mmuext_op() or the XSM
>> hook bail early if d is not a PV guest.
>>
>> I would hesitate at putting that check inside the hvm conditional at
>> this point.
>
> I am not sure what you meant here.

Something like this:

diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c
index 92df36f..cc14905 100644
--- a/xen/arch/x86/mm.c
+++ b/xen/arch/x86/mm.c
@@ -2997,6 +2997,9 @@ long do_mmuext_op(
     if ( (pg_owner = get_pg_owner(foreigndom)) == NULL )
         return -ESRCH;
 
+    if ( !is_pv_domain(pg_owner) )
+        return -EINVAL;
+
     rc = xsm_mmuext_op(XSM_TARGET, d, pg_owner);
     if ( rc )
     {


(Completely untested)

~Andrew

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