Re: [PATCH v4] xen/iommu: cleanup iommu related domctl handling

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Wed, 20 Apr 2022, Juergen Gross wrote:
> Today iommu_do_domctl() is being called from arch_do_domctl() in the
> "default:" case of a switch statement. This has led already to crashes
> due to unvalidated parameters.
> 
> Fix that by moving the call of iommu_do_domctl() to the main switch
> statement of do_domctl().
> 
> Signed-off-by: Juergen Gross <jgross@xxxxxxxx>

For the ARM side:

Reviewed-by: Stefano Stabellini <sstabellini@xxxxxxxxxx>


I have no opinion on the ENOSYS vs EOPNOTSUPP discussion.



> ---
> V3:
> - new patch
> V4:
> - add iommu_do_domctl() stub for !CONFIG_HAS_PASSTHROUGH (Andrew Cooper,
>   Jan Beulich)
> ---
>  xen/arch/arm/domctl.c   | 11 +----------
>  xen/arch/x86/domctl.c   |  2 +-
>  xen/common/domctl.c     |  7 +++++++
>  xen/include/xen/iommu.h | 12 +++++++++---
>  4 files changed, 18 insertions(+), 14 deletions(-)
> 
> diff --git a/xen/arch/arm/domctl.c b/xen/arch/arm/domctl.c
> index 6245af6d0b..1baf25c3d9 100644
> --- a/xen/arch/arm/domctl.c
> +++ b/xen/arch/arm/domctl.c
> @@ -176,16 +176,7 @@ long arch_do_domctl(struct xen_domctl *domctl, struct 
> domain *d,
>          return rc;
>      }
>      default:
> -    {
> -        int rc;
> -
> -        rc = subarch_do_domctl(domctl, d, u_domctl);
> -
> -        if ( rc == -ENOSYS )
> -            rc = iommu_do_domctl(domctl, d, u_domctl);
> -
> -        return rc;
> -    }
> +        return subarch_do_domctl(domctl, d, u_domctl);
>      }
>  }
>  
> diff --git a/xen/arch/x86/domctl.c b/xen/arch/x86/domctl.c
> index a6aae500a3..c9699bb868 100644
> --- a/xen/arch/x86/domctl.c
> +++ b/xen/arch/x86/domctl.c
> @@ -1380,7 +1380,7 @@ long arch_do_domctl(
>          break;
>  
>      default:
> -        ret = iommu_do_domctl(domctl, d, u_domctl);
> +        ret = -ENOSYS;
>          break;
>      }
>  
> diff --git a/xen/common/domctl.c b/xen/common/domctl.c
> index 5879117580..0a866e3132 100644
> --- a/xen/common/domctl.c
> +++ b/xen/common/domctl.c
> @@ -871,6 +871,13 @@ long cf_check 
> do_domctl(XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl)
>              copyback = 1;
>          break;
>  
> +    case XEN_DOMCTL_assign_device:
> +    case XEN_DOMCTL_test_assign_device:
> +    case XEN_DOMCTL_deassign_device:
> +    case XEN_DOMCTL_get_device_group:
> +        ret = iommu_do_domctl(op, d, u_domctl);
> +        break;
> +
>      default:
>          ret = arch_do_domctl(op, d, u_domctl);
>          break;
> diff --git a/xen/include/xen/iommu.h b/xen/include/xen/iommu.h
> index 3a83981464..c6bbb65bbf 100644
> --- a/xen/include/xen/iommu.h
> +++ b/xen/include/xen/iommu.h
> @@ -341,8 +341,17 @@ struct domain_iommu {
>  /* Does the IOMMU pagetable need to be kept synchronized with the P2M */
>  #ifdef CONFIG_HAS_PASSTHROUGH
>  #define need_iommu_pt_sync(d)     (dom_iommu(d)->need_sync)
> +
> +int iommu_do_domctl(struct xen_domctl *domctl, struct domain *d,
> +                    XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl);
>  #else
>  #define need_iommu_pt_sync(d)     ({ (void)(d); false; })
> +
> +static inline int iommu_do_domctl(struct xen_domctl *domctl, struct domain 
> *d,
> +                                  XEN_GUEST_HANDLE_PARAM(xen_domctl_t) 
> u_domctl)
> +{
> +    return -ENOSYS;
> +}
>  #endif
>  
>  int __must_check iommu_suspend(void);
> @@ -356,9 +365,6 @@ int iommu_do_pci_domctl(struct xen_domctl *, struct 
> domain *d,
>                          XEN_GUEST_HANDLE_PARAM(xen_domctl_t));
>  #endif
>  
> -int iommu_do_domctl(struct xen_domctl *, struct domain *d,
> -                    XEN_GUEST_HANDLE_PARAM(xen_domctl_t));
> -
>  void iommu_dev_iotlb_flush_timeout(struct domain *d, struct pci_dev *pdev);
>  
>  /*
> -- 
> 2.34.1
>