Re: [PATCH] xen/arm: XEN_DOMCTL_get_address_size hypercall support

[Julien Grall <julien@xxxxxxx>]

Hi,

On 02/10/2025 12:10, Milan Djokic wrote:
> Hello Andrew,
>
> On 10/2/25 12:10, Andrew Cooper wrote:
> > On 01/10/2025 9:01 pm, Milan Djokic wrote:
> > > Signed-off-by: Milan Djokic <milan_djokic@xxxxxxxx>
> > >
> > > ---
> > > XEN_DOMCTL_get_address_size hypercall is not implemented for arm 
> > > (only for x86)
> > > It would be useful to have this hypercall supported for arm64, in 
> > > order to get
> > > current guest addressing mode and also to verify that 
> > > XEN_DOMCTL_set_address_size
> > > performs switch to target addressing mode (instead of relying on its 
> > > returned error code only).
> > Please don't copy this misfeature of x86 PV guests into ARM.
> >
> > Letting domains be of variable bitness after domain create leads to a
> > whole lot of bugs, many security relevant.
> >
> > 32bit vs 64bit should be an input to domain_create(), not something that
> > is edited after the domain has been constructed.
> Yes, the idea behind this patch is not to introduce variable bitness, 
> just to have the ability to get current addressing mode through 
> hypercall. In our case, we have used it only in domain creation path 
> (after domain_create(), to verify that target addressing mode is set).
> Of course, whether this hypercall support would be useful in mainline is 
> open for discussion.
We already have a series under review [1] that follow what Andrew is 
suggesting. I would rather focus on getting it committed rather than 
trying to workaround it.
> > > diff --git a/xen/arch/arm/arm64/domctl.c b/xen/arch/arm/arm64/domctl.c
> > > index 8720d126c9..f227309e06 100644
> > > --- a/xen/arch/arm/arm64/domctl.c
> > > +++ b/xen/arch/arm/arm64/domctl.c
> > > @@ -33,6 +34,37 @@ static long switch_mode(struct domain *d, enum 
> > > domain_type type)
> > >       return 0;
> > >   }
> > > +static long get_address_size(struct domain *d, uint32_t *address_size)
> > > +{
> > > +    long rc = 0;
> > > +    struct vcpu *v;
> > > +    /* Check invalid arguments */
> > > +    if ( d == NULL || address_size == NULL) {
> > > +        rc = -EINVAL;
> > > +    }
> > > +    /* Domain structure type field and actual vcpu mode must be 
> > > aligned */
> > > +    if(rc == 0) {
> > > +        for_each_vcpu(d, v) {
> > > +            if(vcpu_get_mode(v) != d->arch.type) {
> > > +                rc = -EFAULT;
> > > +            }
> > > +        }
> > This is deeply suspicious.
> >
> > Under what circumstances can the vCPU setting be different from the
> > domain setting?
> It should never happen, this is more of a sanity check. Alternative 
> would be to use only one of vCPU or domain settings, checking against 
> both seemed more complete to me.
This would be a logical error in Xen rather than something that could be 
triggered by the toolstack. I feel it could mislead people using release 
build because the fault is not due to the input provided.
Cheers,

--
Julien Grall