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

[Julien Grall <julien@xen.org>]

On 12/06/2020 02:09, Stefano Stabellini wrote:
> On Thu, 11 Jun 2020, Julien Grall wrote:
>> 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?

It looks like I may have misread the code as we only copy one byte. But 
I still think this is fragile.

For this context, I agree that a write_atomic() should do the job.

However, I still want to answer to your comments below.

> 
> Writing byte by byte is a different case. That is OK. In that case, the
> guest could see the state after 3 bytes written and it would be fine and
> consistent.

Why? What does actually prevent a guest to see an in-between value?

To give a concrete example, if the original value is 0xabc and you want 
to write 0xdef. Why would the guest never see 0xabf or 0xaec?

> If this hadn't been the case, then yes, the existing code
> would also be buggy.
> 
> So if we did the write with a memcpy, it would be fine, no need for
> atomics:
> 
>    memcpy(&guest_runstate->state_entry_time,
>           &v->runstate.state_entry_time,
>           XXX);
> 
> 
> The |= case is different: GCC could implement it in any way it likes,
> including going through a zero-write to any of the bytes in the word, or
> doing an addition then a subtraction. GCC doesn't make any guarantees.
> If we want guarantees we need to use atomics.

Yes GCC can generate assembly for |= in any way it likes. But so does 
for memcpy(). So I still don't understand why one would be fine for you 
and not the other...

Cheers,

-- 
Julien Grall