Re: [Xen-devel] [RFC PATCH V3 05/12] xen: Introduce vm_event

[Tamas K Lengyel <tamas.lengyel@xxxxxxxxxxxx>]

On Mon, Feb 2, 2015 at 8:35 PM, Daniel De Graaf <dgdegra@xxxxxxxxxxxxx> wrote:
> On 01/31/2015 08:24 AM, Tamas K Lengyel wrote:
>>
>> On Fri, Jan 30, 2015 at 6:25 PM, Daniel De Graaf <dgdegra@xxxxxxxxxxxxx>
>> wrote:
>>>
>>> On 01/29/2015 04:46 PM, Tamas K Lengyel wrote:
>>>>
>>>>
>>>> To make it easier to review the renaming process of mem_event ->
>>>> vm_event,
>>>> the process is broken into three pieces, of which this patch is the
>>>> first.
>>>> In this patch the vm_event subsystem is introduced and hooked into the
>>>> build
>>>> process, but it is not yet used anywhere.
>>>>
>>>> Signed-off-by: Tamas K Lengyel <tamas.lengyel@xxxxxxxxxxxx>
>>>
>>>
>>>
>>> [...]
>>>>
>>>>
>>>> diff --git a/xen/include/xsm/dummy.h b/xen/include/xsm/dummy.h
>>>> index f20e89c..d6d403a 100644
>>>> --- a/xen/include/xsm/dummy.h
>>>> +++ b/xen/include/xsm/dummy.h
>>>> @@ -525,6 +525,18 @@ static XSM_INLINE int
>>>> xsm_mem_event_op(XSM_DEFAULT_ARG struct domain *d, int op)
>>>>        XSM_ASSERT_ACTION(XSM_DM_PRIV);
>>>>        return xsm_default_action(action, current->domain, d);
>>>>    }
>>>> +
>>>> +static XSM_INLINE int xsm_vm_event_control(XSM_DEFAULT_ARG struct
>>>> domain
>>>> *d, int mode, int op)
>>>> +{
>>>> +    XSM_ASSERT_ACTION(XSM_PRIV);
>>>> +    return xsm_default_action(action, current->domain, d);
>>>> +}
>>>> +
>>>> +static XSM_INLINE int xsm_vm_event_op(XSM_DEFAULT_ARG struct domain *d,
>>>> int op)
>>>> +{
>>>> +    XSM_ASSERT_ACTION(XSM_DM_PRIV);
>>>> +    return xsm_default_action(action, current->domain, d);
>>>> +}
>>>>    #endif
>>>>
>>> [...]
>>>>
>>>>
>>>> diff --git a/xen/xsm/flask/policy/access_vectors
>>>> b/xen/xsm/flask/policy/access_vectors
>>>> index 1da9f63..a4241b5 100644
>>>> --- a/xen/xsm/flask/policy/access_vectors
>>>> +++ b/xen/xsm/flask/policy/access_vectors
>>>> @@ -250,6 +250,7 @@ class hvm
>>>>        hvmctl
>>>>    # XEN_DOMCTL_set_access_required
>>>>        mem_event
>>>> +    vm_event
>>>>    # XEN_DOMCTL_mem_sharing_op and XENMEM_sharing_op_{share,add_physmap}
>>>> with:
>>>>    #  source = the domain making the hypercall
>>>>    #  target = domain whose memory is being shared
>>>>
>>>
>>> This implies that device model domains should be allowed to use the
>>> operations
>>> covered by xsm_vm_event_op but not those covered by xsm_vm_event_control.
>>> If this is how the eventual operations are intended to be used, the FLASK
>>> permissions also need to be split so that a similar distinction can be
>>> made
>>> in
>>> the policy.
>>>
>>> After looking at the later patches in this series, this appears to be a
>>> flaw
>>> in
>>> the existing FLASK hooks that got copied over.  While it is still useful
>>> to
>>> fix,
>>> it  may be better to make the split in a separate patch from the renames.
>>> Now
>>> that VM events apply to more than just HVM domains, it may be useful to
>>> move
>>> the new permission(s) from class hvm to either domain2 or mmu.
>>>
>>> --
>>> Daniel De Graaf
>>> National Security Agency
>>
>>
>> Moving it to domain2 would make sense to me. The namings seem to be
>> pretty poor so I have a hard time understanding why xsm_vm_event_op
>> and xsm_vm_event_control differ when it comes to device model domains.
>> The event_op corresponds to memops for access, paging and sharing
>> while event_control for the domctl that enables/disables the rings. So
>> yes, I think splitting the name for these separating things would make
>> sense to clarify what they represent but whether that restriction on
>> device model domains was intentional I'm not sure about.
>>
>> Tamas
>
>
> If the device model stubdom does not use (or plan to use) the memory_op
> hypercall, then there is no reason to allow it to make this hypercall,
> and the XSM check should be changed from XSM_DM_PRIV to XSM_PRIV.  From
> a quick look, this seems to be true, but I would defer to someone who
> has actually used or developed this subsystem.

This subsystem hasn't really seen much use AFAIK and I'm not aware on
anyone using it in device model stubdom, thus this change would be
reasonable.

>
> As far as the naming, several other hypercalls such as tmem have a
> distinction between "use" and "control" that is reflected in the XSM
> policy.  From a quick look at how the hypercalls work, the domctl seems
> to be a toolstack-only feature that is set when building a domain, while
> the mem_event_op hypercall is used by a helper process.
>
> I think it might be possible to move the helper process to a stub domain
> when creating a very disaggregated system.  In that case, a distinct
> permission for its hypercalls would be useful to let the stub domain
> perform sharing operations without being able to turn sharing on and
> off.  Otherwise the current single permission (moved to domain2) should
> be sufficient.

I would rather keep with the current single permission and split it
only when there is a need for it. In our current model the secondary
(security) control domain pretty much holds equivalent rights to dom0
over a subset of domains, so for us there is no benefit in having such
refined distinction between permissions.

>
>
> --
> Daniel De Graaf
> National Security Agency

Thanks,
Tamas

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