* [Xen-devel] [PATCH] xen/sched: don't let XEN_RUNSTATE_UPDATE leak into vcpu_runstate_get()
@ 2019-09-24 7:42 Juergen Gross
2019-09-24 8:39 ` Jan Beulich
2019-09-24 15:16 ` Jan Beulich
0 siblings, 2 replies; 6+ messages in thread
From: Juergen Gross @ 2019-09-24 7:42 UTC (permalink / raw)
To: xen-devel
Cc: Juergen Gross, Andrew Cooper, Wei Liu, Jan Beulich, Roger Pau Monné
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@suse.com>
---
xen/arch/x86/domain.c | 17 ++++++++++-------
1 file changed, 10 insertions(+), 7 deletions(-)
diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c
index dbdf6b1bc2..c4eceaab3f 100644
--- 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;
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--;
- 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);
smp_wmb();
}
@@ -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);
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);
}
update_guest_memory_policy(v, &policy);
--
2.16.4
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [Xen-devel] [PATCH] xen/sched: don't let XEN_RUNSTATE_UPDATE leak into vcpu_runstate_get()
2019-09-24 7:42 [Xen-devel] [PATCH] xen/sched: don't let XEN_RUNSTATE_UPDATE leak into vcpu_runstate_get() Juergen Gross
@ 2019-09-24 8:39 ` Jan Beulich
2019-09-24 9:03 ` Jürgen Groß
2019-09-24 15:16 ` Jan Beulich
1 sibling, 1 reply; 6+ messages in thread
From: Jan Beulich @ 2019-09-24 8:39 UTC (permalink / raw)
To: Juergen Gross; +Cc: xen-devel, Roger Pau Monné, Wei Liu, Andrew Cooper
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@suse.com>
Perhaps this also wants a Reported-by tag?
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?
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Xen-devel] [PATCH] xen/sched: don't let XEN_RUNSTATE_UPDATE leak into vcpu_runstate_get()
2019-09-24 8:39 ` Jan Beulich
@ 2019-09-24 9:03 ` Jürgen Groß
2019-09-24 9:26 ` Jan Beulich
0 siblings, 1 reply; 6+ messages in thread
From: Jürgen Groß @ 2019-09-24 9:03 UTC (permalink / raw)
To: Jan Beulich; +Cc: xen-devel, Roger Pau Monné, Wei Liu, Andrew Cooper
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@suse.com>
>
> 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@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Xen-devel] [PATCH] xen/sched: don't let XEN_RUNSTATE_UPDATE leak into vcpu_runstate_get()
2019-09-24 9:03 ` Jürgen Groß
@ 2019-09-24 9:26 ` Jan Beulich
0 siblings, 0 replies; 6+ messages in thread
From: Jan Beulich @ 2019-09-24 9:26 UTC (permalink / raw)
To: Jürgen Groß, Andrew Cooper
Cc: xen-devel, Wei Liu, Roger Pau Monné
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@suse.com>
>>
>> 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@suse.com>
> 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@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Xen-devel] [PATCH] xen/sched: don't let XEN_RUNSTATE_UPDATE leak into vcpu_runstate_get()
2019-09-24 7:42 [Xen-devel] [PATCH] xen/sched: don't let XEN_RUNSTATE_UPDATE leak into vcpu_runstate_get() Juergen Gross
2019-09-24 8:39 ` Jan Beulich
@ 2019-09-24 15:16 ` Jan Beulich
2019-09-24 15:22 ` Jürgen Groß
1 sibling, 1 reply; 6+ messages in thread
From: Jan Beulich @ 2019-09-24 15:16 UTC (permalink / raw)
To: Juergen Gross; +Cc: xen-devel, Roger Pau Monné, Wei Liu, Andrew Cooper
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@suse.com>
> ---
> xen/arch/x86/domain.c | 17 ++++++++++-------
> 1 file changed, 10 insertions(+), 7 deletions(-)
I had this committed already and was about to push, but this
definitely wants a similar change for Arm.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Xen-devel] [PATCH] xen/sched: don't let XEN_RUNSTATE_UPDATE leak into vcpu_runstate_get()
2019-09-24 15:16 ` Jan Beulich
@ 2019-09-24 15:22 ` Jürgen Groß
0 siblings, 0 replies; 6+ messages in thread
From: Jürgen Groß @ 2019-09-24 15:22 UTC (permalink / raw)
To: Jan Beulich; +Cc: xen-devel, Roger Pau Monné, Wei Liu, Andrew Cooper
On 24.09.19 17:16, 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@suse.com>
>> ---
>> xen/arch/x86/domain.c | 17 ++++++++++-------
>> 1 file changed, 10 insertions(+), 7 deletions(-)
>
> I had this committed already and was about to push, but this
> definitely wants a similar change for Arm.
Oh, I was fooled by cscope again, which gets fed an arch specific
file list.
Sending V2 soon.
Juergen
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2019-09-24 15:22 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-09-24 7:42 [Xen-devel] [PATCH] xen/sched: don't let XEN_RUNSTATE_UPDATE leak into vcpu_runstate_get() Juergen Gross
2019-09-24 8:39 ` Jan Beulich
2019-09-24 9:03 ` Jürgen Groß
2019-09-24 9:26 ` Jan Beulich
2019-09-24 15:16 ` Jan Beulich
2019-09-24 15:22 ` Jürgen Groß
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.