| Message ID | 57D987E6020000780010EE8F@prv-mh.provo.novell.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 14/09/16 16:24, Jan Beulich wrote: > No need to have the same logic twice. > > Signed-off-by: Jan Beulich <jbeulich@suse.com> > > --- a/xen/arch/x86/domain.c > +++ b/xen/arch/x86/domain.c > @@ -1745,22 +1745,22 @@ static void load_segments(struct vcpu *n > (unsigned long *)pv->kernel_sp; > unsigned long cs_and_mask, rflags; > > + /* Fold upcall mask and architectural IOPL into RFLAGS.IF. */ > + rflags = regs->rflags & ~(X86_EFLAGS_IF|X86_EFLAGS_IOPL); > + rflags |= !vcpu_info(n, evtchn_upcall_mask) << 9; > + if ( VM_ASSIST(n->domain, architectural_iopl) ) > + rflags |= n->arch.pv_vcpu.iopl; > + > if ( is_pv_32bit_vcpu(n) ) > { > unsigned int *esp = ring_1(regs) ? > (unsigned int *)regs->rsp : > (unsigned int *)pv->kernel_sp; > - unsigned int cs_and_mask, eflags; The unshadowed cs_and_mask is unsigned long, not int, which means the put_user() below will clobber a 32bit PV guests stack frame. Other than that, Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com> for the intended change. ~Andrew > int ret = 0; > > /* CS longword also contains full evtchn_upcall_mask. */ > cs_and_mask = (unsigned short)regs->cs | > ((unsigned int)vcpu_info(n, evtchn_upcall_mask) << 16); > - /* Fold upcall mask into RFLAGS.IF. */ > - eflags = regs->_eflags & ~(X86_EFLAGS_IF|X86_EFLAGS_IOPL); > - eflags |= !vcpu_info(n, evtchn_upcall_mask) << 9; > - if ( VM_ASSIST(n->domain, architectural_iopl) ) > - eflags |= n->arch.pv_vcpu.iopl; > > if ( !ring_1(regs) ) > { > @@ -1770,7 +1770,7 @@ static void load_segments(struct vcpu *n > } > > if ( ret | > - put_user(eflags, esp-1) | > + put_user(rflags, esp-1) | > put_user(cs_and_mask, esp-2) | > put_user(regs->_eip, esp-3) | > put_user(uregs->gs, esp-4) | > @@ -1805,12 +1805,6 @@ static void load_segments(struct vcpu *n > cs_and_mask = (unsigned long)regs->cs | > ((unsigned long)vcpu_info(n, evtchn_upcall_mask) << 32); > > - /* Fold upcall mask into RFLAGS.IF. */ > - rflags = regs->rflags & ~(X86_EFLAGS_IF|X86_EFLAGS_IOPL); > - rflags |= !vcpu_info(n, evtchn_upcall_mask) << 9; > - if ( VM_ASSIST(n->domain, architectural_iopl) ) > - rflags |= n->arch.pv_vcpu.iopl; > - > if ( put_user(regs->ss, rsp- 1) | > put_user(regs->rsp, rsp- 2) | > put_user(rflags, rsp- 3) | > > > > > > _______________________________________________ > Xen-devel mailing list > Xen-devel@lists.xen.org > https://lists.xen.org/xen-devel
>>> On 14.09.16 at 19:12, <andrew.cooper3@citrix.com> wrote: > On 14/09/16 16:24, Jan Beulich wrote: >> --- a/xen/arch/x86/domain.c >> +++ b/xen/arch/x86/domain.c >> @@ -1745,22 +1745,22 @@ static void load_segments(struct vcpu *n >> (unsigned long *)pv->kernel_sp; >> unsigned long cs_and_mask, rflags; >> >> + /* Fold upcall mask and architectural IOPL into RFLAGS.IF. */ >> + rflags = regs->rflags & ~(X86_EFLAGS_IF|X86_EFLAGS_IOPL); >> + rflags |= !vcpu_info(n, evtchn_upcall_mask) << 9; >> + if ( VM_ASSIST(n->domain, architectural_iopl) ) >> + rflags |= n->arch.pv_vcpu.iopl; >> + >> if ( is_pv_32bit_vcpu(n) ) >> { >> unsigned int *esp = ring_1(regs) ? >> (unsigned int *)regs->rsp : >> (unsigned int *)pv->kernel_sp; >> - unsigned int cs_and_mask, eflags; > > The unshadowed cs_and_mask is unsigned long, not int, which means the > put_user() below will clobber a 32bit PV guests stack frame. No, put_user() determines the access size from its second (pointer) argument. > Other than that, Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com> > for the intended change. Well, with the above (it not being clear what change you would have expected, should one be needed in the first place) I'll have to wait for clarification. Jan
On 15/09/2016 07:28, Jan Beulich wrote: >>>> On 14.09.16 at 19:12, <andrew.cooper3@citrix.com> wrote: >> On 14/09/16 16:24, Jan Beulich wrote: >>> --- a/xen/arch/x86/domain.c >>> +++ b/xen/arch/x86/domain.c >>> @@ -1745,22 +1745,22 @@ static void load_segments(struct vcpu *n >>> (unsigned long *)pv->kernel_sp; >>> unsigned long cs_and_mask, rflags; >>> >>> + /* Fold upcall mask and architectural IOPL into RFLAGS.IF. */ >>> + rflags = regs->rflags & ~(X86_EFLAGS_IF|X86_EFLAGS_IOPL); >>> + rflags |= !vcpu_info(n, evtchn_upcall_mask) << 9; >>> + if ( VM_ASSIST(n->domain, architectural_iopl) ) >>> + rflags |= n->arch.pv_vcpu.iopl; >>> + >>> if ( is_pv_32bit_vcpu(n) ) >>> { >>> unsigned int *esp = ring_1(regs) ? >>> (unsigned int *)regs->rsp : >>> (unsigned int *)pv->kernel_sp; >>> - unsigned int cs_and_mask, eflags; >> The unshadowed cs_and_mask is unsigned long, not int, which means the >> put_user() below will clobber a 32bit PV guests stack frame. > No, put_user() determines the access size from its second (pointer) > argument. Oh - so it does. Mind putting at least note to that effect in the commit message? My first thought upon seeing it was wondering whether you had a stale patch which didn't compile. With that, Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
--- a/xen/arch/x86/domain.c +++ b/xen/arch/x86/domain.c @@ -1745,22 +1745,22 @@ static void load_segments(struct vcpu *n (unsigned long *)pv->kernel_sp; unsigned long cs_and_mask, rflags; + /* Fold upcall mask and architectural IOPL into RFLAGS.IF. */ + rflags = regs->rflags & ~(X86_EFLAGS_IF|X86_EFLAGS_IOPL); + rflags |= !vcpu_info(n, evtchn_upcall_mask) << 9; + if ( VM_ASSIST(n->domain, architectural_iopl) ) + rflags |= n->arch.pv_vcpu.iopl; + if ( is_pv_32bit_vcpu(n) ) { unsigned int *esp = ring_1(regs) ? (unsigned int *)regs->rsp : (unsigned int *)pv->kernel_sp; - unsigned int cs_and_mask, eflags; int ret = 0; /* CS longword also contains full evtchn_upcall_mask. */ cs_and_mask = (unsigned short)regs->cs | ((unsigned int)vcpu_info(n, evtchn_upcall_mask) << 16); - /* Fold upcall mask into RFLAGS.IF. */ - eflags = regs->_eflags & ~(X86_EFLAGS_IF|X86_EFLAGS_IOPL); - eflags |= !vcpu_info(n, evtchn_upcall_mask) << 9; - if ( VM_ASSIST(n->domain, architectural_iopl) ) - eflags |= n->arch.pv_vcpu.iopl; if ( !ring_1(regs) ) { @@ -1770,7 +1770,7 @@ static void load_segments(struct vcpu *n } if ( ret | - put_user(eflags, esp-1) | + put_user(rflags, esp-1) | put_user(cs_and_mask, esp-2) | put_user(regs->_eip, esp-3) | put_user(uregs->gs, esp-4) | @@ -1805,12 +1805,6 @@ static void load_segments(struct vcpu *n cs_and_mask = (unsigned long)regs->cs | ((unsigned long)vcpu_info(n, evtchn_upcall_mask) << 32); - /* Fold upcall mask into RFLAGS.IF. */ - rflags = regs->rflags & ~(X86_EFLAGS_IF|X86_EFLAGS_IOPL); - rflags |= !vcpu_info(n, evtchn_upcall_mask) << 9; - if ( VM_ASSIST(n->domain, architectural_iopl) ) - rflags |= n->arch.pv_vcpu.iopl; - if ( put_user(regs->ss, rsp- 1) | put_user(regs->rsp, rsp- 2) | put_user(rflags, rsp- 3) |