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

[Jürgen Groß <jgross@xxxxxxxx>]

On 24.09.19 10:39, Jan Beulich wrote:
> 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?
Yes. That was Andrew, right?

> 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?
Hmm, I'm not sure this will make things easier.

The requested sequence is:

- copy the byte with the XEN_RUNSTATE_UPDATE bit set first
- copy all other bytes (not clearing the XEN_RUNSTATE_UPDATE bit)
- copy the byte with the now cleared XEN_RUNSTATE_UPDATE bit last

And this has to work for 64- and 32-bit variants of the structure.

So dropping the last hunk is wrong already, and I don't think having
a local copy of state_entry_time only will make things easier, as
you'd need to:

- copy the byte with the XEN_RUNSTATE_UPDATE bit set first
- copy v->runstate.state
- copy local runstate.state_entry_time
- copy v->runstate.time[]
- copy the byte with the now cleared XEN_RUNSTATE_UPDATE bit last

And you'd need to special case the compat case.


Juergen

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