Re: [for-4.22][PATCH] xen/arm: Fail domain construction if a secondary vCPU cannot be created

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 08/07/2026 8:49 am, Michal Orzel wrote:
> construct_domain() creates the secondary vCPUs in a loop, but on a
> vcpu_create() failure it only prints a message and breaks out of the
> loop returning success. As a result the domain can be constructed
> with fewer vCPUs than d->max_vcpus, leaving NULL holes in d->vcpu[]
> below max_vcpus.

I'd suggest phrasing this as "partially constructed".  "holes" isn't
really what's going on, given the linear nature of allocation.

Because of the object visibility, and because constructing vCPUs isn't
atomic, all code needs to cope with d->vcpu[] having no, some or all of
d->max_vcpus constructed.

>
> When the guest probes the redistributor of a vCPU that was never created,
> get_vcpu_from_rdist() only checks vcpu_id against d->max_vcpus and then
> dereferences the NULL d->vcpu[vcpu_id], resulting in a data abort.

That's unsafe, especially as vcpu_id is calculated from an MMIO access.

diff --git a/xen/arch/arm/vgic-v3.c b/xen/arch/arm/vgic-v3.c
index c1c4d6f71ea8..c01cc596d593 100644
--- a/xen/arch/arm/vgic-v3.c
+++ b/xen/arch/arm/vgic-v3.c
@@ -1111,10 +1111,10 @@ static struct vcpu *get_vcpu_from_rdist(struct
domain *d,
     unsigned int vcpu_id;
 
     vcpu_id = region->first_cpu + ((gpa - region->base) / GICV3_GICR_SIZE);
-    if ( unlikely(vcpu_id >= d->max_vcpus) )
-        return NULL;
 
-    v = d->vcpu[vcpu_id];
+    v = domain_vcpu(d, vcpu_id);
+    if ( !v )
+        return NULL;
 
     *offset = gpa - v->arch.vgic.rdist_base;
 

Do you want me to submit this separately?


>
> Return an error instead of breaking out of the loop. Both callers
> (construct_domU() and construct_hwdom()) already propagate a negative
> return value and fail domain construction, which is the correct
> behaviour: a domain that cannot provide the requested number of vCPUs
> should not be brought up.
>
> Fixes: 6b0e8e43348a ("xen/arm: allocate secondaries dom0 vcpus")
> Signed-off-by: Michal Orzel <michal.orzel@xxxxxxx>
> ---
>  xen/arch/arm/domain_build.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> index 550617f152bb..b46574fd32aa 100644
> --- a/xen/arch/arm/domain_build.c
> +++ b/xen/arch/arm/domain_build.c
> @@ -1847,7 +1847,7 @@ int __init construct_domain(struct domain *d, struct 
> kernel_info *kinfo)
>          if ( vcpu_create(d, i) == NULL )
>          {
>              printk("Failed to allocate d%dv%d\n", d->domain_id, i);
> -            break;
> +            return -EINVAL;
>          }
>  
>          if ( is_64bit_domain(d) )

On x86, we explicitly tolerate a failure to build all of dom0's CPUs, if
at least one did get constructed.  This is to increase the chances that
the server can boot and at least let an admin in to look at things.

However, I can see why such a behaviour is not wanted in a "single
pre-packaged system" as used by automotive.

~Andrew