Re: [Xen-devel] [PATCH v4 2/7] xen: xsm: flask: introduce XENMAPSPACE_gmfn_share for memory sharing

[Zhongze Liu <blackskygg@xxxxxxxxx>]

Hi Jan,

(Sorry for the late reply.)

2018-02-02 16:32 GMT+08:00 Jan Beulich <JBeulich@xxxxxxxx>:
>>>> On 01.02.18 at 19:11, <blackskygg@xxxxxxxxx> wrote:
>> 2018-02-01 18:23 GMT+08:00 Jan Beulich <JBeulich@xxxxxxxx>:
>>>>>> On 30.01.18 at 18:50, <blackskygg@xxxxxxxxx> wrote:
>>>> --- a/xen/include/xsm/dummy.h
>>>> +++ b/xen/include/xsm/dummy.h
>>>> @@ -521,6 +521,12 @@ static XSM_INLINE int 
>>>> xsm_map_gmfn_foreign(XSM_DEFAULT_ARG struct domain *d, str
>>>>      return xsm_default_action(action, d, t);
>>>>  }
>>>>
>>>> +static XSM_INLINE int xsm_map_gmfn_share(XSM_DEFAULT_ARG struct domain 
>>>> *d, struct domain *t)
>>>> +{
>>>> +    XSM_ASSERT_ACTION(XSM_TARGET);
>>>> +    return xsm_default_action(action, current->domain, t);
>>>
>>> How does this represent a proper default equivalent of ...
>>>
>>> --- a/xen/xsm/flask/hooks.c
>>> +++ b/xen/xsm/flask/hooks.c
>>> @@ -1196,6 +1196,12 @@ static int flask_map_gmfn_foreign(struct domain *d, 
>>> struct domain *t)
>>>      return domain_has_perm(d, t, SECCLASS_MMU, MMU__MAP_READ | 
>>> MMU__MAP_WRITE);
>>>  }
>>>
>>> +static int flask_map_gmfn_share(struct domain *d, struct domain *t)
>>> +{
>>> +    return current_has_perm(t, SECCLASS_MMU, MMU__MAP_READ | 
>>> MMU__MAP_WRITE) ?:
>>> +        domain_has_perm(d, t, SECCLASS_MMU, MMU__SHARE_MEM);
>>>
>>> ... this?
>>
>> The full flask check tries to guarantee that:
>>   1) (c) over (d), which will always be checked somewhere higher in
>>       the call stack for these kinds of calls;
>
> This is in no way apparent here. At the very least a comment to
> that effect should be added, but perhaps even better would be
> if you had an explicit xsm_default_action(..., current->domain, d)
> call there. Whether one or the other is preferable I'd leave to
> Daniel.
>

During our discussion on v3 of the patch set, Daniel has already expressed
his preference not to duplicate the (c)-over-(d) check here, and that's why
I've eliminated the check in this version. But I do agree with you on adding a
comment here ...

>>   2) (c) over (t), namely, MMU__MAP_READ/WRITE;
>>   3) (d) over (t), namely, MMU__SHARE_MEM
>>
>> In my default builtin actions, checks 1) and 2) are done by the
>> xsm_default_action
>> function, but I can't think of a way to do check 3), because we don't
>> have the proper
>> equivalence of MMU__SHARE_MEM (xsm_default_action is definitely not a
>> choice).
>> Want to hear your and other maintainers' suggestions about how to do
>> this properly.
>
> "(d) over (t)" would suggest xsm_default_action(action, d, t), which
> I don't think is appropriate here. In fact aiui d and t are unrelated to
> one another in terms of mutual privilege. I don't think 3) needs
> expressing in the dummy version; it's really the apparent lack of 1)k
> which I've been commenting on. But again, I'll leave it to Daniel to
> tell you otherwise. What is imperative in any event is that you
> extend the description to also reason about the dummy logic, at
> least as long as it isn't a clear equivalent of the flask variant.
>

... and here.

Cheers,

Zhongze Liu

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel