[2/3] x86: relax LDT check in arch_set_info_guest()

Commit Message

Jan Beulich Dec. 6, 2019, 10:14 a.m. UTC

It is wrong for us to check the base address when there's no LDT in the
first place.

Signed-off-by: Jan Beulich <[e-mail]>
---
TBD: I also wonder whether we wouldn't better set v->arch.pv.ldt_base to
     zero for an empty LDT, just like do_mmuext_op() does.

Comments

Andrew Cooper Dec. 6, 2019, 10:33 a.m. UTC | #1

On 06/12/2019 10:14, Jan Beulich wrote:
> It is wrong for us to check the base address when there's no LDT in the
> first place.
>
> Signed-off-by: Jan Beulich <[e-mail]>
> ---
> TBD: I also wonder whether we wouldn't better set v->arch.pv.ldt_base to
>      zero for an empty LDT, just like do_mmuext_op() does.

My query with patch 1 is also applicable here.

As for setting it to zero, we should use something non-canonical
instead.  Doing so would have saved us from XSA-298, which was only a
problem in guests because the base falling to 0.

~Andrew

Jan Beulich Dec. 6, 2019, 11:35 a.m. UTC | #2

On 06.12.2019 11:33, Andrew Cooper wrote:
> On 06/12/2019 10:14, Jan Beulich wrote:
>> It is wrong for us to check the base address when there's no LDT in the
>> first place.
>>
>> Signed-off-by: Jan Beulich <[e-mail]>
>> ---
>> TBD: I also wonder whether we wouldn't better set v->arch.pv.ldt_base to
>>      zero for an empty LDT, just like do_mmuext_op() does.
> 
> My query with patch 1 is also applicable here.

As is my answer there.

> As for setting it to zero, we should use something non-canonical
> instead.  Doing so would have saved us from XSA-298, which was only a
> problem in guests because the base falling to 0.

I can certainly do so (in do_mmuext_op() then as well).

Jan

Patch

--- a/xen/arch/x86/domain.c
+++ b/xen/arch/x86/domain.c
@@ -989,8 +989,9 @@  int arch_set_info_guest(
         for ( i = 0; !fail && i < nr_gdt_frames; ++i )
             fail |= v->arch.pv.gdt_frames[i] != c(gdt_frames[i]);
 
-        fail |= v->arch.pv.ldt_base != c(ldt_base);
         fail |= v->arch.pv.ldt_ents != c(ldt_ents);
+        if ( v->arch.pv.ldt_ents )
+            fail |= v->arch.pv.ldt_base != c(ldt_base);
 
         if ( fail )
            return -EOPNOTSUPP;