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@suse.com>
---
Another possibility would even be to merge iommu_do_domctl() completely
into do_domctl(), but I wanted to start with a less intrusive variant.
V3:
- new patch
---
 xen/arch/arm/domctl.c | 11 +----------
 xen/arch/x86/domctl.c |  2 +-
 xen/common/domctl.c   |  7 +++++++
 3 files changed, 9 insertions(+), 11 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;
-- 
2.34.1

On 19/04/2022 14:52, 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@suse.com>
> ---
> Another possibility would even be to merge iommu_do_domctl() completely
> into do_domctl(), but I wanted to start with a less intrusive variant.
> V3:
> - new patch

I definitely prefer this approach, thanks.  In addition to being
clearer, it's also faster because there isn't a long line of "do you
understand this subop?" calls when we know exactly what it is.

However, I think we need stub for the !HAS_PASSTHROUGH case now that it
is being called from common code.

I'd forgotten that it was used on ARM now, and yes - it absolutely
should be called from somewhere common, not from the arch hooks.

~Andrew

On 19.04.22 16:51, Andrew Cooper wrote:
> On 19/04/2022 14:52, 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@suse.com>
>> ---
>> Another possibility would even be to merge iommu_do_domctl() completely
>> into do_domctl(), but I wanted to start with a less intrusive variant.
>> V3:
>> - new patch
> 
> I definitely prefer this approach, thanks.  In addition to being
> clearer, it's also faster because there isn't a long line of "do you
> understand this subop?" calls when we know exactly what it is.
> 
> However, I think we need stub for the !HAS_PASSTHROUGH case now that it
> is being called from common code.

Okay, I'll add it. Jan, are you fine with a stub?


Juergen

On 19.04.2022 16:56, Juergen Gross wrote:
> On 19.04.22 16:51, Andrew Cooper wrote:
>> On 19/04/2022 14:52, 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@suse.com>
>>> ---
>>> Another possibility would even be to merge iommu_do_domctl() completely
>>> into do_domctl(), but I wanted to start with a less intrusive variant.
>>> V3:
>>> - new patch
>>
>> I definitely prefer this approach, thanks.  In addition to being
>> clearer, it's also faster because there isn't a long line of "do you
>> understand this subop?" calls when we know exactly what it is.
>>
>> However, I think we need stub for the !HAS_PASSTHROUGH case now that it
>> is being called from common code.
> 
> Okay, I'll add it. Jan, are you fine with a stub?

Sure.

Jan

On 19.04.2022 15:52, Juergen Gross wrote:
> --- 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:

As said in reply to Andrew, I'm not convinced having these enumerated here
is a good idea. But in any event the whole addition wants framing by
#ifdef CONFIG_HAS_PASSTHROUGH now.

For x86 I wonder whether the adjustment wouldn't allow to drop domctl.c's
including of xen/iommu.h (but perhaps it's included indirectly anyway).

Jan

> +        ret = iommu_do_domctl(op, d, u_domctl);
> +        break;
> +
>      default:
>          ret = arch_do_domctl(op, d, u_domctl);
>          break;