[PATCH] KVM: x86/vmx: Do not skip segment attributes if unusable bit is set

[PATCH] KVM: x86/vmx: Do not skip segment attributes if unusable bit is set

[Hendrik Borghorst]

When serializing and deserializing kvm_sregs, attributes of the segment
descriptors are stored by user space. For unusable segments,
vmx_segment_access_rights skips all attributes and sets them to 0.

This means we zero out the DPL (Descriptor Privilege Level) for unusable
entries.

Unusable segments are - contrary to their name - usable in 64bit mode and
are used by guests to for example create a linear map through the
NULL selector.

VMENTER checks if SS.DPL is correct depending on the CS segment type.
For types 9 (Execute Only) and 11 (Execute Read), CS.DPL must be equal to
SS.DPL [1].

We have seen real world guests setting CS to a usable segment with DPL=3
and SS to an unusable segment with DPL=3. Once we go through an sregs
get/set cycle, SS.DPL turns to 0. This causes the virtual machine to crash
reproducibly.

This commit changes the attribute logic to always preserve attributes for
unusable segments. According to [2] SS.DPL is always saved on VM exits,
regardless of the unusable bit so user space applications should have saved
the information on serialization correctly.

[3] specifies that besides SS.DPL the rest of the attributes of the
descriptors are undefined after VM entry if unusable bit is set. So, there
should be no harm in setting them all to the previous state.

[1] Intel SDM Vol 3C 26.3.1.2 Checks on Guest Segment Registers
[2] Intel SDM Vol 3C 27.3.2 Saving Segment Registers and Descriptor-Table
Registers
[3] Intel SDM Vol 3C 26.3.2.2 Loading Guest Segment Registers and
Descriptor-Table Registers

Cc: Alexander Graf <graf@amazon.de>
Signed-off-by: Hendrik Borghorst <hborghor@amazon.de>
---
 arch/x86/kvm/vmx/vmx.c | 21 +++++++++------------
 1 file changed, 9 insertions(+), 12 deletions(-)

diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
index 63247c57c72c..4ae248e87f5e 100644
--- a/arch/x86/kvm/vmx/vmx.c
+++ b/arch/x86/kvm/vmx/vmx.c
@@ -3412,18 +3412,15 @@ static u32 vmx_segment_access_rights(struct kvm_segment *var)
 {
 	u32 ar;
 
-	if (var->unusable || !var->present)
-		ar = 1 << 16;
-	else {
-		ar = var->type & 15;
-		ar |= (var->s & 1) << 4;
-		ar |= (var->dpl & 3) << 5;
-		ar |= (var->present & 1) << 7;
-		ar |= (var->avl & 1) << 12;
-		ar |= (var->l & 1) << 13;
-		ar |= (var->db & 1) << 14;
-		ar |= (var->g & 1) << 15;
-	}
+	ar = var->type & 15;
+	ar |= (var->s & 1) << 4;
+	ar |= (var->dpl & 3) << 5;
+	ar |= (var->present & 1) << 7;
+	ar |= (var->avl & 1) << 12;
+	ar |= (var->l & 1) << 13;
+	ar |= (var->db & 1) << 14;
+	ar |= (var->g & 1) << 15;
+	ar |= (var->unusable || !var->present) << 16;
 
 	return ar;
 }
-- 
2.37.1




Amazon Development Center Germany GmbH
Krausenstr. 38
10117 Berlin
Geschaeftsfuehrung: Christian Schlaeger, Jonathan Weiss
Eingetragen am Amtsgericht Charlottenburg unter HRB 149173 B
Sitz: Berlin
Ust-ID: DE 289 237 879

Re: [PATCH] KVM: x86/vmx: Do not skip segment attributes if unusable bit is set

[Jim Mattson]

On Mon, Nov 14, 2022 at 8:50 AM Hendrik Borghorst <hborghor@amazon.de> wrote:
>
> When serializing and deserializing kvm_sregs, attributes of the segment
> descriptors are stored by user space. For unusable segments,
> vmx_segment_access_rights skips all attributes and sets them to 0.
>
> This means we zero out the DPL (Descriptor Privilege Level) for unusable
> entries.
>
> Unusable segments are - contrary to their name - usable in 64bit mode and
> are used by guests to for example create a linear map through the
> NULL selector.
>
> VMENTER checks if SS.DPL is correct depending on the CS segment type.
> For types 9 (Execute Only) and 11 (Execute Read), CS.DPL must be equal to
> SS.DPL [1].
>
> We have seen real world guests setting CS to a usable segment with DPL=3
> and SS to an unusable segment with DPL=3. Once we go through an sregs
> get/set cycle, SS.DPL turns to 0. This causes the virtual machine to crash
> reproducibly.
>
> This commit changes the attribute logic to always preserve attributes for
> unusable segments. According to [2] SS.DPL is always saved on VM exits,
> regardless of the unusable bit so user space applications should have saved
> the information on serialization correctly.
>
> [3] specifies that besides SS.DPL the rest of the attributes of the
> descriptors are undefined after VM entry if unusable bit is set. So, there
> should be no harm in setting them all to the previous state.
>
> [1] Intel SDM Vol 3C 26.3.1.2 Checks on Guest Segment Registers
> [2] Intel SDM Vol 3C 27.3.2 Saving Segment Registers and Descriptor-Table
> Registers
> [3] Intel SDM Vol 3C 26.3.2.2 Loading Guest Segment Registers and
> Descriptor-Table Registers
>
> Cc: Alexander Graf <graf@amazon.de>
> Signed-off-by: Hendrik Borghorst <hborghor@amazon.de>
> ---
>  arch/x86/kvm/vmx/vmx.c | 21 +++++++++------------
>  1 file changed, 9 insertions(+), 12 deletions(-)
>
> diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
> index 63247c57c72c..4ae248e87f5e 100644
> --- a/arch/x86/kvm/vmx/vmx.c
> +++ b/arch/x86/kvm/vmx/vmx.c
> @@ -3412,18 +3412,15 @@ static u32 vmx_segment_access_rights(struct kvm_segment *var)
>  {
>         u32 ar;
>
> -       if (var->unusable || !var->present)
> -               ar = 1 << 16;
> -       else {
> -               ar = var->type & 15;
> -               ar |= (var->s & 1) << 4;
> -               ar |= (var->dpl & 3) << 5;
> -               ar |= (var->present & 1) << 7;
> -               ar |= (var->avl & 1) << 12;
> -               ar |= (var->l & 1) << 13;
> -               ar |= (var->db & 1) << 14;
> -               ar |= (var->g & 1) << 15;
> -       }
> +       ar = var->type & 15;
> +       ar |= (var->s & 1) << 4;
> +       ar |= (var->dpl & 3) << 5;
> +       ar |= (var->present & 1) << 7;
> +       ar |= (var->avl & 1) << 12;
> +       ar |= (var->l & 1) << 13;
> +       ar |= (var->db & 1) << 14;
> +       ar |= (var->g & 1) << 15;
> +       ar |= (var->unusable || !var->present) << 16;
>
>         return ar;
>  }
Reviewed-by: Jim Mattson <jmattson@google.com>

Re: [PATCH] KVM: x86/vmx: Do not skip segment attributes if unusable bit is set

[Alexander Graf]

On 14.11.22 17:48, Hendrik Borghorst wrote:
> When serializing and deserializing kvm_sregs, attributes of the segment
> descriptors are stored by user space. For unusable segments,
> vmx_segment_access_rights skips all attributes and sets them to 0.
>
> This means we zero out the DPL (Descriptor Privilege Level) for unusable
> entries.
>
> Unusable segments are - contrary to their name - usable in 64bit mode and
> are used by guests to for example create a linear map through the
> NULL selector.
>
> VMENTER checks if SS.DPL is correct depending on the CS segment type.
> For types 9 (Execute Only) and 11 (Execute Read), CS.DPL must be equal to
> SS.DPL [1].
>
> We have seen real world guests setting CS to a usable segment with DPL=3
> and SS to an unusable segment with DPL=3. Once we go through an sregs
> get/set cycle, SS.DPL turns to 0. This causes the virtual machine to crash
> reproducibly.
>
> This commit changes the attribute logic to always preserve attributes for
> unusable segments. According to [2] SS.DPL is always saved on VM exits,
> regardless of the unusable bit so user space applications should have saved
> the information on serialization correctly.
>
> [3] specifies that besides SS.DPL the rest of the attributes of the
> descriptors are undefined after VM entry if unusable bit is set. So, there
> should be no harm in setting them all to the previous state.
>
> [1] Intel SDM Vol 3C 26.3.1.2 Checks on Guest Segment Registers
> [2] Intel SDM Vol 3C 27.3.2 Saving Segment Registers and Descriptor-Table
> Registers
> [3] Intel SDM Vol 3C 26.3.2.2 Loading Guest Segment Registers and
> Descriptor-Table Registers
>
> Cc: Alexander Graf <graf@amazon.de>
> Signed-off-by: Hendrik Borghorst <hborghor@amazon.de>


I'm still fascinated by the fact that "unusable" means "is a valid 
segment under these conditions" :).

Reviewed-by: Alexander Graf <graf@amazon.com>

Alex





Amazon Development Center Germany GmbH
Krausenstr. 38
10117 Berlin
Geschaeftsfuehrung: Christian Schlaeger, Jonathan Weiss
Eingetragen am Amtsgericht Charlottenburg unter HRB 149173 B
Sitz: Berlin
Ust-ID: DE 289 237 879

Re: [PATCH] KVM: x86/vmx: Do not skip segment attributes if unusable bit is set

[Paolo Bonzini]

On 11/14/22 17:48, Hendrik Borghorst wrote:
> When serializing and deserializing kvm_sregs, attributes of the segment
> descriptors are stored by user space. For unusable segments,
> vmx_segment_access_rights skips all attributes and sets them to 0.
> 
> This means we zero out the DPL (Descriptor Privilege Level) for unusable
> entries.
> 
> Unusable segments are - contrary to their name - usable in 64bit mode and
> are used by guests to for example create a linear map through the
> NULL selector.
> 
> VMENTER checks if SS.DPL is correct depending on the CS segment type.
> For types 9 (Execute Only) and 11 (Execute Read), CS.DPL must be equal to
> SS.DPL [1].
> 
> We have seen real world guests setting CS to a usable segment with DPL=3
> and SS to an unusable segment with DPL=3. Once we go through an sregs
> get/set cycle, SS.DPL turns to 0. This causes the virtual machine to crash
> reproducibly.
> 
> This commit changes the attribute logic to always preserve attributes for
> unusable segments. According to [2] SS.DPL is always saved on VM exits,
> regardless of the unusable bit so user space applications should have saved
> the information on serialization correctly.
> 
> [3] specifies that besides SS.DPL the rest of the attributes of the
> descriptors are undefined after VM entry if unusable bit is set. So, there
> should be no harm in setting them all to the previous state.
> 
> [1] Intel SDM Vol 3C 26.3.1.2 Checks on Guest Segment Registers
> [2] Intel SDM Vol 3C 27.3.2 Saving Segment Registers and Descriptor-Table
> Registers
> [3] Intel SDM Vol 3C 26.3.2.2 Loading Guest Segment Registers and
> Descriptor-Table Registers
> 
> Cc: Alexander Graf <graf@amazon.de>
> Signed-off-by: Hendrik Borghorst <hborghor@amazon.de>
> ---
>   arch/x86/kvm/vmx/vmx.c | 21 +++++++++------------
>   1 file changed, 9 insertions(+), 12 deletions(-)

Hi Hendrik,

thanks for the patch!  I have queued it now for 6.2-rc6, and added Cc: 
stable.

Would you mind providing a test in the form of a patch for 
tools/testing/selftests/kvm?  I think a kvm-unit-tests testcase would be 
harder to do because there's no easy way to force 
KVM_GET_SREGS/KVM_SET_SREGS.

Thanks,

Paolo