| Message ID | 20190925042917.11392-1-jgross@suse.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v2] xen/sched: don't let XEN_RUNSTATE_UPDATE leak into vcpu_runstate_get() | expand |
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>
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,
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
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,
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);
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(-)