Re: [Xen-devel] [PATCH v3 2/7] xsm: flask: change the dummy xsm policy and flask hook for map_gmfn_foregin

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 19.10.17 at 19:36, <dgdegra@xxxxxxxxxxxxx> wrote:
> On 10/19/2017 07:58 AM, Jan Beulich wrote:
>>>>> On 19.10.17 at 04:36, <blackskygg@xxxxxxxxx> wrote:
>>> --- a/xen/include/xsm/dummy.h
>>> +++ b/xen/include/xsm/dummy.h
>>> @@ -516,7 +516,8 @@ static XSM_INLINE int 
> xsm_remove_from_physmap(XSM_DEFAULT_ARG struct domain *d1,
>>>   static XSM_INLINE int xsm_map_gmfn_foreign(XSM_DEFAULT_ARG struct domain 
> *d, struct domain *t)
>>>   {
>>>       XSM_ASSERT_ACTION(XSM_TARGET);
>>> -    return xsm_default_action(action, d, t);
>>> +    return xsm_default_action(action, current->domain, d) ?:
>>> +        xsm_default_action(action, current->domain, t);
>>>   }
>> 
>> When all three domains are different, how does the changed
>> policy reflect the original "d has privilege over t" requirement?
>> I understand you want to relax the current condition, but this
>> shouldn't come at the price of granting access when access
>> should be denied. Nor the inverse - the current domain not
>> having privilege over both does also not mean d doesn't have
>> the necessary privilege over t.
>> 
>> I continue to think that you can't validly retrofit the new
>> intended functionality onto the existing hypercall, even if
>> nothing except the permission check needs to be different.
>> 
>> Jan
> 
> If this operation is going to be allowed at all (and I agree it has
> valid use cases), then there won't be a privilege relationship between
> (d) and (t) to check - they'll both be (somewhat related) domUs as far
> as Xen can tell.  If this hypercall isn't used, adding a new hypercall
> (subop) is the only way I'd see to do it - and that seems very redundant
> as it'd need to do all the same checks except for the one about the
> relationship between (d) and (t).  I don't see the reason why the
> existing hypercall should deny being used for that purpose once it's
> possible using other means.

One problem is, as you mention here, ...

> The only possible problem that springs to mind is a restricted kernel
> interface (such as the one used by QEMU in dom0 that restricts to a
> single target domain) that now doesn't realize it's relaying an
> operation that also requires permission over (t) after only checking
> that the origin is allowed to modify (d).

... the delegation of privilege checking responsibility to a
possibly untrusted environment. Plus, as explained before,
current callers expect privilege of d over t to be validated,
which isn't happening anymore with the proposed change. If
the existing sub-op was to be modified, I think we'd need
(with c representing the current domain)
- (d over t) || ((c over d) && (c over t)) for not regressing
  the pre-existing use case,
- only (c over d) && (c over t) for not permitting something
  that isn't intended to be permitted in the new use case.
Unless the sub-op has room for adding a flag to indicate
which of the two is meant (I didn't check), I don't see a way
around adding another sub-op, no matter how similar this
would end up being.

Jan


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