Re: [PATCH 1/2] xen/arm: Convert runstate address during hypcall

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Thu, 11 Jun 2020, Julien Grall wrote:
> > > +        return -EINVAL;
> > >      }
> > >
> > > -    __copy_to_guest(runstate_guest(v), &runstate, 1);
> > > +    v->arch.runstate_guest.page = page;
> > > +    v->arch.runstate_guest.offset = offset;
> > > +
> > > +    spin_unlock(&v->arch.runstate_guest.lock);
> > > +
> > > +    return 0;
> > > +}
> > > +
> > > +
> > > +/* Update per-VCPU guest runstate shared memory area (if registered). */
> > > +static void update_runstate_area(struct vcpu *v)
> > > +{
> > > +    struct vcpu_runstate_info *guest_runstate;
> > > +    void *p;
> > > +
> > > +    spin_lock(&v->arch.runstate_guest.lock);
> > >
> > > -    if ( guest_handle )
> > > +    if ( v->arch.runstate_guest.page )
> > >      {
> > > -        runstate.state_entry_time &= ~XEN_RUNSTATE_UPDATE;
> > > +        p = __map_domain_page(v->arch.runstate_guest.page);
> > > +        guest_runstate = p + v->arch.runstate_guest.offset;
> > > +
> > > +        if ( VM_ASSIST(v->domain, runstate_update_flag) )
> > > +        {
> > > +            v->runstate.state_entry_time |= XEN_RUNSTATE_UPDATE;
> > > +            guest_runstate->state_entry_time |= XEN_RUNSTATE_UPDATE;
> >
> > I think that this write to guest_runstate should use write_atomic or
> > another atomic write operation.
> 
> I thought about suggesting the same, but  guest_copy_* helpers may not
> do a single memory write to state_entry_time.
> What are you trying to prevent with the write_atomic()?

I am thinking that without using an atomic write, it would be (at least
theoretically) possible for a guest to see a partial write to
state_entry_time, which is not good. In theory, the set of assembly
instructions generated by the compiler could go through an intermediate
state that we don't want the guest to see. In practice, I doubt that any
possible combination of assembly instructions generated by the compiler
could lead to something harmful.