KVM: X86: Fix the decoding of segment overrides in 64bit mode

Commit Message

Wanpeng Li March 22, 2018, 8:34 a.m. UTC

From: Wanpeng Li <[e-mail]>

Explicit segment overides other than %fs and %gs are documented as ignored by
both Intel and AMD.

In practice, this means that:

 * Explicit uses of %ss don't actually yield #SS[0] for non-canonical
   memory references.
 * Explicit uses of %{e,c,d}s don't override %rbp/%rsp-based memory references
   to yield #GP[0] for non-canonical memory references.

Cc: Paolo Bonzini <[e-mail]>
Cc: Radim Krčmář <[e-mail]>
Signed-off-by: Wanpeng Li <[e-mail]>
---
 arch/x86/kvm/emulate.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

Comments

Paolo Bonzini March 22, 2018, 10:07 a.m. UTC | #1

On 22/03/2018 09:34, Wanpeng Li wrote:
> From: Wanpeng Li <[e-mail]>
> 
> Explicit segment overides other than %fs and %gs are documented as ignored by
> both Intel and AMD.
> 
> In practice, this means that:
> 
>  * Explicit uses of %ss don't actually yield #SS[0] for non-canonical
>    memory references.
>  * Explicit uses of %{e,c,d}s don't override %rbp/%rsp-based memory references
>    to yield #GP[0] for non-canonical memory references.
> 
> Cc: Paolo Bonzini <[e-mail]>
> Cc: Radim Krčmář <[e-mail]>
> Signed-off-by: Wanpeng Li <[e-mail]>
> ---
>  arch/x86/kvm/emulate.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c
> index dd88158..5091255 100644
> --- a/arch/x86/kvm/emulate.c
> +++ b/arch/x86/kvm/emulate.c
> @@ -5148,8 +5148,10 @@ int x86_decode_insn(struct x86_emulate_ctxt *ctxt, void *insn, int insn_len)
>  		case 0x2e:	/* CS override */
>  		case 0x36:	/* SS override */
>  		case 0x3e:	/* DS override */
> -			has_seg_override = true;
> -			ctxt->seg_override = (ctxt->b >> 3) & 3;
> +			if (mode != X86EMUL_MODE_PROT64) {
> +				has_seg_override = true;
> +				ctxt->seg_override = (ctxt->b >> 3) & 3;
> +			}
>  			break;
>  		case 0x64:	/* FS override */
>  		case 0x65:	/* GS override */
> 

Testcase, please...

Paolo

Andrew Cooper March 22, 2018, 10:19 a.m. UTC | #2

On 22/03/2018 10:07, Paolo Bonzini wrote:
> On 22/03/2018 09:34, Wanpeng Li wrote:
>> From: Wanpeng Li <[e-mail]>
>>
>> Explicit segment overides other than %fs and %gs are documented as ignored by
>> both Intel and AMD.
>>
>> In practice, this means that:
>>
>>  * Explicit uses of %ss don't actually yield #SS[0] for non-canonical
>>    memory references.
>>  * Explicit uses of %{e,c,d}s don't override %rbp/%rsp-based memory references
>>    to yield #GP[0] for non-canonical memory references.
>>
>> Cc: Paolo Bonzini <[e-mail]>
>> Cc: Radim Krčmář <[e-mail]>
>> Signed-off-by: Wanpeng Li <[e-mail]>

When porting fixes from other projects, it is customary to identify so
in the commit message.  In this case, the fix you've ported is
http://xenbits.xen.org/gitweb/?p=xen.git;a=commitdiff;h=b7dce29d9faf3597d009c853ed1fcbed9f7a7f68

Here is an example of how Xen ports fixes from Linux for the drivers
that we share. 
http://xenbits.xen.org/gitweb/?p=xen.git;a=commitdiff;h=4e131596f1defec9407b6e60d584a696beaf5d7e


>> ---
>>  arch/x86/kvm/emulate.c | 6 ++++--
>>  1 file changed, 4 insertions(+), 2 deletions(-)
>>
>> diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c
>> index dd88158..5091255 100644
>> --- a/arch/x86/kvm/emulate.c
>> +++ b/arch/x86/kvm/emulate.c
>> @@ -5148,8 +5148,10 @@ int x86_decode_insn(struct x86_emulate_ctxt *ctxt, void *insn, int insn_len)
>>  		case 0x2e:	/* CS override */
>>  		case 0x36:	/* SS override */
>>  		case 0x3e:	/* DS override */
>> -			has_seg_override = true;
>> -			ctxt->seg_override = (ctxt->b >> 3) & 3;
>> +			if (mode != X86EMUL_MODE_PROT64) {
>> +				has_seg_override = true;
>> +				ctxt->seg_override = (ctxt->b >> 3) & 3;
>> +			}
>>  			break;
>>  		case 0x64:	/* FS override */
>>  		case 0x65:	/* GS override */
>>
> Testcase, please...

If you want to crib from one, this is the testcase I made for Xen.

http://xenbits.xen.org/docs/xtf/test-memop-seg.html

With the impending KVM/PVH work which is ongoing, it will soon be easy
to run Xen's HVM test suite unmodified under KVM, but we're not quite
there yet.

~Andrew

Paolo Bonzini March 22, 2018, 10:42 a.m. UTC | #3

On 22/03/2018 11:19, Andrew Cooper wrote:
> On 22/03/2018 10:07, Paolo Bonzini wrote:
>> On 22/03/2018 09:34, Wanpeng Li wrote:
>>> From: Wanpeng Li <[e-mail]>
>>>
>>> Explicit segment overides other than %fs and %gs are documented as ignored by
>>> both Intel and AMD.
>>>
>>> In practice, this means that:
>>>
>>>  * Explicit uses of %ss don't actually yield #SS[0] for non-canonical
>>>    memory references.
>>>  * Explicit uses of %{e,c,d}s don't override %rbp/%rsp-based memory references
>>>    to yield #GP[0] for non-canonical memory references.
>>>
>>> Cc: Paolo Bonzini <[e-mail]>
>>> Cc: Radim Krčmář <[e-mail]>
>>> Signed-off-by: Wanpeng Li <[e-mail]>
> 
> When porting fixes from other projects, it is customary to identify so
> in the commit message.  In this case, the fix you've ported is
> http://xenbits.xen.org/gitweb/?p=xen.git;a=commitdiff;h=b7dce29d9faf3597d009c853ed1fcbed9f7a7f68
> 
> Here is an example of how Xen ports fixes from Linux for the drivers
> that we share. 
> http://xenbits.xen.org/gitweb/?p=xen.git;a=commitdiff;h=4e131596f1defec9407b6e60d584a696beaf5d7e

Thanks Andrew.  The code is of course completely different, but the
commit message is 1:1.  Wanpeng, please acknowledge Jan in v2!

>> Testcase, please...
> 
> If you want to crib from one, this is the testcase I made for Xen.
> 
> http://xenbits.xen.org/docs/xtf/test-memop-seg.html

How does it ensure that the code is executed through the emulator and
not by the processor?

> With the impending KVM/PVH work which is ongoing, it will soon be easy
> to run Xen's HVM test suite unmodified under KVM, but we're not quite
> there yet.

What does the test suite use for console I/O?

Paolo

Andrew Cooper March 22, 2018, 11:04 a.m. UTC | #4

On 22/03/2018 10:42, Paolo Bonzini wrote:
> On 22/03/2018 11:19, Andrew Cooper wrote:
>> On 22/03/2018 10:07, Paolo Bonzini wrote:
>>> On 22/03/2018 09:34, Wanpeng Li wrote:
>>>> From: Wanpeng Li <[e-mail]>
>>>>
>>>> Explicit segment overides other than %fs and %gs are documented as ignored by
>>>> both Intel and AMD.
>>>>
>>>> In practice, this means that:
>>>>
>>>>  * Explicit uses of %ss don't actually yield #SS[0] for non-canonical
>>>>    memory references.
>>>>  * Explicit uses of %{e,c,d}s don't override %rbp/%rsp-based memory references
>>>>    to yield #GP[0] for non-canonical memory references.
>>>>
>>>> Cc: Paolo Bonzini <[e-mail]>
>>>> Cc: Radim Krčmář <[e-mail]>
>>>> Signed-off-by: Wanpeng Li <[e-mail]>
>> When porting fixes from other projects, it is customary to identify so
>> in the commit message.  In this case, the fix you've ported is
>> http://xenbits.xen.org/gitweb/?p=xen.git;a=commitdiff;h=b7dce29d9faf3597d009c853ed1fcbed9f7a7f68
>>
>> Here is an example of how Xen ports fixes from Linux for the drivers
>> that we share. 
>> http://xenbits.xen.org/gitweb/?p=xen.git;a=commitdiff;h=4e131596f1defec9407b6e60d584a696beaf5d7e
> Thanks Andrew.  The code is of course completely different, but the
> commit message is 1:1.  Wanpeng, please acknowledge Jan in v2!

Thanks, but it is actually my patch, which is why I was confused at
seeing my own commit message on LKML.

Also, the chances are that there are similar issues decoding the
instruction info field in the VMCS, which is how I stumbled onto this in
the first place.  I haven't yet fixed that side of things for Xen.

>
>>> Testcase, please...
>> If you want to crib from one, this is the testcase I made for Xen.
>>
>> http://xenbits.xen.org/docs/xtf/test-memop-seg.html
> How does it ensure that the code is executed through the emulator and
> not by the processor?

This test, and most of the tests in general, deliberately set things up
to execute the test cases first on the main processor, and then via the
emulator, and complain when any result is unexpected.

We've got a Force Emulation Prefix (ud2a; .ascii "xen") for doing
magic.  Originally, this was used for PV guests to explicitly request an
emulated CPUID, but I extended it to HVM guests for "emulate the next
instruction", after we had some guest user => guest kernel privilege
escalations because of incorrect emulation.

Fundamentally, any multi-vcpu guest can force an arbitrary instruction
through the emulator, because rewriting a couple of bytes of instruction
stream is far far far faster than a vmexit.  I chose to introduce a
explicit way to force this to occur, for testing purposes.

>
>> With the impending KVM/PVH work which is ongoing, it will soon be easy
>> to run Xen's HVM test suite unmodified under KVM, but we're not quite
>> there yet.
> What does the test suite use for console I/O?

Depends on what it available as it boots, but one of the default
consoles is port 0x12.  If things need to be tweaked to work more
cleanly, then that is entirely fine.

~Andrew

Wanpeng Li March 22, 2018, 11:14 a.m. UTC | #5

2018-03-22 19:04 GMT+08:00 Andrew Cooper <[e-mail]>:
> On 22/03/2018 10:42, Paolo Bonzini wrote:
>> On 22/03/2018 11:19, Andrew Cooper wrote:
>>> On 22/03/2018 10:07, Paolo Bonzini wrote:
>>>> On 22/03/2018 09:34, Wanpeng Li wrote:
>>>>> From: Wanpeng Li <[e-mail]>
>>>>>
>>>>> Explicit segment overides other than %fs and %gs are documented as ignored by
>>>>> both Intel and AMD.
>>>>>
>>>>> In practice, this means that:
>>>>>
>>>>>  * Explicit uses of %ss don't actually yield #SS[0] for non-canonical
>>>>>    memory references.
>>>>>  * Explicit uses of %{e,c,d}s don't override %rbp/%rsp-based memory references
>>>>>    to yield #GP[0] for non-canonical memory references.
>>>>>
>>>>> Cc: Paolo Bonzini <[e-mail]>
>>>>> Cc: Radim Krčmář <[e-mail]>
>>>>> Signed-off-by: Wanpeng Li <[e-mail]>
>>> When porting fixes from other projects, it is customary to identify so
>>> in the commit message.  In this case, the fix you've ported is
>>> http://xenbits.xen.org/gitweb/?p=xen.git;a=commitdiff;h=b7dce29d9faf3597d009c853ed1fcbed9f7a7f68
>>>
>>> Here is an example of how Xen ports fixes from Linux for the drivers
>>> that we share.
>>> http://xenbits.xen.org/gitweb/?p=xen.git;a=commitdiff;h=4e131596f1defec9407b6e60d584a696beaf5d7e
>> Thanks Andrew.  The code is of course completely different, but the
>> commit message is 1:1.  Wanpeng, please acknowledge Jan in v2!
>
> Thanks, but it is actually my patch, which is why I was confused at
> seeing my own commit message on LKML.

Thanks Andrew's original patch. Anyway, it is a really small stuff, I
will drop this patch to avoid the confusing.

Regards,
Wanpeng Li

>
> Also, the chances are that there are similar issues decoding the
> instruction info field in the VMCS, which is how I stumbled onto this in
> the first place.  I haven't yet fixed that side of things for Xen.
>
>>
>>>> Testcase, please...
>>> If you want to crib from one, this is the testcase I made for Xen.
>>>
>>> http://xenbits.xen.org/docs/xtf/test-memop-seg.html
>> How does it ensure that the code is executed through the emulator and
>> not by the processor?
>
> This test, and most of the tests in general, deliberately set things up
> to execute the test cases first on the main processor, and then via the
> emulator, and complain when any result is unexpected.
>
> We've got a Force Emulation Prefix (ud2a; .ascii "xen") for doing
> magic.  Originally, this was used for PV guests to explicitly request an
> emulated CPUID, but I extended it to HVM guests for "emulate the next
> instruction", after we had some guest user => guest kernel privilege
> escalations because of incorrect emulation.
>
> Fundamentally, any multi-vcpu guest can force an arbitrary instruction
> through the emulator, because rewriting a couple of bytes of instruction
> stream is far far far faster than a vmexit.  I chose to introduce a
> explicit way to force this to occur, for testing purposes.
>
>>
>>> With the impending KVM/PVH work which is ongoing, it will soon be easy
>>> to run Xen's HVM test suite unmodified under KVM, but we're not quite
>>> there yet.
>> What does the test suite use for console I/O?
>
> Depends on what it available as it boots, but one of the default
> consoles is port 0x12.  If things need to be tweaked to work more
> cleanly, then that is entirely fine.
>
> ~Andrew

Paolo Bonzini March 22, 2018, 12:38 p.m. UTC | #6

On 22/03/2018 12:04, Andrew Cooper wrote:
> We've got a Force Emulation Prefix (ud2a; .ascii "xen") for doing
> magic.  Originally, this was used for PV guests to explicitly request an
> emulated CPUID, but I extended it to HVM guests for "emulate the next
> instruction", after we had some guest user => guest kernel privilege
> escalations because of incorrect emulation.

Wanpeng, why don't you add it behind a new kvm module parameter? :)

Paolo

Wanpeng Li March 22, 2018, 1:39 p.m. UTC | #7

2018-03-22 20:38 GMT+08:00 Paolo Bonzini <[e-mail]>:
> On 22/03/2018 12:04, Andrew Cooper wrote:
>> We've got a Force Emulation Prefix (ud2a; .ascii "xen") for doing
>> magic.  Originally, this was used for PV guests to explicitly request an
>> emulated CPUID, but I extended it to HVM guests for "emulate the next
>> instruction", after we had some guest user => guest kernel privilege
>> escalations because of incorrect emulation.
>
> Wanpeng, why don't you add it behind a new kvm module parameter? :)

Great point! I will have a try. Thanks Paolo and Andrew. :)

Regards,
Wanpeng Li

Andrew Cooper March 22, 2018, 1:53 p.m. UTC | #8

On 22/03/18 13:39, Wanpeng Li wrote:
> 2018-03-22 20:38 GMT+08:00 Paolo Bonzini <[e-mail]>:
>> On 22/03/2018 12:04, Andrew Cooper wrote:
>>> We've got a Force Emulation Prefix (ud2a; .ascii "xen") for doing
>>> magic.  Originally, this was used for PV guests to explicitly request an
>>> emulated CPUID, but I extended it to HVM guests for "emulate the next
>>> instruction", after we had some guest user => guest kernel privilege
>>> escalations because of incorrect emulation.
>> Wanpeng, why don't you add it behind a new kvm module parameter? :)
> Great point! I will have a try. Thanks Paolo and Andrew. :)

Using the force emulation prefix requires intercepting #UD, which is in
general a BadThing(tm) for security.  Therefore, we have a build time
configuration option to compile in support, and require that test
systems explicitly opt into using it via a command line parameter.

http://xenbits.xen.org/gitweb/?p=xen.git;a=blob;f=xen/arch/x86/hvm/hvm.c;h=db52312882205e65b32e587106ca795f4bfab2eb;hb=refs/heads/staging#l3741
is the general #UD intercept handler if you want a reference.  (You can
ignore the cross-vendor part, which is leftovers from
http://developer.amd.com/wordpress/media/2012/10/CrossVendorMigration.pdf )

~Andrew

Wanpeng Li March 23, 2018, 2:27 p.m. UTC | #9

2018-03-22 21:53 GMT+08:00 Andrew Cooper <[e-mail]>:
> On 22/03/18 13:39, Wanpeng Li wrote:
>> 2018-03-22 20:38 GMT+08:00 Paolo Bonzini <[e-mail]>:
>>> On 22/03/2018 12:04, Andrew Cooper wrote:
>>>> We've got a Force Emulation Prefix (ud2a; .ascii "xen") for doing
>>>> magic.  Originally, this was used for PV guests to explicitly request an
>>>> emulated CPUID, but I extended it to HVM guests for "emulate the next
>>>> instruction", after we had some guest user => guest kernel privilege
>>>> escalations because of incorrect emulation.
>>> Wanpeng, why don't you add it behind a new kvm module parameter? :)
>> Great point! I will have a try. Thanks Paolo and Andrew. :)
>
> Using the force emulation prefix requires intercepting #UD, which is in
> general a BadThing(tm) for security.  Therefore, we have a build time

Yeah, however kvm intercepts and emulates #UD by default, should we
add a new kvm module parameter to enable it and disable by default?
Paolo.

> configuration option to compile in support, and require that test
> systems explicitly opt into using it via a command line parameter.
>
> http://xenbits.xen.org/gitweb/?p=xen.git;a=blob;f=xen/arch/x86/hvm/hvm.c;h=db52312882205e65b32e587106ca795f4bfab2eb;hb=refs/heads/staging#l3741
> is the general #UD intercept handler if you want a reference.  (You can

Thanks Andrew, it is useful. :) In addition, I didn't see the
test-memop-seg testcase has "Forced Emulation Prefix", when the prefix
is added to each instruction in the testcase?

Regards,
Wanpeng Li

Andrew Cooper March 23, 2018, 2:43 p.m. UTC | #10

On 23/03/18 14:27, Wanpeng Li wrote:
> 2018-03-22 21:53 GMT+08:00 Andrew Cooper <[e-mail]>:
>> On 22/03/18 13:39, Wanpeng Li wrote:
>>> 2018-03-22 20:38 GMT+08:00 Paolo Bonzini <[e-mail]>:
>>>> On 22/03/2018 12:04, Andrew Cooper wrote:
>>>>> We've got a Force Emulation Prefix (ud2a; .ascii "xen") for doing
>>>>> magic.  Originally, this was used for PV guests to explicitly request an
>>>>> emulated CPUID, but I extended it to HVM guests for "emulate the next
>>>>> instruction", after we had some guest user => guest kernel privilege
>>>>> escalations because of incorrect emulation.
>>>> Wanpeng, why don't you add it behind a new kvm module parameter? :)
>>> Great point! I will have a try. Thanks Paolo and Andrew. :)
>> Using the force emulation prefix requires intercepting #UD, which is in
>> general a BadThing(tm) for security.  Therefore, we have a build time
> Yeah, however kvm intercepts and emulates #UD by default, should we
> add a new kvm module parameter to enable it and disable by default?
> Paolo.
>
>> configuration option to compile in support, and require that test
>> systems explicitly opt into using it via a command line parameter.
>>
>> http://xenbits.xen.org/gitweb/?p=xen.git;a=blob;f=xen/arch/x86/hvm/hvm.c;h=db52312882205e65b32e587106ca795f4bfab2eb;hb=refs/heads/staging#l3741
>> is the general #UD intercept handler if you want a reference.  (You can
> Thanks Andrew, it is useful. :) In addition, I didn't see the
> test-memop-seg testcase has "Forced Emulation Prefix", when the prefix
> is added to each instruction in the testcase?

It has ended up substantially more ugly than I first intended, due to
several assembler bugs in older GCC and Clang toolchains.

http://xenbits.xen.org/gitweb/?p=xtf.git;a=blob;f=tests/memop-seg/asm.S;h=698661425bcdc9c181b235e323c2460e06c6e986;hb=HEAD#l35

I previously has FEP passed as a second parameter, but that becomes
prohibitively complicated to extract when testing %ss or %esp.  FEP is
now encoded in the bottom bit of the address passed in.

This was the cleanest way I could find of testing every combination, but
I'm open to improvements if anyone can spot any.

~Andrew

Paolo Bonzini March 23, 2018, 3:04 p.m. UTC | #11

On 23/03/2018 15:27, Wanpeng Li wrote:
> 2018-03-22 21:53 GMT+08:00 Andrew Cooper <[e-mail]>:
>> On 22/03/18 13:39, Wanpeng Li wrote:
>>> 2018-03-22 20:38 GMT+08:00 Paolo Bonzini <[e-mail]>:
>>>> On 22/03/2018 12:04, Andrew Cooper wrote:
>>>>> We've got a Force Emulation Prefix (ud2a; .ascii "xen") for doing
>>>>> magic.  Originally, this was used for PV guests to explicitly request an
>>>>> emulated CPUID, but I extended it to HVM guests for "emulate the next
>>>>> instruction", after we had some guest user => guest kernel privilege
>>>>> escalations because of incorrect emulation.
>>>> Wanpeng, why don't you add it behind a new kvm module parameter? :)
>>> Great point! I will have a try. Thanks Paolo and Andrew. :)
>>
>> Using the force emulation prefix requires intercepting #UD, which is in
>> general a BadThing(tm) for security.  Therefore, we have a build time
> 
> Yeah, however kvm intercepts and emulates #UD by default, should we
> add a new kvm module parameter to enable it and disable by default?

No, the module parameter should only be about the force-emulation prefix.

Paolo

Patch

diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c
index dd88158..5091255 100644
--- a/arch/x86/kvm/emulate.c
+++ b/arch/x86/kvm/emulate.c
@@ -5148,8 +5148,10 @@  int x86_decode_insn(struct x86_emulate_ctxt *ctxt, void *insn, int insn_len)
 		case 0x2e:	/* CS override */
 		case 0x36:	/* SS override */
 		case 0x3e:	/* DS override */
-			has_seg_override = true;
-			ctxt->seg_override = (ctxt->b >> 3) & 3;
+			if (mode != X86EMUL_MODE_PROT64) {
+				has_seg_override = true;
+				ctxt->seg_override = (ctxt->b >> 3) & 3;
+			}
 			break;
 		case 0x64:	/* FS override */
 		case 0x65:	/* GS override */