Re: [Xen-devel] [PATCH 4/6] xsm: flask: change the interface and default policy for xsm_map_gmfn_foregin

[Zhongze Liu <blackskygg@xxxxxxxxx>]

Hi Jan,

Thanks for reviewing my patch.

2017-08-23 17:55 GMT+08:00 Jan Beulich <JBeulich@xxxxxxxx>:
>>>> On 22.08.17 at 20:08, <blackskygg@xxxxxxxxx> wrote:
>> The original xsm_map_gmfn_foregin policy checks if source domain has the 
>> proper
>> privileges over the target domain. Under this policy, it's not allowed if a 
>> Dom0
>> wants to map pages from one DomU to another, this restricts some useful yet 
>> not
>> dangerous usages of the API, such as sharing pages among DomU's by calling
>> XENMEM_add_to_physmap from Dom0.
>>
>> Change the policy to: IIF current domain has the proper privilege on the
>> target domain and source domain, grant the access.
>
> You say "and here", yet ...
>
>> --- a/xen/include/xsm/dummy.h
>> +++ b/xen/include/xsm/dummy.h
>> @@ -525,10 +525,12 @@ static XSM_INLINE int 
>> xsm_remove_from_physmap(XSM_DEFAULT_ARG struct domain *d1,
>>      return xsm_default_action(action, d1, d2);
>>  }
>>
>> -static XSM_INLINE int xsm_map_gmfn_foreign(XSM_DEFAULT_ARG struct domain 
>> *d, struct domain *t)
>> +static XSM_INLINE int xsm_map_gmfn_foreign(XSM_DEFAULT_ARG struct domain 
>> *cd,
>> +                                           struct domain *d, struct domain 
>> *t)
>>  {
>>      XSM_ASSERT_ACTION(XSM_TARGET);
>> -    return xsm_default_action(action, d, t);
>> +    return xsm_default_action(action, cd, d) ||
>> +        xsm_default_action(action, cd, t);
>>  }
>
> ... you use "or" here and ...

This might be confusing. But think of returning 0 as "allowed", the
only condition where this
statement returns a 0 is when both calls return 0 -- so it's actually
an "and". (Think of de-morgan's law.)

But as Stefano has pointed out, I should preserve the error code.
And as Daniel has pointed out, I should also check if d and t can share memory.

>
>> --- a/xen/xsm/flask/hooks.c
>> +++ b/xen/xsm/flask/hooks.c
>> @@ -1165,9 +1165,11 @@ static int flask_remove_from_physmap(struct domain 
>> *d1, struct domain *d2)
>>      return domain_has_perm(d1, d2, SECCLASS_MMU, MMU__PHYSMAP);
>>  }
>>
>> -static int flask_map_gmfn_foreign(struct domain *d, struct domain *t)
>> +static int flask_map_gmfn_foreign(struct domain *cd,
>> +                                  struct domain *d, struct domain *t)
>>  {
>> -    return domain_has_perm(d, t, SECCLASS_MMU, MMU__MAP_READ | 
>> MMU__MAP_WRITE);
>> +    return domain_has_perm(cd, d, SECCLASS_MMU, MMU__MAP_READ | 
>> MMU__MAP_WRITE) ||
>> +        domain_has_perm(cd, t, SECCLASS_MMU, MMU__MAP_READ | 
>> MMU__MAP_WRITE);
>>  }
>
> ... here. A domain can't have XSM_TARGET permission over two
> other domains, so what you want to do here can't work at all,
> afaict.

I agree with what Stefano has said below.

Cheers,

Zhongze Liu.

>
> Jan
>

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