[PATCH 0/3] KVM: x86/emulator: Segment load fixes

[PATCH 0/3] KVM: x86/emulator: Segment load fixes

[Michal Luczaj]

Two small fixes for __load_segment_descriptor(), along with a KUT
x86/emulator test.

And a question to maintainers: is it ok to send patches for two repos in
one series?

Michal Luczaj (3):
  KVM: x86/emulator: Fix segment load privilege level validation
  KVM: x86/emulator: Fix comment in __load_segment_descriptor()
  x86: Test CPL=3 DS/ES/FS/GS RPL=DPL=0 segment descriptor load

-- 
2.39.0

[PATCH 1/3] KVM: x86/emulator: Fix segment load privilege level validation

[Michal Luczaj]

Intel SDM describes what steps are taken by the CPU to verify if a
memory segment can actually be used at a given privilege level. Loading
DS/ES/FS/GS involves checking segment's type as well as making sure that
neither selector's RPL nor caller's CPL are greater than segment's DPL.

Emulator implements Intel's pseudocode in __load_segment_descriptor(),
even quoting the pseudocode in the comments. Although the pseudocode is
correctly translated, the implementation is incorrect. This is most
likely due to SDM, at the time, being wrong.

Patch fixes emulator's logic and updates the pseudocode in the comment.
Below are historical notes.

Emulator code for handling segment descriptors appears to have been
introduced in March 2010 in commit 38ba30ba51a0 ("KVM: x86 emulator:
Emulate task switch in emulator.c"). Intel SDM Vol 2A: Instruction Set
Reference, A-M (Order Number: 253666-034US, _March 2010_) lists the
steps for loading segment registers in section related to MOV
instruction:

  IF DS, ES, FS, or GS is loaded with non-NULL selector
  THEN
    IF segment selector index is outside descriptor table limits
    or segment is not a data or readable code segment
    or ((segment is a data or nonconforming code segment)
    and (both RPL and CPL > DPL))   <---
      THEN #GP(selector); FI;

This is precisely what __load_segment_descriptor() quotes and
implements. But there's a twist; a few SDM revisions later
(253667-044US), in August 2012, the snippet above becomes:

  IF DS, ES, FS, or GS is loaded with non-NULL selector
  THEN
    IF segment selector index is outside descriptor table limits
    or segment is not a data or readable code segment
    or ((segment is a data or nonconforming code segment)
      [note: missing or superfluous parenthesis?]
    or ((RPL > DPL) and (CPL > DPL))   <---
      THEN #GP(selector); FI;

Many SDMs later (253667-065US), in December 2017, pseudocode reaches
what seems to be its final form:

  IF DS, ES, FS, or GS is loaded with non-NULL selector
  THEN
    IF segment selector index is outside descriptor table limits
    OR segment is not a data or readable code segment
    OR ((segment is a data or nonconforming code segment)
        AND ((RPL > DPL) or (CPL > DPL)))   <---
      THEN #GP(selector); FI;

Signed-off-by: Michal Luczaj <mhal@rbox.co>
---
 arch/x86/kvm/emulate.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c
index 5cc3efa0e21c..81b8f5dcfa44 100644
--- a/arch/x86/kvm/emulate.c
+++ b/arch/x86/kvm/emulate.c
@@ -1695,11 +1695,11 @@ static int __load_segment_descriptor(struct x86_emulate_ctxt *ctxt,
 		/*
 		 * segment is not a data or readable code segment or
 		 * ((segment is a data or nonconforming code segment)
-		 * and (both RPL and CPL > DPL))
+		 * and ((RPL > DPL) or (CPL > DPL)))
 		 */
 		if ((seg_desc.type & 0xa) == 0x8 ||
 		    (((seg_desc.type & 0xc) != 0xc) &&
-		     (rpl > dpl && cpl > dpl)))
+		     (rpl > dpl || cpl > dpl)))
 			goto exception;
 		break;
 	}
-- 
2.39.0

[PATCH 2/3] KVM: x86/emulator: Fix comment in __load_segment_descriptor()

[Michal Luczaj]

The comment refers to the same condition twice. Make it reflect what the
code actually does.

No functional change intended.

Signed-off-by: Michal Luczaj <mhal@rbox.co>
---
 arch/x86/kvm/emulate.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c
index 81b8f5dcfa44..91581bfeba22 100644
--- a/arch/x86/kvm/emulate.c
+++ b/arch/x86/kvm/emulate.c
@@ -1633,7 +1633,7 @@ static int __load_segment_descriptor(struct x86_emulate_ctxt *ctxt,
 	case VCPU_SREG_SS:
 		/*
 		 * segment is not a writable data segment or segment
-		 * selector's RPL != CPL or segment selector's RPL != CPL
+		 * selector's RPL != CPL or DPL != CPL
 		 */
 		if (rpl != cpl || (seg_desc.type & 0xa) != 0x2 || dpl != cpl)
 			goto exception;
-- 
2.39.0

[kvm-unit-tests PATCH 3/3] x86: Test CPL=3 DS/ES/FS/GS RPL=DPL=0 segment descriptor load

[Michal Luczaj]

User space loading of DS, ES, FS, or GS is forbidden for a DPL=0
segment descriptor (conforming code segment being an exception).
Verify that #GP is raised if

	((segment is a data or nonconforming code segment)
	 AND ((RPL > DPL) or (CPL > DPL)))

Signed-off-by: Michal Luczaj <mhal@rbox.co>
---
Test is implemented only for x86_64 as it's making use of
run_in_user(). Should it be reimplemented for _32 as well?

 x86/emulator64.c | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/x86/emulator64.c b/x86/emulator64.c
index 7f55d38..c58441c 100644
--- a/x86/emulator64.c
+++ b/x86/emulator64.c
@@ -355,6 +355,21 @@ static void test_movabs(uint64_t *mem)
 	report(rcx == 0x9090909090909090, "64-bit mov imm2");
 }
 
+static void load_dpl0_seg(void)
+{
+	asm volatile(KVM_FEP "mov %0, %%fs" :: "r" (KERNEL_CS)); /* RPL=0 */
+}
+
+static void test_user_load_dpl0_seg(void)
+{
+	bool raised_vector;
+
+	run_in_user((usermode_func)load_dpl0_seg, GP_VECTOR, 0, 0, 0, 0,
+		    &raised_vector);
+
+	report(raised_vector, "Wanted #GP on CPL=3 DPL=0 segment load");
+}
+
 static void test_push16(uint64_t *mem)
 {
 	uint64_t rsp1, rsp2;
@@ -456,6 +471,7 @@ static void test_emulator_64(void *mem)
 	if (is_fep_available()) {
 		test_mmx_movq_mf(mem);
 		test_movabs(mem);
+		test_user_load_dpl0_seg();
 	}
 
 	test_push16(mem);
-- 
2.39.0

Re: [PATCH 0/3] KVM: x86/emulator: Segment load fixes

[Sean Christopherson]

On Thu, Jan 26, 2023, Michal Luczaj wrote:
> Two small fixes for __load_segment_descriptor(), along with a KUT
> x86/emulator test.
> 
> And a question to maintainers: is it ok to send patches for two repos in
> one series?

No, in the future please send two separate series (no need to do so for this case).
Us humans can easily figure out what's going on, but b4[*] gets confused, e.g.
`b4 am` will fail.

What I usually do to connect the KVM-unit-test change to the kernel/KVM change is
to post the kernel patches first, and then update the KVM-unit-test patch(es) to
provide the lore link to the kernel patches.

Thanks for asking, and a _huge_ thanks for writing tests!

[*] git://git.kernel.org/pub/scm/utils/b4/b4.git

Re: [PATCH 1/3] KVM: x86/emulator: Fix segment load privilege level validation

[Paolo Bonzini]

On 1/26/23 02:34, Michal Luczaj wrote:
> Intel SDM describes what steps are taken by the CPU to verify if a
> memory segment can actually be used at a given privilege level. Loading
> DS/ES/FS/GS involves checking segment's type as well as making sure that
> neither selector's RPL nor caller's CPL are greater than segment's DPL.
> 
> Emulator implements Intel's pseudocode in __load_segment_descriptor(),
> even quoting the pseudocode in the comments. Although the pseudocode is
> correctly translated, the implementation is incorrect. This is most
> likely due to SDM, at the time, being wrong.
> 
> Patch fixes emulator's logic and updates the pseudocode in the comment.
> Below are historical notes.
> 
> Emulator code for handling segment descriptors appears to have been
> introduced in March 2010 in commit 38ba30ba51a0 ("KVM: x86 emulator:
> Emulate task switch in emulator.c"). Intel SDM Vol 2A: Instruction Set
> Reference, A-M (Order Number: 253666-034US, _March 2010_) lists the
> steps for loading segment registers in section related to MOV
> instruction:
> 
>    IF DS, ES, FS, or GS is loaded with non-NULL selector
>    THEN
>      IF segment selector index is outside descriptor table limits
>      or segment is not a data or readable code segment
>      or ((segment is a data or nonconforming code segment)
>      and (both RPL and CPL > DPL))   <---
>        THEN #GP(selector); FI;
> 
> This is precisely what __load_segment_descriptor() quotes and
> implements. But there's a twist; a few SDM revisions later
> (253667-044US), in August 2012, the snippet above becomes:
> 
>    IF DS, ES, FS, or GS is loaded with non-NULL selector
>    THEN
>      IF segment selector index is outside descriptor table limits
>      or segment is not a data or readable code segment
>      or ((segment is a data or nonconforming code segment)
>        [note: missing or superfluous parenthesis?]
>      or ((RPL > DPL) and (CPL > DPL))   <---
>        THEN #GP(selector); FI;
> 
> Many SDMs later (253667-065US), in December 2017, pseudocode reaches
> what seems to be its final form:
> 
>    IF DS, ES, FS, or GS is loaded with non-NULL selector
>    THEN
>      IF segment selector index is outside descriptor table limits
>      OR segment is not a data or readable code segment
>      OR ((segment is a data or nonconforming code segment)
>          AND ((RPL > DPL) or (CPL > DPL)))   <---
>        THEN #GP(selector); FI;
> 
> Signed-off-by: Michal Luczaj <mhal@rbox.co>
> ---
>   arch/x86/kvm/emulate.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c
> index 5cc3efa0e21c..81b8f5dcfa44 100644
> --- a/arch/x86/kvm/emulate.c
> +++ b/arch/x86/kvm/emulate.c
> @@ -1695,11 +1695,11 @@ static int __load_segment_descriptor(struct x86_emulate_ctxt *ctxt,
>   		/*
>   		 * segment is not a data or readable code segment or
>   		 * ((segment is a data or nonconforming code segment)
> -		 * and (both RPL and CPL > DPL))
> +		 * and ((RPL > DPL) or (CPL > DPL)))
>   		 */
>   		if ((seg_desc.type & 0xa) == 0x8 ||
>   		    (((seg_desc.type & 0xc) != 0xc) &&
> -		     (rpl > dpl && cpl > dpl)))
> +		     (rpl > dpl || cpl > dpl)))
>   			goto exception;
>   		break;
>   	}

Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>

Re: [PATCH 2/3] KVM: x86/emulator: Fix comment in __load_segment_descriptor()

[Paolo Bonzini]

On 1/26/23 02:34, Michal Luczaj wrote:
> The comment refers to the same condition twice. Make it reflect what the
> code actually does.
> 
> No functional change intended.
> 
> Signed-off-by: Michal Luczaj <mhal@rbox.co>
> ---
>   arch/x86/kvm/emulate.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c
> index 81b8f5dcfa44..91581bfeba22 100644
> --- a/arch/x86/kvm/emulate.c
> +++ b/arch/x86/kvm/emulate.c
> @@ -1633,7 +1633,7 @@ static int __load_segment_descriptor(struct x86_emulate_ctxt *ctxt,
>   	case VCPU_SREG_SS:
>   		/*
>   		 * segment is not a writable data segment or segment
> -		 * selector's RPL != CPL or segment selector's RPL != CPL
> +		 * selector's RPL != CPL or DPL != CPL
>   		 */
>   		if (rpl != cpl || (seg_desc.type & 0xa) != 0x2 || dpl != cpl)
>   			goto exception;

Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>

Re: [PATCH 0/3] KVM: x86/emulator: Segment load fixes

[Michal Luczaj]

On 1/26/23 18:33, Sean Christopherson wrote:
> On Thu, Jan 26, 2023, Michal Luczaj wrote:
>> Two small fixes for __load_segment_descriptor(), along with a KUT
>> x86/emulator test.
>>
>> And a question to maintainers: is it ok to send patches for two repos in
>> one series?
> 
> No, in the future please send two separate series (no need to do so for this case).
> Us humans can easily figure out what's going on, but b4[*] gets confused, e.g.
> `b4 am` will fail.
> 
> What I usually do to connect the KVM-unit-test change to the kernel/KVM change is
> to post the kernel patches first, and then update the KVM-unit-test patch(es) to
> provide the lore link to the kernel patches.

All right, thank you for explaining.

Michal

Re: [PATCH 0/3] KVM: x86/emulator: Segment load fixes

[Sean Christopherson]

On Thu, 26 Jan 2023 02:34:02 +0100, Michal Luczaj wrote:
> Two small fixes for __load_segment_descriptor(), along with a KUT
> x86/emulator test.
> 
> And a question to maintainers: is it ok to send patches for two repos in
> one series?
> 
> Michal Luczaj (3):
> 
> [...]

Applied to kvm-x86 misc, thanks!

[1/3] KVM: x86/emulator: Fix segment load privilege level validation
      https://github.com/kvm-x86/linux/commit/0735d1c34e49
[2/3] KVM: x86/emulator: Fix comment in __load_segment_descriptor()
      https://github.com/kvm-x86/linux/commit/096691e0d2a1

--
https://github.com/kvm-x86/linux/tree/next
https://github.com/kvm-x86/linux/tree/fixes

Re: [PATCH 0/3] KVM: x86/emulator: Segment load fixes

[Michal Luczaj]

On 1/26/23 18:33, Sean Christopherson wrote:
> On Thu, Jan 26, 2023, Michal Luczaj wrote:
>> Two small fixes for __load_segment_descriptor(), along with a KUT
>> x86/emulator test.
>>
>> And a question to maintainers: is it ok to send patches for two repos in
>> one series?
> 
> No, in the future please send two separate series (no need to do so for this case).
> Us humans can easily figure out what's going on, but b4[*] gets confused, e.g.
> `b4 am` will fail.
> 
> What I usually do to connect the KVM-unit-test change to the kernel/KVM change is
> to post the kernel patches first, and then update the KVM-unit-test patch(es) to
> provide the lore link to the kernel patches.
> 
> Thanks for asking, and a _huge_ thanks for writing tests!
> ...

My pleasure ;) I've noticed two months have passed, but the test was not
merged. Is there something I should improve?

thanks,
Michal

Re: [PATCH 0/3] KVM: x86/emulator: Segment load fixes

[Sean Christopherson]

On Mon, Mar 27, 2023, Michal Luczaj wrote:
> On 1/26/23 18:33, Sean Christopherson wrote:
> > On Thu, Jan 26, 2023, Michal Luczaj wrote:
> >> Two small fixes for __load_segment_descriptor(), along with a KUT
> >> x86/emulator test.
> >>
> >> And a question to maintainers: is it ok to send patches for two repos in
> >> one series?
> > 
> > No, in the future please send two separate series (no need to do so for this case).
> > Us humans can easily figure out what's going on, but b4[*] gets confused, e.g.
> > `b4 am` will fail.
> > 
> > What I usually do to connect the KVM-unit-test change to the kernel/KVM change is
> > to post the kernel patches first, and then update the KVM-unit-test patch(es) to
> > provide the lore link to the kernel patches.
> > 
> > Thanks for asking, and a _huge_ thanks for writing tests!
> > ...
> 
> My pleasure ;) I've noticed two months have passed, but the test was not
> merged. Is there something I should improve?

Nope, KUT x86 is simply lagging badly on maintenance.  I am planning on putting
together a pull request this week, I'll make sure your patch is included.

Re: [PATCH 0/3] KVM: x86/emulator: Segment load fixes

[Sean Christopherson]

On Thu, 26 Jan 2023 02:34:02 +0100, Michal Luczaj wrote:
> Two small fixes for __load_segment_descriptor(), along with a KUT
> x86/emulator test.
> 
> And a question to maintainers: is it ok to send patches for two repos in
> one series?
> 
> Michal Luczaj (3):
> 
> [...]

Applied the KUT patch to kvm-x86 next, thanks!

[3/3] x86: Test CPL=3 DS/ES/FS/GS RPL=DPL=0 segment descriptor load
      https://github.com/kvm-x86/kvm-unit-tests/commit/05b0460e7f29

--
https://github.com/kvm-x86/kvm-unit-tests/tree/next