* [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.