1. Patchew
  2. Xen
  3. View series

[Xen-devel] [PATCH v2] xen/sched: don't let XEN_RUNSTATE_UPDATE leak into vcpu_runstate_get()

Juergen Gross posted 1 patch 6 years, 1 month ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/xen tags/patchew/20190925042917.11392-1-jgross@suse.com
There is a newer version of this series
xen/arch/arm/domain.c | 13 ++++++++-----
xen/arch/x86/domain.c | 17 ++++++++++-------
2 files changed, 18 insertions(+), 12 deletions(-)
[Xen-devel] [PATCH v2] xen/sched: don't let XEN_RUNSTATE_UPDATE leak into vcpu_runstate_get()
Posted by Juergen Gross 6 years, 1 month ago 
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").

Reported-by: Andrew Cooper <andrew.cooper3@citrix.com>
Signed-off-by: Juergen Gross <jgross@suse.com>
---
V2: add handling on ARM, too (Jan Beulich)
---
 xen/arch/arm/domain.c | 13 ++++++++-----
 xen/arch/x86/domain.c | 17 ++++++++++-------
 2 files changed, 18 insertions(+), 12 deletions(-)

diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
index ae13e47e86..d681ff5c6e 100644
--- a/xen/arch/arm/domain.c
+++ b/xen/arch/arm/domain.c
@@ -280,28 +280,31 @@ static void ctxt_switch_to(struct vcpu *n)
 static void update_runstate_area(struct vcpu *v)
 {
     void __user *guest_handle = NULL;
+    struct vcpu_runstate_info runstate;
 
     if ( guest_handle_is_null(runstate_guest(v)) )
         return;
 
+    memcpy(&runstate, &v->runstate, sizeof(runstate));
+
     if ( VM_ASSIST(v->domain, runstate_update_flag) )
     {
         guest_handle = &v->runstate_guest.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();
     }
 
-    __copy_to_guest(runstate_guest(v), &v->runstate, 1);
+    __copy_to_guest(runstate_guest(v), &runstate, 1);
 
     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);
     }
 }
 
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
Re: [Xen-devel] [PATCH v2] xen/sched: don't let XEN_RUNSTATE_UPDATE leak into vcpu_runstate_get()
Posted by Jan Beulich 6 years, 1 month ago 
On 25.09.2019 06:29, 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").
> 
> Reported-by: Andrew Cooper <andrew.cooper3@citrix.com>
> Signed-off-by: Juergen Gross <jgross@suse.com>

Once again
Reviewed-by: Jan Beulich <jbeulich@suse.com>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v2] xen/sched: don't let XEN_RUNSTATE_UPDATE leak into vcpu_runstate_get()
Posted by Julien Grall 6 years, 1 month ago 
Hi,

On 9/25/19 5:29 AM, 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").
> 
> Reported-by: Andrew Cooper <andrew.cooper3@citrix.com>
> Signed-off-by: Juergen Gross <jgross@suse.com>
> ---
> V2: add handling on ARM, too (Jan Beulich)
> ---
>   xen/arch/arm/domain.c | 13 ++++++++-----
>   xen/arch/x86/domain.c | 17 ++++++++++-------
>   2 files changed, 18 insertions(+), 12 deletions(-)
> 
> diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
> index ae13e47e86..d681ff5c6e 100644
> --- a/xen/arch/arm/domain.c
> +++ b/xen/arch/arm/domain.c
> @@ -280,28 +280,31 @@ static void ctxt_switch_to(struct vcpu *n)
>   static void update_runstate_area(struct vcpu *v)
>   {
>       void __user *guest_handle = NULL;
> +    struct vcpu_runstate_info runstate;
>   
>       if ( guest_handle_is_null(runstate_guest(v)) )
>           return;
>   
> +    memcpy(&runstate, &v->runstate, sizeof(runstate));

I am not really happy with this solution. AFAICT, you only copy the full 
structure here just for the benefits of updating state_entry_time.

I saw you discuss about it with Jan, so it would be nice to log at least 
in the commit message the reason why this is done like that.

Cheers,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v2] xen/sched: don't let XEN_RUNSTATE_UPDATE leak into vcpu_runstate_get()
Posted by Jürgen Groß 6 years, 1 month ago 
On 26.09.19 16:27, Julien Grall wrote:
> Hi,
> 
> On 9/25/19 5:29 AM, 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").
>>
>> Reported-by: Andrew Cooper <andrew.cooper3@citrix.com>
>> Signed-off-by: Juergen Gross <jgross@suse.com>
>> ---
>> V2: add handling on ARM, too (Jan Beulich)
>> ---
>>   xen/arch/arm/domain.c | 13 ++++++++-----
>>   xen/arch/x86/domain.c | 17 ++++++++++-------
>>   2 files changed, 18 insertions(+), 12 deletions(-)
>>
>> diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
>> index ae13e47e86..d681ff5c6e 100644
>> --- a/xen/arch/arm/domain.c
>> +++ b/xen/arch/arm/domain.c
>> @@ -280,28 +280,31 @@ static void ctxt_switch_to(struct vcpu *n)
>>   static void update_runstate_area(struct vcpu *v)
>>   {
>>       void __user *guest_handle = NULL;
>> +    struct vcpu_runstate_info runstate;
>>       if ( guest_handle_is_null(runstate_guest(v)) )
>>           return;
>> +    memcpy(&runstate, &v->runstate, sizeof(runstate));
> 
> I am not really happy with this solution. AFAICT, you only copy the full 
> structure here just for the benefits of updating state_entry_time.
> 
> I saw you discuss about it with Jan, so it would be nice to log at least 
> in the commit message the reason why this is done like that.

Isn't the reference to commit 2529c850ea48f036 enough? The update
protocol is clearly described in that commit message.


Juergen

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v2] xen/sched: don't let XEN_RUNSTATE_UPDATE leak into vcpu_runstate_get()
Posted by Julien Grall 6 years, 1 month ago 
Hi,

On 9/26/19 3:45 PM, Jürgen Groß wrote:
> On 26.09.19 16:27, Julien Grall wrote:
>> Hi,
>>
>> On 9/25/19 5:29 AM, 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").
>>>
>>> Reported-by: Andrew Cooper <andrew.cooper3@citrix.com>
>>> Signed-off-by: Juergen Gross <jgross@suse.com>
>>> ---
>>> V2: add handling on ARM, too (Jan Beulich)
>>> ---
>>>   xen/arch/arm/domain.c | 13 ++++++++-----
>>>   xen/arch/x86/domain.c | 17 ++++++++++-------
>>>   2 files changed, 18 insertions(+), 12 deletions(-)
>>>
>>> diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
>>> index ae13e47e86..d681ff5c6e 100644
>>> --- a/xen/arch/arm/domain.c
>>> +++ b/xen/arch/arm/domain.c
>>> @@ -280,28 +280,31 @@ static void ctxt_switch_to(struct vcpu *n)
>>>   static void update_runstate_area(struct vcpu *v)
>>>   {
>>>       void __user *guest_handle = NULL;
>>> +    struct vcpu_runstate_info runstate;
>>>       if ( guest_handle_is_null(runstate_guest(v)) )
>>>           return;
>>> +    memcpy(&runstate, &v->runstate, sizeof(runstate));
>>
>> I am not really happy with this solution. AFAICT, you only copy the 
>> full structure here just for the benefits of updating state_entry_time.
>>
>> I saw you discuss about it with Jan, so it would be nice to log at 
>> least in the commit message the reason why this is done like that.
> 
> Isn't the reference to commit 2529c850ea48f036 enough? The update
> protocol is clearly described in that commit message.

I meant the reason to use the 'memcpy', which sounds like quite a waste, 
compare to only copy state_entry_time.

Cheers,

-- 
Julien Grall

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

Patchew 2.1-devel-b67e4c2

© 2016 - 2025 Red Hat, Inc.