Re: PCI passthrough for HVM with stubdomain broken by "tools/libxl: handle the iomem parameter with the memory_mapping hcall"

[Daniel De Graaf <dgdegra@tycho.nsa.gov>]

On 06/23/2016 11:22 AM, Marek Marczykowski-Górecki wrote:
> On Thu, Jun 23, 2016 at 11:00:42AM -0400, Daniel De Graaf wrote:
>> On 06/23/2016 09:25 AM, Marek Marczykowski-Górecki wrote:
>> [...]
>>> Ok, after drawing a flowchart of the control in this function after your
>>> change, on a piece of paper, this case looks fine. But depending on how
>>> the domain was found (explicit loop or rcu_lock_domain_by_id), different
>>> locks are held, which makes it harder to follow what is going on.
>>>
>>> Crazy idea: how about making the code easy/easier to read instead of
>>> obfuscating it even more? XEN_DOMCTL_getdomaininfo semantic is
>>> convolved enough. How about this version (2 patches):
>> [...]
>>> xen: allow XEN_DOMCTL_getdomaininfo for device model
>>>
>>> Allow device model domain to get info about its target domain.
>>> It is used during PCI passthrough setup (xc_domain_memory_mapping
>>> checks for guest being auto-translated). While it happens in stubdomain,
>>> it failed, breaking PCI passthrough in such setup.
>>>
>>> While it is possible to workaround this at toolstack side, it seems
>>> logical to allow device model to get information about its target
>>> domain.
>>>
>>> The problem was exposed by c428c9f "tools/libxl: handle the iomem
>>> parameter with the memory_mapping hcall".
>>>
>>> Signed-off-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>
>>> ---
>>>  xen/include/xsm/dummy.h | 5 ++++-
>>>  1 file changed, 4 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/xen/include/xsm/dummy.h b/xen/include/xsm/dummy.h
>>> index 406cd18..70a1633 100644
>>> --- a/xen/include/xsm/dummy.h
>>> +++ b/xen/include/xsm/dummy.h
>>> @@ -128,7 +128,10 @@ static XSM_INLINE int xsm_domctl(XSM_DEFAULT_ARG struct domain *d, int cmd)
>>>      case XEN_DOMCTL_unbind_pt_irq:
>>>          return xsm_default_action(XSM_DM_PRIV, current->domain, d);
>>>      case XEN_DOMCTL_getdomaininfo:
>>> -        return xsm_default_action(XSM_XS_PRIV, current->domain, d);
>>> +        if (current->domain->target)
>>> +            return xsm_default_action(XSM_DM_PRIV, current->domain, d);
>>> +        else
>>> +            return xsm_default_action(XSM_XS_PRIV, current->domain, d);
>>>      default:
>>>          return xsm_default_action(XSM_PRIV, current->domain, d);
>>>      }
>>
>> I would prefer testing for the xenstore flag instead of testing for the
>> target field.  It ends up being the same thing in reality, since nobody
>> sane would make the xenstore also a device model (and not also dom0).
>>
>>       case XEN_DOMCTL_getdomaininfo:
>>           if ( src->is_xenstore )
>>               return 0;
>>           return xsm_default_action(XSM_DM_PRIV, current->domain, d);
>>
>> This makes it clear that xenstore is the special case, and removes the
>> need for the one-off XSM_XS_PRIV constant.
>
> This was my initial idea, but I don't really understand the comment
> about link-time verification if the behaviour is the same for xsm not
> compiled vs disabled. But if skipping xsm_default_action here doesn't
> break this magic, I'm for it.

That magic is just for the parameter to the XSM hook: in this case, it's
the XSM_OTHER in xsm_domctl that is being verified.  There is no magic
in xsm_default_action.

> Updated patch (with removal of XSM_XS_PRIV):
>
> xen: allow XEN_DOMCTL_getdomaininfo for device model domains
>
> Allow device model domain to get info about its target domain.
> It is used during PCI passthrough setup (xc_domain_memory_mapping
> checks for guest being auto-translated). While it happens in stubdomain,
> it failed, breaking PCI passthrough in such setup.
>
> While it is possible to workaround this at toolstack side, it seems
> logical to allow device model to get information about its target
> domain.
>
> Also, since this was the only usage of XSM_XS_PRIV, which now gets
> handled inline, drop it.
>
> The problem was exposed by c428c9f "tools/libxl: handle the iomem
> parameter with the memory_mapping hcall".
>
> Signed-off-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>

This would be a good patch to include the corresponding change to the
XSM policy (tweaked from Jan's email):

> --- a/tools/flask/policy/modules/xen/xen.if
> +++ b/tools/flask/policy/modules/xen/xen.if
> @@ -148,7 +148,7 @@ define(`device_model', `
>  	create_channel($2, $1, $2_channel)
>  	allow $1 $2_channel:event create;
>
> -	allow $1 $2_target:domain shutdown;
> +	allow $1 $2_target:domain { getdomaininfo shutdown };
>  	allow $1 $2_target:mmu { map_read map_write adjust physmap target_hack };
>  	allow $1 $2_target:hvm { getparam setparam trackdirtyvram hvmctl irqlevel pciroute pcilevel cacheattr send_irq };
>  ')

With that included, or elsewhere in the series:

Acked-by: Daniel De Graaf <dgdegra@tycho.nsa.gov>

> ---
>  xen/include/xsm/dummy.h | 8 +++-----
>  xen/include/xsm/xsm.h   | 1 -
>  2 files changed, 3 insertions(+), 6 deletions(-)
>
> diff --git a/xen/include/xsm/dummy.h b/xen/include/xsm/dummy.h
> index 406cd18..2768861 100644
> --- a/xen/include/xsm/dummy.h
> +++ b/xen/include/xsm/dummy.h
> @@ -71,10 +71,6 @@ static always_inline int xsm_default_action(
>          if ( src->is_privileged )
>              return 0;
>          return -EPERM;
> -    case XSM_XS_PRIV:
> -        if ( src->is_xenstore || src->is_privileged )
> -            return 0;
> -        return -EPERM;
>      default:
>          LINKER_BUG_ON(1);
>          return -EPERM;
> @@ -128,7 +124,9 @@ static XSM_INLINE int xsm_domctl(XSM_DEFAULT_ARG struct domain *d, int cmd)
>      case XEN_DOMCTL_unbind_pt_irq:
>          return xsm_default_action(XSM_DM_PRIV, current->domain, d);
>      case XEN_DOMCTL_getdomaininfo:
> -        return xsm_default_action(XSM_XS_PRIV, current->domain, d);
> +        if ( current->domain->is_xenstore )
> +            return 0;
> +        return xsm_default_action(XSM_DM_PRIV, current->domain, d);
>      default:
>          return xsm_default_action(XSM_PRIV, current->domain, d);
>      }
> diff --git a/xen/include/xsm/xsm.h b/xen/include/xsm/xsm.h
> index 0d525ec..09672e7 100644
> --- a/xen/include/xsm/xsm.h
> +++ b/xen/include/xsm/xsm.h
> @@ -38,7 +38,6 @@ enum xsm_default {
>      XSM_DM_PRIV,  /* Device model can perform on its target domain */
>      XSM_TARGET,   /* Can perform on self or your target domain */
>      XSM_PRIV,     /* Privileged - normally restricted to dom0 */
> -    XSM_XS_PRIV,  /* Xenstore domain - can do some privileged operations */
>      XSM_OTHER     /* Something more complex */
>  };
>  typedef enum xsm_default xsm_default_t;
>


-- 
Daniel De Graaf
National Security Agency

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