Re: [Xen-devel] [PATCH v15 14/14] x86/domctl: Don't pause the whole domain if only getting vcpu state

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 03.08.18 at 15:53, <aisaila@xxxxxxxxxxxxxxx> wrote:
> --- a/xen/arch/x86/domctl.c
> +++ b/xen/arch/x86/domctl.c
> @@ -591,12 +591,12 @@ long arch_do_domctl(
>               !is_hvm_domain(d) )
>              break;
>  
> -        domain_pause(d);
> +        vcpu_pause(d->vcpu[domctl->u.hvmcontext_partial.instance]);
>          ret = hvm_save_one(d, domctl->u.hvmcontext_partial.type,
>                             domctl->u.hvmcontext_partial.instance,
>                             domctl->u.hvmcontext_partial.buffer,
>                             &domctl->u.hvmcontext_partial.bufsz);
> -        domain_unpause(d);
> +        vcpu_unpause(d->vcpu[domctl->u.hvmcontext_partial.instance]);

Same issue here - there's no bounds check of
domctl->u.hvmcontext_partial.instance before its use as array index.
I'm afraid you can't do the pausing here anymore, both for this reason
and because you still need to pause the whole domain for HVMSR_PER_DOM
type records. Yet you'll know the type only inside hvm_save_one().

> --- a/xen/arch/x86/hvm/save.c
> +++ b/xen/arch/x86/hvm/save.c
> @@ -138,6 +138,7 @@ int hvm_save_one(struct domain *d, unsigned int typecode, 
> unsigned int instance,
>      int rv;
>      hvm_domain_context_t ctxt = { };
>      const struct hvm_save_descriptor *desc;
> +    uint32_t off = 0;

Why does this get moved here?

> @@ -157,29 +156,23 @@ int hvm_save_one(struct domain *d, unsigned int 
> typecode, unsigned int instance,
>                 d->domain_id, typecode, rv);
>      else if ( rv = -ENOENT, ctxt.cur >= sizeof(*desc) )
>      {
> -        uint32_t off;
> -
> -        for ( off = 0; off <= (ctxt.cur - sizeof(*desc)); off += 
> desc->length )
> +        desc = (void *)(ctxt.data + off);
> +        /* Move past header */
> +        off += sizeof(*desc);
> +        if ( ctxt.cur < desc->length ||
> +             off > ctxt.cur - desc->length )
> +            rv = -EFAULT;
> +        if ( instance == desc->instance )
>          {
> -            desc = (void *)(ctxt.data + off);
> -            /* Move past header */
> -            off += sizeof(*desc);
> -            if ( ctxt.cur < desc->length ||
> -                 off > ctxt.cur - desc->length )
> -                break;
> -            if ( instance == desc->instance )
> -            {
> -                rv = 0;
> -                if ( guest_handle_is_null(handle) )
> -                    *bufsz = desc->length;
> -                else if ( *bufsz < desc->length )
> -                    rv = -ENOBUFS;
> -                else if ( copy_to_guest(handle, ctxt.data + off, 
> desc->length) )
> -                    rv = -EFAULT;
> -                else
> -                    *bufsz = desc->length;
> -                break;
> -            }

You can't just delete this loop - it's still needed for multi-instance
records which aren't per-vCPU (PIC is the only example right now
iirc). Since the instance is going to be the correct one for
HVMSR_PER_VCPU type records, can't you simply leave the code
here alone?

Jan



_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel