* [PATCH] x86_emulate: Always truncate %eip out of long mode @ 2015-12-10 20:03 Andrew Cooper 2015-12-11 10:47 ` Jan Beulich 2015-12-15 8:53 ` Jan Beulich 0 siblings, 2 replies; 7+ messages in thread From: Andrew Cooper @ 2015-12-10 20:03 UTC (permalink / raw) To: Xen-devel; +Cc: Andrew Cooper, Jan Beulich _regs.eip needs to be truncated after having size added to it, or bad situations can occur. e.g. emulating an instruction which crosses the 4GB boundary causes _regs.eip to become invalid (have some of the upper 32 bits set), and fail vmentry checks when returning back to the guest. The comment /* real hardware doesn't truncate */ seems to appear in c/s ddef8e16 "Tweak x86 emulator interface." without any justification. I have not been able to find any information to prove or disprove the claim, but emulating oneself into a vmentry failure is definitely the wrong thing to do. Trucate the instruction pointer at 32 or 16 bits, according to %cs.D. Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> --- CC: Jan Beulich <JBeulich@suse.com> v2: Use def_ad_bytes, to allow truncating to 16 bits for a 16bit code segment. --- xen/arch/x86/x86_emulate/x86_emulate.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/xen/arch/x86/x86_emulate/x86_emulate.c b/xen/arch/x86/x86_emulate/x86_emulate.c index f1454ce..03e7aab 100644 --- a/xen/arch/x86/x86_emulate/x86_emulate.c +++ b/xen/arch/x86/x86_emulate/x86_emulate.c @@ -570,8 +570,10 @@ do{ asm volatile ( \ /* Fetch next part of the instruction being emulated. */ #define insn_fetch_bytes(_size) \ ({ unsigned long _x = 0, _eip = _regs.eip; \ - if ( !mode_64bit() ) _eip = (uint32_t)_eip; /* ignore upper dword */ \ - _regs.eip += (_size); /* real hardware doesn't truncate */ \ + _regs.eip += (_size); \ + if ( !mode_64bit() ) { /* Truncate eip to def_ad_bytes (2 or 4). */ \ + _eip &= ~((1UL << (def_ad_bytes * 8)) - 1); \ + _regs.eip &= ~((1UL << (def_ad_bytes * 8)) - 1); }; \ generate_exception_if((uint8_t)(_regs.eip - \ ctxt->regs->eip) > MAX_INST_LEN, \ EXC_GP, 0); \ -- 2.1.4 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH] x86_emulate: Always truncate %eip out of long mode 2015-12-10 20:03 [PATCH] x86_emulate: Always truncate %eip out of long mode Andrew Cooper @ 2015-12-11 10:47 ` Jan Beulich 2015-12-11 11:12 ` Andrew Cooper 2015-12-15 8:53 ` Jan Beulich 1 sibling, 1 reply; 7+ messages in thread From: Jan Beulich @ 2015-12-11 10:47 UTC (permalink / raw) To: Aravind Gopalakrishnan, Suravee Suthikulpanit, Andrew Cooper, Donald D Dugger, Jun Nakajima, Kevin Tian Cc: Xen-devel >>> On 10.12.15 at 21:03, <andrew.cooper3@citrix.com> wrote: > --- a/xen/arch/x86/x86_emulate/x86_emulate.c > +++ b/xen/arch/x86/x86_emulate/x86_emulate.c > @@ -570,8 +570,10 @@ do{ asm volatile ( > \ > /* Fetch next part of the instruction being emulated. */ > #define insn_fetch_bytes(_size) \ > ({ unsigned long _x = 0, _eip = _regs.eip; \ > - if ( !mode_64bit() ) _eip = (uint32_t)_eip; /* ignore upper dword */ \ > - _regs.eip += (_size); /* real hardware doesn't truncate */ \ > + _regs.eip += (_size); \ > + if ( !mode_64bit() ) { /* Truncate eip to def_ad_bytes (2 or 4). */ \ > + _eip &= ~((1UL << (def_ad_bytes * 8)) - 1); \ Okay, what we use for actual fetching gets truncated. I'm fine with that. > + _regs.eip &= ~((1UL << (def_ad_bytes * 8)) - 1); }; \ But I don't think it's correct to truncate what eventually gets written back. If we're in doubt what real hardware does, and if the manuals are of no help, perhaps we should ask the hardware folks? AMD? Intel? Please clarify. Furthermore, doesn't this make the wrapping-inside-an-insn situation worse (i.e. what looks broken for 32- and 64-bit modes now gets broken also for 16-bit mode)? Jan ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] x86_emulate: Always truncate %eip out of long mode 2015-12-11 10:47 ` Jan Beulich @ 2015-12-11 11:12 ` Andrew Cooper 2015-12-11 13:28 ` Jan Beulich 0 siblings, 1 reply; 7+ messages in thread From: Andrew Cooper @ 2015-12-11 11:12 UTC (permalink / raw) To: Jan Beulich, Aravind Gopalakrishnan, Suravee Suthikulpanit, Donald D Dugger, Jun Nakajima, Kevin Tian Cc: Xen-devel On 11/12/15 10:47, Jan Beulich wrote: >>>> On 10.12.15 at 21:03, <andrew.cooper3@citrix.com> wrote: >> --- a/xen/arch/x86/x86_emulate/x86_emulate.c >> +++ b/xen/arch/x86/x86_emulate/x86_emulate.c >> @@ -570,8 +570,10 @@ do{ asm volatile ( >> \ >> /* Fetch next part of the instruction being emulated. */ >> #define insn_fetch_bytes(_size) \ >> ({ unsigned long _x = 0, _eip = _regs.eip; \ >> - if ( !mode_64bit() ) _eip = (uint32_t)_eip; /* ignore upper dword */ \ >> - _regs.eip += (_size); /* real hardware doesn't truncate */ \ >> + _regs.eip += (_size); \ >> + if ( !mode_64bit() ) { /* Truncate eip to def_ad_bytes (2 or 4). */ \ >> + _eip &= ~((1UL << (def_ad_bytes * 8)) - 1); \ > Okay, what we use for actual fetching gets truncated. I'm fine > with that. > >> + _regs.eip &= ~((1UL << (def_ad_bytes * 8)) - 1); }; \ > But I don't think it's correct to truncate what eventually gets > written back. The written-back value is the one which must be truncated, to avoid a vmentry failure in the 32bit case. The 16bit case is definitely less obvious. > If we're in doubt what real hardware does, and if > the manuals are of no help, perhaps we should ask the hardware > folks? In 16bit segments, the 32bit %eip register does exist, but the instruction pointer is mapped to the first 16 bits of it. The Intel manual warns: "The 8086 16-bit instruction pointer (IP) is mapped to the lower 16-bits of the EIP register. Note this register is a 32-bit register and unintentional address wrapping may occur." I can't locate anything in particular in the AMD manuals which talks specifically about 16bit semantics. > > AMD? Intel? Please clarify. > > Furthermore, doesn't this make the wrapping-inside-an-insn > situation worse (i.e. what looks broken for 32- and 64-bit modes > now gets broken also for 16-bit mode)? I don't understand which "broken" you are referring to here. ~Andrew ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] x86_emulate: Always truncate %eip out of long mode 2015-12-11 11:12 ` Andrew Cooper @ 2015-12-11 13:28 ` Jan Beulich 0 siblings, 0 replies; 7+ messages in thread From: Jan Beulich @ 2015-12-11 13:28 UTC (permalink / raw) To: Andrew Cooper Cc: Kevin Tian, Suravee Suthikulpanit, Donald D Dugger, Xen-devel, Aravind Gopalakrishnan, Jun Nakajima >>> On 11.12.15 at 12:12, <andrew.cooper3@citrix.com> wrote: > On 11/12/15 10:47, Jan Beulich wrote: >> Furthermore, doesn't this make the wrapping-inside-an-insn >> situation worse (i.e. what looks broken for 32- and 64-bit modes >> now gets broken also for 16-bit mode)? > > I don't understand which "broken" you are referring to here. The (u8) cast on the difference of the two eip values in the subsequent instruction check hides wraps, and hence an instruction crossing (not ending at) the 4G or 16E boundary already goes undetected without your change, but your change extends the issue to a 16-bit instruction crossing the 64k boundary. Jan ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] x86_emulate: Always truncate %eip out of long mode 2015-12-10 20:03 [PATCH] x86_emulate: Always truncate %eip out of long mode Andrew Cooper 2015-12-11 10:47 ` Jan Beulich @ 2015-12-15 8:53 ` Jan Beulich 2016-01-06 15:44 ` Jan Beulich 1 sibling, 1 reply; 7+ messages in thread From: Jan Beulich @ 2015-12-15 8:53 UTC (permalink / raw) To: Andrew Cooper; +Cc: Xen-devel [-- Attachment #1: Type: text/plain, Size: 2966 bytes --] >>> On 10.12.15 at 21:03, <andrew.cooper3@citrix.com> wrote: > --- a/xen/arch/x86/x86_emulate/x86_emulate.c > +++ b/xen/arch/x86/x86_emulate/x86_emulate.c > @@ -570,8 +570,10 @@ do{ asm volatile ( > \ > /* Fetch next part of the instruction being emulated. */ > #define insn_fetch_bytes(_size) \ > ({ unsigned long _x = 0, _eip = _regs.eip; \ > - if ( !mode_64bit() ) _eip = (uint32_t)_eip; /* ignore upper dword */ \ > - _regs.eip += (_size); /* real hardware doesn't truncate */ \ > + _regs.eip += (_size); \ > + if ( !mode_64bit() ) { /* Truncate eip to def_ad_bytes (2 or 4). */ \ > + _eip &= ~((1UL << (def_ad_bytes * 8)) - 1); \ > + _regs.eip &= ~((1UL << (def_ad_bytes * 8)) - 1); }; \ Did you test this? The ~ seems quite wrong, and this would also cause undefined behavior in 32-bit compilation of the emulator test... Anyway, how about the following replacement patch, along the lines of what I think I had described in reply to the first variant of the patch? Jan x86emul: fix rIP handling Deal with rIP just like with any other register: Truncate to designated width upon entry, and write back the zero-extended 32-bit value when emulating 32-bit code, and leave the upper 48 bits unchanged for 16-bit code. Signed-off-by: Jan Beulich <jbeulich@suse.com> --- a/xen/arch/x86/x86_emulate/x86_emulate.c +++ b/xen/arch/x86/x86_emulate/x86_emulate.c @@ -570,7 +570,6 @@ do{ asm volatile ( /* Fetch next part of the instruction being emulated. */ #define insn_fetch_bytes(_size) \ ({ unsigned long _x = 0, _eip = _regs.eip; \ - if ( !mode_64bit() ) _eip = (uint32_t)_eip; /* ignore upper dword */ \ _regs.eip += (_size); /* real hardware doesn't truncate */ \ generate_exception_if((uint8_t)(_regs.eip - \ ctxt->regs->eip) > MAX_INST_LEN, \ @@ -1481,6 +1480,10 @@ x86_emulate( #endif } + /* Truncate rIP to def_ad_bytes (2 or 4) if necessary. */ + if ( def_ad_bytes < sizeof(_regs.eip) ) + _regs.eip &= (1UL << (def_ad_bytes * 8)) - 1; + /* Prefix bytes. */ for ( ; ; ) { @@ -3793,6 +3796,21 @@ x86_emulate( /* Commit shadow register state. */ _regs.eflags &= ~EFLG_RF; + switch ( __builtin_expect(def_ad_bytes, sizeof(_regs.eip)) ) + { + uint16_t ip; + + case 2: + ip = _regs.eip; + _regs.eip = ctxt->regs->eip; + *(uint16_t *)&_regs.eip = ip; + break; +#ifdef __x86_64__ + case 4: + _regs.rip = _regs._eip; + break; +#endif + } *ctxt->regs = _regs; done: [-- Attachment #2: x86emul-truncate-IP.patch --] [-- Type: text/plain, Size: 1721 bytes --] x86emul: fix rIP handling Deal with rIP just like with any other register: Truncate to designated width upon entry, and write back the zero-extended 32-bit value when emulating 32-bit code, and leave the upper 48 bits unchanged for 16-bit code. Signed-off-by: Jan Beulich <jbeulich@suse.com> --- a/xen/arch/x86/x86_emulate/x86_emulate.c +++ b/xen/arch/x86/x86_emulate/x86_emulate.c @@ -570,7 +570,6 @@ do{ asm volatile ( /* Fetch next part of the instruction being emulated. */ #define insn_fetch_bytes(_size) \ ({ unsigned long _x = 0, _eip = _regs.eip; \ - if ( !mode_64bit() ) _eip = (uint32_t)_eip; /* ignore upper dword */ \ _regs.eip += (_size); /* real hardware doesn't truncate */ \ generate_exception_if((uint8_t)(_regs.eip - \ ctxt->regs->eip) > MAX_INST_LEN, \ @@ -1481,6 +1480,10 @@ x86_emulate( #endif } + /* Truncate rIP to def_ad_bytes (2 or 4) if necessary. */ + if ( def_ad_bytes < sizeof(_regs.eip) ) + _regs.eip &= (1UL << (def_ad_bytes * 8)) - 1; + /* Prefix bytes. */ for ( ; ; ) { @@ -3793,6 +3796,21 @@ x86_emulate( /* Commit shadow register state. */ _regs.eflags &= ~EFLG_RF; + switch ( __builtin_expect(def_ad_bytes, sizeof(_regs.eip)) ) + { + uint16_t ip; + + case 2: + ip = _regs.eip; + _regs.eip = ctxt->regs->eip; + *(uint16_t *)&_regs.eip = ip; + break; +#ifdef __x86_64__ + case 4: + _regs.rip = _regs._eip; + break; +#endif + } *ctxt->regs = _regs; done: [-- Attachment #3: Type: text/plain, Size: 126 bytes --] _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] x86_emulate: Always truncate %eip out of long mode 2015-12-15 8:53 ` Jan Beulich @ 2016-01-06 15:44 ` Jan Beulich 2016-01-06 16:09 ` Andrew Cooper 0 siblings, 1 reply; 7+ messages in thread From: Jan Beulich @ 2016-01-06 15:44 UTC (permalink / raw) To: Andrew Cooper; +Cc: Xen-devel Ping? >>> On 15.12.15 at 09:53, <JBeulich@suse.com> wrote: >>>> On 10.12.15 at 21:03, <andrew.cooper3@citrix.com> wrote: >> --- a/xen/arch/x86/x86_emulate/x86_emulate.c >> +++ b/xen/arch/x86/x86_emulate/x86_emulate.c >> @@ -570,8 +570,10 @@ do{ asm volatile ( > >> \ >> /* Fetch next part of the instruction being emulated. */ >> #define insn_fetch_bytes(_size) \ >> ({ unsigned long _x = 0, _eip = _regs.eip; \ >> - if ( !mode_64bit() ) _eip = (uint32_t)_eip; /* ignore upper dword */ \ >> - _regs.eip += (_size); /* real hardware doesn't truncate */ \ >> + _regs.eip += (_size); \ >> + if ( !mode_64bit() ) { /* Truncate eip to def_ad_bytes (2 or 4). */ \ >> + _eip &= ~((1UL << (def_ad_bytes * 8)) - 1); \ >> + _regs.eip &= ~((1UL << (def_ad_bytes * 8)) - 1); }; \ > > Did you test this? The ~ seems quite wrong, and this would also > cause undefined behavior in 32-bit compilation of the emulator > test... Anyway, how about the following replacement patch, > along the lines of what I think I had described in reply to the first > variant of the patch? > > Jan > > x86emul: fix rIP handling > > Deal with rIP just like with any other register: Truncate to designated > width upon entry, and write back the zero-extended 32-bit value when > emulating 32-bit code, and leave the upper 48 bits unchanged for 16-bit > code. > > Signed-off-by: Jan Beulich <jbeulich@suse.com> > > --- a/xen/arch/x86/x86_emulate/x86_emulate.c > +++ b/xen/arch/x86/x86_emulate/x86_emulate.c > @@ -570,7 +570,6 @@ do{ asm volatile ( > /* Fetch next part of the instruction being emulated. */ > #define insn_fetch_bytes(_size) \ > ({ unsigned long _x = 0, _eip = _regs.eip; \ > - if ( !mode_64bit() ) _eip = (uint32_t)_eip; /* ignore upper dword */ \ > _regs.eip += (_size); /* real hardware doesn't truncate */ \ > generate_exception_if((uint8_t)(_regs.eip - \ > ctxt->regs->eip) > MAX_INST_LEN, \ > @@ -1481,6 +1480,10 @@ x86_emulate( > #endif > } > > + /* Truncate rIP to def_ad_bytes (2 or 4) if necessary. */ > + if ( def_ad_bytes < sizeof(_regs.eip) ) > + _regs.eip &= (1UL << (def_ad_bytes * 8)) - 1; > + > /* Prefix bytes. */ > for ( ; ; ) > { > @@ -3793,6 +3796,21 @@ x86_emulate( > > /* Commit shadow register state. */ > _regs.eflags &= ~EFLG_RF; > + switch ( __builtin_expect(def_ad_bytes, sizeof(_regs.eip)) ) > + { > + uint16_t ip; > + > + case 2: > + ip = _regs.eip; > + _regs.eip = ctxt->regs->eip; > + *(uint16_t *)&_regs.eip = ip; > + break; > +#ifdef __x86_64__ > + case 4: > + _regs.rip = _regs._eip; > + break; > +#endif > + } > *ctxt->regs = _regs; > > done: ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] x86_emulate: Always truncate %eip out of long mode 2016-01-06 15:44 ` Jan Beulich @ 2016-01-06 16:09 ` Andrew Cooper 0 siblings, 0 replies; 7+ messages in thread From: Andrew Cooper @ 2016-01-06 16:09 UTC (permalink / raw) To: Jan Beulich; +Cc: Xen-devel On 06/01/16 15:44, Jan Beulich wrote: > Ping? Sorry - this is still on my todo list, but I have more urgent work currently. ~Andrew > >>>> On 15.12.15 at 09:53, <JBeulich@suse.com> wrote: >>>>> On 10.12.15 at 21:03, <andrew.cooper3@citrix.com> wrote: >>> --- a/xen/arch/x86/x86_emulate/x86_emulate.c >>> +++ b/xen/arch/x86/x86_emulate/x86_emulate.c >>> @@ -570,8 +570,10 @@ do{ asm volatile ( >>> \ >>> /* Fetch next part of the instruction being emulated. */ >>> #define insn_fetch_bytes(_size) \ >>> ({ unsigned long _x = 0, _eip = _regs.eip; \ >>> - if ( !mode_64bit() ) _eip = (uint32_t)_eip; /* ignore upper dword */ \ >>> - _regs.eip += (_size); /* real hardware doesn't truncate */ \ >>> + _regs.eip += (_size); \ >>> + if ( !mode_64bit() ) { /* Truncate eip to def_ad_bytes (2 or 4). */ \ >>> + _eip &= ~((1UL << (def_ad_bytes * 8)) - 1); \ >>> + _regs.eip &= ~((1UL << (def_ad_bytes * 8)) - 1); }; \ >> Did you test this? The ~ seems quite wrong, and this would also >> cause undefined behavior in 32-bit compilation of the emulator >> test... Anyway, how about the following replacement patch, >> along the lines of what I think I had described in reply to the first >> variant of the patch? >> >> Jan >> >> x86emul: fix rIP handling >> >> Deal with rIP just like with any other register: Truncate to designated >> width upon entry, and write back the zero-extended 32-bit value when >> emulating 32-bit code, and leave the upper 48 bits unchanged for 16-bit >> code. >> >> Signed-off-by: Jan Beulich <jbeulich@suse.com> >> >> --- a/xen/arch/x86/x86_emulate/x86_emulate.c >> +++ b/xen/arch/x86/x86_emulate/x86_emulate.c >> @@ -570,7 +570,6 @@ do{ asm volatile ( >> /* Fetch next part of the instruction being emulated. */ >> #define insn_fetch_bytes(_size) \ >> ({ unsigned long _x = 0, _eip = _regs.eip; \ >> - if ( !mode_64bit() ) _eip = (uint32_t)_eip; /* ignore upper dword */ \ >> _regs.eip += (_size); /* real hardware doesn't truncate */ \ >> generate_exception_if((uint8_t)(_regs.eip - \ >> ctxt->regs->eip) > MAX_INST_LEN, \ >> @@ -1481,6 +1480,10 @@ x86_emulate( >> #endif >> } >> >> + /* Truncate rIP to def_ad_bytes (2 or 4) if necessary. */ >> + if ( def_ad_bytes < sizeof(_regs.eip) ) >> + _regs.eip &= (1UL << (def_ad_bytes * 8)) - 1; >> + >> /* Prefix bytes. */ >> for ( ; ; ) >> { >> @@ -3793,6 +3796,21 @@ x86_emulate( >> >> /* Commit shadow register state. */ >> _regs.eflags &= ~EFLG_RF; >> + switch ( __builtin_expect(def_ad_bytes, sizeof(_regs.eip)) ) >> + { >> + uint16_t ip; >> + >> + case 2: >> + ip = _regs.eip; >> + _regs.eip = ctxt->regs->eip; >> + *(uint16_t *)&_regs.eip = ip; >> + break; >> +#ifdef __x86_64__ >> + case 4: >> + _regs.rip = _regs._eip; >> + break; >> +#endif >> + } >> *ctxt->regs = _regs; >> >> done: > > ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2016-01-06 16:09 UTC | newest] Thread overview: 7+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2015-12-10 20:03 [PATCH] x86_emulate: Always truncate %eip out of long mode Andrew Cooper 2015-12-11 10:47 ` Jan Beulich 2015-12-11 11:12 ` Andrew Cooper 2015-12-11 13:28 ` Jan Beulich 2015-12-15 8:53 ` Jan Beulich 2016-01-06 15:44 ` Jan Beulich 2016-01-06 16:09 ` Andrew Cooper
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.