Re: [XEN PATCH v8 2/5] x86/pvh: Allow (un)map_pirq when dom0 is PVH

[Jan Beulich <jbeulich@xxxxxxxx>]

On 16.05.2024 11:52, Jiqian Chen wrote:
> If run Xen with PVH dom0 and hvm domU, hvm will map a pirq for
> a passthrough device by using gsi, see
> xen_pt_realize->xc_physdev_map_pirq and
> pci_add_dm_done->xc_physdev_map_pirq.

xen_pt_realize() is in qemu, which imo wants saying here (for being a different
repo), the more that pci_add_dm_done() is in libxl.

> --- a/xen/arch/x86/hvm/hypercall.c
> +++ b/xen/arch/x86/hvm/hypercall.c
> @@ -74,6 +74,8 @@ long hvm_physdev_op(int cmd, XEN_GUEST_HANDLE_PARAM(void) 
> arg)
>      {
>      case PHYSDEVOP_map_pirq:
>      case PHYSDEVOP_unmap_pirq:
> +        break;

I think this could do with a comment as to why it's permitted as well as giving
a reference to where further restrictions are enforced (or simply mentioning
the constraint of this only being permitted for management of other domains).

> --- a/xen/arch/x86/physdev.c
> +++ b/xen/arch/x86/physdev.c
> @@ -305,11 +305,23 @@ ret_t do_physdev_op(int cmd, 
> XEN_GUEST_HANDLE_PARAM(void) arg)
>      case PHYSDEVOP_map_pirq: {
>          physdev_map_pirq_t map;
>          struct msi_info msi;
> +        struct domain *d;
>  
>          ret = -EFAULT;
>          if ( copy_from_guest(&map, arg, 1) != 0 )
>              break;
>  
> +        d = rcu_lock_domain_by_any_id(map.domid);
> +        if ( d == NULL )
> +            return -ESRCH;
> +        /* If caller is the same HVM guest as current, check pirq flag */

The caller is always current. What I think you mean is "caller is same as
the subject domain". I'm also having trouble with seeing the usefulness
of saying "check pirq flag". Instead I think you want to state the
restriction here that you actually mean to enforce (which would also mean
mentioning PVH in some way, to distinguish from the "normal HVM" case).

> +        if ( !is_pv_domain(d) && !has_pirq(d) && map.domid == DOMID_SELF )

You exclude DOMID_SELF but not the domain's ID? Why not simply check d
being current->domain, thus covering both cases? Plus you could use
rcu_lock_domain_by_id() to exclude DOMID_SELF, and you could use
rcu_lock_remote_domain_by_id() to exclude the local domain altogether.
Finally I'm not even sure you need the RCU lock here (else you could
use knownalive_domain_from_domid()). But perhaps that's better to cover
the qemu-in-stubdom case, which we have to consider potentially malicious.

I'm also inclined to suggest to use is_hvm_domain() here in favor of
!is_pv_domain().

Jan