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

[Julien Grall <julien@xxxxxxx>]

Hi Stefano,

On 11/06/2020 19:50, Stefano Stabellini wrote:
> 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. 
It is already the case with existing implementation as Xen may write 
byte by byte. So are you suggesting the existing code is also buggy?
> 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.
Well, I think you first need to define the theoritical state you are 
worried about. Then we can discuss whether they can happen in practice 
and how we can prevent them in the existing and new code.
So what sort of state your are concerned?

Cheers,

--
Julien Grall