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 11:03, Jürgen Groß wrote:
> 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?

Oh, sorry, I had actually meant to ask at the same time: The mail
describing the issue came from him, but it saying "After a
complicated investigation", and based on prior instances I wouldn't
be certain it wasn't one of his colleagues Cc-ed there who actually
isolated it. Andrew, could you clarify?

>> 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,

For big-endian - yes. For little endian, if there's no 64-bit
store available, I agree as well. But Arm32 e.g. does have a
suitable store insn, which iirc is even atomic when used with
a sufficiently aligned memory address.

> 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

The plan was to simply copy the entire state_entry_time last.
But yes, I can see how we might make too many assumptions on
how the guest-copying functions work, or demand too much of
extra special casing there by an architecture.

I'm not overly happy with the current model, but based on the
above
Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>

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

There's already a suitable if() / else covering this.

Jan

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