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

[Zhongze Liu <blackskygg@xxxxxxxxx>]

Hi Daniel and Jan,

2017-10-20 21:34 GMT+08:00 Daniel De Graaf <dgdegra@xxxxxxxxxxxxx>:
> On 10/20/2017 02:14 AM, Jan Beulich wrote:
>>>>>
>>>>> 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
>
>
> I would say the current lack of a check for (c over t) is an oversight,
> which mostly doesn't matter because the ability to modify arbitrary
> memory in your target is transitive in almost any security model (c can
> modify d's code to modify t, so a malicious c can compromise t anyway).
> If the three domains are all different, the only way this can happen in
> non-XSM is for (c) to be dom0 or for your device model to have a device
> model (which I don't think is forbidden, but doubt anyone uses).
>
> I now agree that this deserves a new subop, since this code is reached
> via the stable memory_op and not just a domctl.

How about changing the policy to (c over d) && ((d over t) || (c over t))?
Given that (c over d) is a must, which is always checked somewhere higher
in the call stack as Daniel pointed out,  permitting (d over t) or (c
over t) actually infers
permitting the other.

- if you permit (d over t) but not (c over t):
  Given (c over t),
  (c) can first map the src page from (t) into its own memory space and then map
  this page from its own memory space to (d)'s memory space.

- if you permit (c over t) but not (d over t):
  Given (d over t),
  (c) can first map (d)'s pages into its own memory space and modify (d)'s code
  to issues a hypercall that maps (t)'s memory pages into (d)'s memory space.

I'm not very familiar with Xen's security model. So I might be totally
wrong here.
If so, please correct me.

And if you still think adding a new subop is necessary, do you have
any suggestions
on this?

Cheers,

Zhongze Liu

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