| Message ID | 56E2F9E102000078000DBAD6@prv-mh.provo.novell.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 11/03/16 16:01, Jan Beulich wrote: > In that case (with the new value being held in, or now in one case cast > to, a 32-bit variable) there's no need to go through the long mode part > of the checks. > > Primarily this was meant to hopefully address Coverity ID 1355278, but > since the change produces smaller code as well I think we should use it > even if it doesn't help that (benign) warning. > > Also it's more in line with jmp_rel() for commit_far_branch() to do the > _regs.eip update, so adjust that at once. > > Signed-off-by: Jan Beulich <jbeulich@suse.com> Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
>>> On 21.04.16 at 10:36, <andrew.cooper3@citrix.com> wrote: > On 11/03/16 16:01, Jan Beulich wrote: >> In that case (with the new value being held in, or now in one case cast >> to, a 32-bit variable) there's no need to go through the long mode part >> of the checks. >> >> Primarily this was meant to hopefully address Coverity ID 1355278, but >> since the change produces smaller code as well I think we should use it >> even if it doesn't help that (benign) warning. >> >> Also it's more in line with jmp_rel() for commit_far_branch() to do the >> _regs.eip update, so adjust that at once. >> >> Signed-off-by: Jan Beulich <jbeulich@suse.com> > > Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com> Wei, considering that this at least attempts to address a Coverity issue, I'd like to ask for this to be considered to be allowed in despite being past code freeze. Thanks, Jan
On Thu, Apr 21, 2016 at 02:55:33AM -0600, Jan Beulich wrote: > >>> On 21.04.16 at 10:36, <andrew.cooper3@citrix.com> wrote: > > On 11/03/16 16:01, Jan Beulich wrote: > >> In that case (with the new value being held in, or now in one case cast > >> to, a 32-bit variable) there's no need to go through the long mode part > >> of the checks. > >> > >> Primarily this was meant to hopefully address Coverity ID 1355278, but > >> since the change produces smaller code as well I think we should use it > >> even if it doesn't help that (benign) warning. > >> > >> Also it's more in line with jmp_rel() for commit_far_branch() to do the > >> _regs.eip update, so adjust that at once. > >> > >> Signed-off-by: Jan Beulich <jbeulich@suse.com> > > > > Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com> > > Wei, > > considering that this at least attempts to address a Coverity issue, > I'd like to ask for this to be considered to be allowed in despite > being past code freeze. > > Thanks, Jan > Release-acked-by: Wei Liu <wei.liu2@citrix.com>
--- a/xen/arch/x86/x86_emulate/x86_emulate.c +++ b/xen/arch/x86/x86_emulate/x86_emulate.c @@ -652,13 +652,20 @@ do { _regs.eip = ip; \ } while (0) -#define validate_far_branch(cs, ip) \ - generate_exception_if(in_longmode(ctxt, ops) && (cs)->attr.fields.l \ - ? !is_canonical_address(ip) \ - : (ip) > (cs)->limit, EXC_GP, 0) +#define validate_far_branch(cs, ip) ({ \ + if ( sizeof(ip) <= 4 ) { \ + ASSERT(in_longmode(ctxt, ops) <= 0); \ + generate_exception_if((ip) > (cs)->limit, EXC_GP, 0); \ + } else \ + generate_exception_if(in_longmode(ctxt, ops) && \ + (cs)->attr.fields.l \ + ? !is_canonical_address(ip) \ + : (ip) > (cs)->limit, EXC_GP, 0); \ +}) #define commit_far_branch(cs, ip) ({ \ validate_far_branch(cs, ip); \ + _regs.eip = (ip); \ ops->write_segment(x86_seg_cs, cs, ctxt); \ }) @@ -2787,7 +2794,6 @@ x86_emulate( (rc = load_seg(x86_seg_cs, src.val, 1, &cs, ctxt, ops)) || (rc = commit_far_branch(&cs, dst.val)) ) goto done; - _regs.eip = dst.val; break; } @@ -2830,9 +2836,8 @@ x86_emulate( eflags &= 0x257fd5; _regs.eflags &= mask; _regs.eflags |= (eflags & ~mask) | 0x02; - _regs.eip = eip; if ( (rc = load_seg(x86_seg_cs, sel, 1, &cs, ctxt, ops)) || - (rc = commit_far_branch(&cs, eip)) ) + (rc = commit_far_branch(&cs, (uint32_t)eip)) ) goto done; break; } @@ -3465,7 +3470,6 @@ x86_emulate( if ( (rc = load_seg(x86_seg_cs, sel, 0, &cs, ctxt, ops)) || (rc = commit_far_branch(&cs, eip)) ) goto done; - _regs.eip = eip; break; } @@ -3767,11 +3771,11 @@ x86_emulate( &_regs.eip, op_bytes, ctxt)) || (rc = ops->write_segment(x86_seg_cs, &cs, ctxt)) ) goto done; + _regs.eip = src.val; } else if ( (rc = load_seg(x86_seg_cs, sel, 0, &cs, ctxt, ops)) || (rc = commit_far_branch(&cs, src.val)) ) goto done; - _regs.eip = src.val; dst.type = OP_NONE; break;