* [PATCH v7] xen/domain: rewrite emulation_flags_ok()
@ 2025-06-10 14:45 dmkhn
2025-06-16 14:03 ` Roger Pau Monné
0 siblings, 1 reply; 3+ messages in thread
From: dmkhn @ 2025-06-10 14:45 UTC (permalink / raw)
To: xen-devel
Cc: andrew.cooper3, anthony.perard, jbeulich, julien, michal.orzel,
roger.pau, sstabellini, teddy.astie, dmukhin
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 v6:
- simplified checks for PV further
Link to v6: https://lore.kernel.org/xen-devel/20250610004216.3012253-1-dmukhin@ford.com/
Link to CI: https://gitlab.com/xen-project/people/dmukhin/xen/-/pipelines/1862559089
---
xen/arch/x86/domain.c | 78 +++++++++++++++++++++++++++++++++----------
1 file changed, 60 insertions(+), 18 deletions(-)
diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c
index 7536b6c871..fdbd064ebf 100644
--- a/xen/arch/x86/domain.c
+++ b/xen/arch/x86/domain.c
@@ -743,32 +743,74 @@ 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.
+ */
static bool emulation_flags_ok(const struct domain *d, uint32_t emflags)
{
+ enum {
+ CAP_PV = BIT(0, U),
+ CAP_HVM = BIT(1, U),
+ CAP_HWDOM = BIT(2, U),
+ CAP_DOMU = BIT(3, U),
+ };
+ static const struct {
+ unsigned int 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;
+ unsigned int 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
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH v7] xen/domain: rewrite emulation_flags_ok()
2025-06-10 14:45 [PATCH v7] xen/domain: rewrite emulation_flags_ok() dmkhn
@ 2025-06-16 14:03 ` Roger Pau Monné
2025-06-17 7:20 ` Roger Pau Monné
0 siblings, 1 reply; 3+ messages in thread
From: Roger Pau Monné @ 2025-06-16 14:03 UTC (permalink / raw)
To: dmkhn
Cc: xen-devel, andrew.cooper3, anthony.perard, jbeulich, julien,
michal.orzel, sstabellini, teddy.astie, dmukhin
On Tue, Jun 10, 2025 at 02:45:12PM +0000, dmkhn@proton.me wrote:
> 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 v6:
> - simplified checks for PV further
>
> Link to v6: https://lore.kernel.org/xen-devel/20250610004216.3012253-1-dmukhin@ford.com/
> Link to CI: https://gitlab.com/xen-project/people/dmukhin/xen/-/pipelines/1862559089
> ---
> xen/arch/x86/domain.c | 78 +++++++++++++++++++++++++++++++++----------
> 1 file changed, 60 insertions(+), 18 deletions(-)
>
> diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c
> index 7536b6c871..fdbd064ebf 100644
> --- a/xen/arch/x86/domain.c
> +++ b/xen/arch/x86/domain.c
> @@ -743,32 +743,74 @@ 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.
Not sure if it's worth expanding the sentence a bit to add the reason
why X86_EMU_XXX should be used (so that we take build-time config
options into account for short-circuited emulations).
> + */
> static bool emulation_flags_ok(const struct domain *d, uint32_t emflags)
> {
> + enum {
> + CAP_PV = BIT(0, U),
> + CAP_HVM = BIT(1, U),
> + CAP_HWDOM = BIT(2, U),
> + CAP_DOMU = BIT(3, U),
> + };
You probably want to name this type.
> + static const struct {
> + unsigned int caps;
So it can be used here
> + 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;
> + unsigned int caps = (is_pv_domain(d) ? CAP_PV : CAP_HVM) |
> + (is_hardware_domain(d) ? CAP_HWDOM : CAP_DOMU);
And here instead of using unsigned int?
> +
> #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
>
>
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH v7] xen/domain: rewrite emulation_flags_ok()
2025-06-16 14:03 ` Roger Pau Monné
@ 2025-06-17 7:20 ` Roger Pau Monné
0 siblings, 0 replies; 3+ messages in thread
From: Roger Pau Monné @ 2025-06-17 7:20 UTC (permalink / raw)
To: dmkhn
Cc: xen-devel, andrew.cooper3, anthony.perard, jbeulich, julien,
michal.orzel, sstabellini, teddy.astie, dmukhin
On Mon, Jun 16, 2025 at 04:03:14PM +0200, Roger Pau Monné wrote:
> On Tue, Jun 10, 2025 at 02:45:12PM +0000, dmkhn@proton.me wrote:
> > 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 v6:
> > - simplified checks for PV further
> >
> > Link to v6: https://lore.kernel.org/xen-devel/20250610004216.3012253-1-dmukhin@ford.com/
> > Link to CI: https://gitlab.com/xen-project/people/dmukhin/xen/-/pipelines/1862559089
> > ---
> > xen/arch/x86/domain.c | 78 +++++++++++++++++++++++++++++++++----------
> > 1 file changed, 60 insertions(+), 18 deletions(-)
> >
> > diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c
> > index 7536b6c871..fdbd064ebf 100644
> > --- a/xen/arch/x86/domain.c
> > +++ b/xen/arch/x86/domain.c
> > @@ -743,32 +743,74 @@ 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.
>
> Not sure if it's worth expanding the sentence a bit to add the reason
> why X86_EMU_XXX should be used (so that we take build-time config
> options into account for short-circuited emulations).
With this comment adjusted as you have in v8:
Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>
Thanks, Roger.
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2025-06-17 7:20 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-06-10 14:45 [PATCH v7] xen/domain: rewrite emulation_flags_ok() dmkhn
2025-06-16 14:03 ` Roger Pau Monné
2025-06-17 7:20 ` Roger Pau Monné
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.