[v8] xen/domain: rewrite emulation_flags_ok()

[PATCH v8] xen/domain: rewrite emulation_flags_ok()

Posted by dmkhn@proton.me 4 months, 2 weeks ago

From: Denis Mukhin <dmukhin@ford.com>

Rewrite emulation_flags_ok() to simplify future modifications.

No functional change intended.

Signed-off-by: Denis Mukhin <dmukhin@ford.com>
---
Changes since v7:
- expanded the code commentary
- added named type for domain capabilities

Link to v7: https://lore.kernel.org/xen-devel/20250610144500.3176661-1-dmukhin@ford.com/
Link to CI: https://gitlab.com/xen-project/people/dmukhin/xen/-/pipelines/1873169059
---
 xen/arch/x86/domain.c | 79 +++++++++++++++++++++++++++++++++----------
 1 file changed, 61 insertions(+), 18 deletions(-)

diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c
index 7536b6c8717e..d2049e4c636b 100644
--- a/xen/arch/x86/domain.c
+++ b/xen/arch/x86/domain.c
@@ -743,32 +743,75 @@ int arch_sanitise_domain_config(struct xen_domctl_createdomain *config)
     return 0;
 }
 
+/*
+ * Verify that the domain's emulation flags resolve to a supported configuration.
+ *
+ * This ensures we only allow a known, safe subset of emulation combinations
+ * (for both functionality and security). Arbitrary mixes are likely to cause
+ * errors (e.g. null pointer dereferences).
+ *
+ * NB: use the internal X86_EMU_XXX symbols, not the public XEN_X86_EMU_XXX
+ * symbols, to take build-time config options (e.g. CONFIG_HVM) into account
+ * for short-circuited emulations.
+ */
 static bool emulation_flags_ok(const struct domain *d, uint32_t emflags)
 {
+    enum domain_capability {
+        CAP_PV          = BIT(0, U),
+        CAP_HVM         = BIT(1, U),
+        CAP_HWDOM       = BIT(2, U),
+        CAP_DOMU        = BIT(3, U),
+    };
+    static const struct {
+        enum domain_capability caps;
+        uint32_t min;
+        uint32_t opt;
+    } configs[] = {
+#ifdef CONFIG_PV
+        /* PV dom0 and domU */
+        {
+            .caps   = CAP_PV | CAP_HWDOM | CAP_DOMU,
+            .opt    = X86_EMU_PIT,
+        },
+#endif /* #ifdef CONFIG_PV */
+
+#ifdef CONFIG_HVM
+        /* PVH dom0 */
+        {
+            .caps   = CAP_HVM | CAP_HWDOM,
+            .min    = X86_EMU_LAPIC | X86_EMU_IOAPIC | X86_EMU_VPCI,
+        },
+
+        /* PVH domU */
+        {
+            .caps   = CAP_HVM | CAP_DOMU,
+            .min    = X86_EMU_LAPIC,
+        },
+
+        /* HVM domU */
+        {
+            .caps   = CAP_HVM | CAP_DOMU,
+            .min    = X86_EMU_ALL & ~(X86_EMU_VPCI | X86_EMU_USE_PIRQ),
+            /* HVM PIRQ feature is user-selectable. */
+            .opt    = X86_EMU_USE_PIRQ,
+        },
+#endif /* #ifdef CONFIG_HVM */
+    };
+    unsigned int i;
+    enum domain_capability caps = (is_pv_domain(d) ? CAP_PV : CAP_HVM) |
+                                  (is_hardware_domain(d) ? CAP_HWDOM : CAP_DOMU);
+
 #ifdef CONFIG_HVM
     /* This doesn't catch !CONFIG_HVM case but it is better than nothing */
     BUILD_BUG_ON(X86_EMU_ALL != XEN_X86_EMU_ALL);
 #endif
 
-    if ( is_hvm_domain(d) )
-    {
-        if ( is_hardware_domain(d) &&
-             emflags != (X86_EMU_VPCI | X86_EMU_LAPIC | X86_EMU_IOAPIC) )
-            return false;
-        if ( !is_hardware_domain(d) &&
-             /* HVM PIRQ feature is user-selectable. */
-             (emflags & ~X86_EMU_USE_PIRQ) !=
-             (X86_EMU_ALL & ~(X86_EMU_VPCI | X86_EMU_USE_PIRQ)) &&
-             emflags != X86_EMU_LAPIC )
-            return false;
-    }
-    else if ( emflags != 0 && emflags != X86_EMU_PIT )
-    {
-        /* PV or classic PVH. */
-        return false;
-    }
+    for ( i = 0; i < ARRAY_SIZE(configs); i++ )
+        if ( (caps & configs[i].caps) == caps &&
+             (emflags & ~configs[i].opt) == configs[i].min )
+            return true;
 
-    return true;
+    return false;
 }
 
 void __init arch_init_idle_domain(struct domain *d)
-- 
2.34.1

Re: [PATCH v8] xen/domain: rewrite emulation_flags_ok()

Posted by Jan Beulich 4 months, 2 weeks ago

On 17.06.2025 03:15, dmkhn@proton.me wrote:
> --- a/xen/arch/x86/domain.c
> +++ b/xen/arch/x86/domain.c
> @@ -743,32 +743,75 @@ int arch_sanitise_domain_config(struct xen_domctl_createdomain *config)
>      return 0;
>  }
>  
> +/*
> + * Verify that the domain's emulation flags resolve to a supported configuration.
> + *
> + * This ensures we only allow a known, safe subset of emulation combinations
> + * (for both functionality and security). Arbitrary mixes are likely to cause
> + * errors (e.g. null pointer dereferences).
> + *
> + * NB: use the internal X86_EMU_XXX symbols, not the public XEN_X86_EMU_XXX
> + * symbols, to take build-time config options (e.g. CONFIG_HVM) into account
> + * for short-circuited emulations.
> + */
>  static bool emulation_flags_ok(const struct domain *d, uint32_t emflags)
>  {
> +    enum domain_capability {
> +        CAP_PV          = BIT(0, U),
> +        CAP_HVM         = BIT(1, U),
> +        CAP_HWDOM       = BIT(2, U),
> +        CAP_DOMU        = BIT(3, U),
> +    };
> +    static const struct {
> +        enum domain_capability caps;
> +        uint32_t min;
> +        uint32_t opt;
> +    } configs[] = {
> +#ifdef CONFIG_PV
> +        /* PV dom0 and domU */
> +        {
> +            .caps   = CAP_PV | CAP_HWDOM | CAP_DOMU,

Just to double check - are we sure Misra / Eclair will like this (ab)use
of an enum?

Jan

Re: [PATCH v8] xen/domain: rewrite emulation_flags_ok()

Posted by Nicola Vetrini 4 months, 2 weeks ago

On 2025-06-17 08:19, Jan Beulich wrote:
> On 17.06.2025 03:15, dmkhn@proton.me wrote:
>> --- a/xen/arch/x86/domain.c
>> +++ b/xen/arch/x86/domain.c
>> @@ -743,32 +743,75 @@ int arch_sanitise_domain_config(struct 
>> xen_domctl_createdomain *config)
>>      return 0;
>>  }
>> 
>> +/*
>> + * Verify that the domain's emulation flags resolve to a supported 
>> configuration.
>> + *
>> + * This ensures we only allow a known, safe subset of emulation 
>> combinations
>> + * (for both functionality and security). Arbitrary mixes are likely 
>> to cause
>> + * errors (e.g. null pointer dereferences).
>> + *
>> + * NB: use the internal X86_EMU_XXX symbols, not the public 
>> XEN_X86_EMU_XXX
>> + * symbols, to take build-time config options (e.g. CONFIG_HVM) into 
>> account
>> + * for short-circuited emulations.
>> + */
>>  static bool emulation_flags_ok(const struct domain *d, uint32_t 
>> emflags)
>>  {
>> +    enum domain_capability {
>> +        CAP_PV          = BIT(0, U),
>> +        CAP_HVM         = BIT(1, U),
>> +        CAP_HWDOM       = BIT(2, U),
>> +        CAP_DOMU        = BIT(3, U),
>> +    };
>> +    static const struct {
>> +        enum domain_capability caps;
>> +        uint32_t min;
>> +        uint32_t opt;
>> +    } configs[] = {
>> +#ifdef CONFIG_PV
>> +        /* PV dom0 and domU */
>> +        {
>> +            .caps   = CAP_PV | CAP_HWDOM | CAP_DOMU,
> 
> Just to double check - are we sure Misra / Eclair will like this 
> (ab)use
> of an enum?
> 
> Jan

Likely not, but x86_64 is build with CONFIG_PV=n

-- 
Nicola Vetrini, B.Sc.
Software Engineer
BUGSENG (https://bugseng.com)
LinkedIn: https://www.linkedin.com/in/nicola-vetrini-a42471253

Re: [PATCH v8] xen/domain: rewrite emulation_flags_ok()

Posted by Jan Beulich 4 months, 2 weeks ago

On 17.06.2025 08:53, Nicola Vetrini wrote:
> On 2025-06-17 08:19, Jan Beulich wrote:
>> On 17.06.2025 03:15, dmkhn@proton.me wrote:
>>> --- a/xen/arch/x86/domain.c
>>> +++ b/xen/arch/x86/domain.c
>>> @@ -743,32 +743,75 @@ int arch_sanitise_domain_config(struct 
>>> xen_domctl_createdomain *config)
>>>      return 0;
>>>  }
>>>
>>> +/*
>>> + * Verify that the domain's emulation flags resolve to a supported 
>>> configuration.
>>> + *
>>> + * This ensures we only allow a known, safe subset of emulation 
>>> combinations
>>> + * (for both functionality and security). Arbitrary mixes are likely 
>>> to cause
>>> + * errors (e.g. null pointer dereferences).
>>> + *
>>> + * NB: use the internal X86_EMU_XXX symbols, not the public 
>>> XEN_X86_EMU_XXX
>>> + * symbols, to take build-time config options (e.g. CONFIG_HVM) into 
>>> account
>>> + * for short-circuited emulations.
>>> + */
>>>  static bool emulation_flags_ok(const struct domain *d, uint32_t 
>>> emflags)
>>>  {
>>> +    enum domain_capability {
>>> +        CAP_PV          = BIT(0, U),
>>> +        CAP_HVM         = BIT(1, U),
>>> +        CAP_HWDOM       = BIT(2, U),
>>> +        CAP_DOMU        = BIT(3, U),
>>> +    };
>>> +    static const struct {
>>> +        enum domain_capability caps;
>>> +        uint32_t min;
>>> +        uint32_t opt;
>>> +    } configs[] = {
>>> +#ifdef CONFIG_PV
>>> +        /* PV dom0 and domU */
>>> +        {
>>> +            .caps   = CAP_PV | CAP_HWDOM | CAP_DOMU,
>>
>> Just to double check - are we sure Misra / Eclair will like this 
>> (ab)use
>> of an enum?
> 
> Likely not, but x86_64 is build with CONFIG_PV=n

CONFIG_HVM code further down in the patch is pretty similar.

Jan

Re: [PATCH v8] xen/domain: rewrite emulation_flags_ok()

Posted by Roger Pau Monné 4 months, 2 weeks ago

On Tue, Jun 17, 2025 at 08:53:51AM +0200, Nicola Vetrini wrote:
> On 2025-06-17 08:19, Jan Beulich wrote:
> > On 17.06.2025 03:15, dmkhn@proton.me wrote:
> > > --- a/xen/arch/x86/domain.c
> > > +++ b/xen/arch/x86/domain.c
> > > @@ -743,32 +743,75 @@ int arch_sanitise_domain_config(struct
> > > xen_domctl_createdomain *config)
> > >      return 0;
> > >  }
> > > 
> > > +/*
> > > + * Verify that the domain's emulation flags resolve to a supported
> > > configuration.
> > > + *
> > > + * This ensures we only allow a known, safe subset of emulation
> > > combinations
> > > + * (for both functionality and security). Arbitrary mixes are
> > > likely to cause
> > > + * errors (e.g. null pointer dereferences).
> > > + *
> > > + * NB: use the internal X86_EMU_XXX symbols, not the public
> > > XEN_X86_EMU_XXX
> > > + * symbols, to take build-time config options (e.g. CONFIG_HVM)
> > > into account
> > > + * for short-circuited emulations.
> > > + */
> > >  static bool emulation_flags_ok(const struct domain *d, uint32_t
> > > emflags)
> > >  {
> > > +    enum domain_capability {
> > > +        CAP_PV          = BIT(0, U),
> > > +        CAP_HVM         = BIT(1, U),
> > > +        CAP_HWDOM       = BIT(2, U),
> > > +        CAP_DOMU        = BIT(3, U),
> > > +    };
> > > +    static const struct {
> > > +        enum domain_capability caps;
> > > +        uint32_t min;
> > > +        uint32_t opt;
> > > +    } configs[] = {
> > > +#ifdef CONFIG_PV
> > > +        /* PV dom0 and domU */
> > > +        {
> > > +            .caps   = CAP_PV | CAP_HWDOM | CAP_DOMU,
> > 
> > Just to double check - are we sure Misra / Eclair will like this (ab)use
> > of an enum?
> > 
> > Jan
> 
> Likely not, but x86_64 is build with CONFIG_PV=n

It's doing the same for HVM also, so it would trigger for the instance
below then.  I will ack v7 then, which used an unsigned int instead.

Thanks, Roger.