Re: [Xen-devel] [PATCH] xen/sched: don't let XEN_RUNSTATE_UPDATE leak into vcpu_runstate_get()

[Jan Beulich <jbeulich@xxxxxxxx>]

On 24.09.2019 09:42, Juergen Gross wrote:
> vcpu_runstate_get() should never return a state entry time with
> XEN_RUNSTATE_UPDATE set. To avoid this let update_runstate_area()
> operate on a local runstate copy.
> 
> This problem was introduced with commit 2529c850ea48f036 ("add update
> indicator to vcpu_runstate_info").
> 
> Signed-off-by: Juergen Gross <jgross@xxxxxxxx>

Perhaps this also wants a Reported-by tag?

In principle the change is fine, but I wonder whether you're (a)
going a little too far and thus you are (b) missing some cleanup
potential:

> --- a/xen/arch/x86/domain.c
> +++ b/xen/arch/x86/domain.c
> @@ -1600,21 +1600,24 @@ bool update_runstate_area(struct vcpu *v)
>      bool rc;
>      struct guest_memory_policy policy = { .nested_guest_mode = false };
>      void __user *guest_handle = NULL;
> +    struct vcpu_runstate_info runstate;

I don't think the full structure needs copying. You already use ...

>      if ( guest_handle_is_null(runstate_guest(v)) )
>          return true;
>  
>      update_guest_memory_policy(v, &policy);
>  
> +    memcpy(&runstate, &v->runstate, sizeof(runstate));
> +
>      if ( VM_ASSIST(v->domain, runstate_update_flag) )
>      {
>          guest_handle = has_32bit_shinfo(v->domain)
>              ? &v->runstate_guest.compat.p->state_entry_time + 1
>              : &v->runstate_guest.native.p->state_entry_time + 1;
>          guest_handle--;

... trickery to get at just the state_entry_time field. I think
you would get away with making a local copy of just that one,
thus ...

> -        v->runstate.state_entry_time |= XEN_RUNSTATE_UPDATE;
> +        runstate.state_entry_time |= XEN_RUNSTATE_UPDATE;
>          __raw_copy_to_guest(guest_handle,
> -                            (void *)(&v->runstate.state_entry_time + 1) - 1, 
> 1);
> +                            (void *)(&runstate.state_entry_time + 1) - 1, 1);

... reducing the complexity of this at least a little, while ...

> @@ -1622,20 +1625,20 @@ bool update_runstate_area(struct vcpu *v)
>      {
>          struct compat_vcpu_runstate_info info;
>  
> -        XLAT_vcpu_runstate_info(&info, &v->runstate);
> +        XLAT_vcpu_runstate_info(&info, &runstate);
>          __copy_to_guest(v->runstate_guest.compat, &info, 1);
>          rc = true;
>      }
>      else
> -        rc = __copy_to_guest(runstate_guest(v), &v->runstate, 1) !=
> -             sizeof(v->runstate);
> +        rc = __copy_to_guest(runstate_guest(v), &runstate, 1) !=
> +             sizeof(runstate);

... taking the opportunity to make this use __copy_to_guest_field()
(storing state_entry_time last), in turn allowing ...

>      if ( guest_handle )
>      {
> -        v->runstate.state_entry_time &= ~XEN_RUNSTATE_UPDATE;
> +        runstate.state_entry_time &= ~XEN_RUNSTATE_UPDATE;
>          smp_wmb();
>          __raw_copy_to_guest(guest_handle,
> -                            (void *)(&v->runstate.state_entry_time + 1) - 1, 
> 1);
> +                            (void *)(&runstate.state_entry_time + 1) - 1, 1);
>      }

... to drop this altogether.

Thoughts?

Jan

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