Re: [RFC XEN PATCH v4 2/5] x86/pvh: Allow (un)map_pirq when caller isn't DOMID_SELF

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Fri, 5 Jan 2024, 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. Then xc_physdev_map_pirq
> will call into Xen, but in hvm_physdev_op, PHYSDEVOP_map_pirq
> is not allowed because currd is PVH dom0 and PVH has no
> X86_EMU_USE_PIRQ flag, it will fail at has_pirq check.
> 
> So, allow PHYSDEVOP_map_pirq when domid of the caller is not
> DOMID_SELF no matter whether currd has X86_EMU_USE_PIRQ flag
> and also allow PHYSDEVOP_unmap_pirq for the failed path to
> unmap pirq.
> 
> Co-developed-by: Huang Rui <ray.huang@xxxxxxx>
> Signed-off-by: Jiqian Chen <Jiqian.Chen@xxxxxxx>
> ---
>  xen/arch/x86/hvm/hypercall.c | 27 +++++++++++++++++++++++++--
>  1 file changed, 25 insertions(+), 2 deletions(-)
> 
> diff --git a/xen/arch/x86/hvm/hypercall.c b/xen/arch/x86/hvm/hypercall.c
> index 6ad5b4d5f11f..632a68be3cc4 100644
> --- a/xen/arch/x86/hvm/hypercall.c
> +++ b/xen/arch/x86/hvm/hypercall.c
> @@ -10,6 +10,7 @@
>  #include <xen/hypercall.h>
>  #include <xen/ioreq.h>
>  #include <xen/nospec.h>
> +#include <xen/guest_access.h>
>  
>  #include <asm/hvm/emulate.h>
>  #include <asm/hvm/support.h>
> @@ -72,8 +73,30 @@ long hvm_physdev_op(int cmd, XEN_GUEST_HANDLE_PARAM(void) 
> arg)
>  
>      switch ( cmd )
>      {
> -    case PHYSDEVOP_map_pirq:
> -    case PHYSDEVOP_unmap_pirq:
> +    case PHYSDEVOP_map_pirq: {
> +        physdev_map_pirq_t map;
> +
> +        if ( copy_from_guest(&map, arg, 1) != 0 )
> +            return -EFAULT;
> +
> +        if ( !has_pirq(currd) && map.domid == DOMID_SELF )
> +            return -ENOSYS;

This looks OK to me although there is already another copy_from_guest in
do_physdev_op, but I don't see an easy way to make it better.

Also, you could write this check like this:

        d = rcu_lock_domain_by_any_id(map.domid);
        if ( d == NULL )
            return -ESRCH;
        if ( !has_pirq(d) )
            return -ENOSYS;
        rcu_unlock_domain(d);

This way it is a bit more generic and not special-cased to DOMID_SELF.

I'll let the x86 maintainers comment on the way the prefer it.


> +        break;
> +    }
> +
> +    case PHYSDEVOP_unmap_pirq: {
> +        physdev_unmap_pirq_t unmap;
> +
> +        if ( copy_from_guest(&unmap, arg, 1) != 0 )
> +            return -EFAULT;
> +
> +        if ( !has_pirq(currd) && unmap.domid == DOMID_SELF )
> +            return -ENOSYS;
> +
> +        break;
> +    }
> +
>      case PHYSDEVOP_eoi:
>      case PHYSDEVOP_irq_status_query:
>      case PHYSDEVOP_get_free_pirq:
> -- 
> 2.34.1
>