[v2,2/3] x86: relax LDT check in arch_set_info_guest()
diff mbox series

Message ID c36cac91-49ae-6bb2-b057-195031979d21@suse.com
State New, archived
Headers show
Series
  • x86: XSA-298 follow-up
Related show

Commit Message

Jan Beulich Dec. 20, 2019, 1:50 p.m. UTC
It is wrong for us to check the base address when there's no LDT in the
first place. Once we don't do this check anymore we can also set the
base address to a non-canonical value when the LDT is empty.

Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
v2: Set v->arch.pv.ldt_base to non-canonical for an empty LDT, plus
    related necessary adjustments.

Comments

Andrew Cooper Dec. 27, 2019, 3:26 p.m. UTC | #1
On 20/12/2019 13:50, Jan Beulich wrote:
> It is wrong for us to check the base address when there's no LDT in the
> first place. Once we don't do this check anymore we can also set the
> base address to a non-canonical value when the LDT is empty.
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

I've only just spotted, but this is a semantic change to the guest. 
Previously, base with ents=0 would be preserved via arch_get_info_guest().

This is likely not interesting from a guests point of view, so is
probably fine to change in the ABI.

As for the change itself, do you realise that you've not actually
relaxed anything?  There are checks earlier in arch_set_info_guest()
which you haven't altered.

Finally, a similar concern about changes which a guest can't actually
make use of, even if this one seems rather more minor.

~Andrew
Jan Beulich Jan. 3, 2020, 10:31 a.m. UTC | #2
On 27.12.2019 16:26, Andrew Cooper wrote:
> On 20/12/2019 13:50, Jan Beulich wrote:
>> It is wrong for us to check the base address when there's no LDT in the
>> first place. Once we don't do this check anymore we can also set the
>> base address to a non-canonical value when the LDT is empty.
>>
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> 
> I've only just spotted, but this is a semantic change to the guest. 
> Previously, base with ents=0 would be preserved via arch_get_info_guest().

I've done (extended from v1) this upon your request; I did notice
this side effect of the change. This is (partly) why I've made an
adjustment to arch_get_info_guest() in the first place.

> Finally, a similar concern about changes which a guest can't actually
> make use of, even if this one seems rather more minor.

Like for the GDT case, the goal isn't so much to allow guests more
relaxed behavior (albeit for ones not caring about being compatible
with older Xen this is still a secondary goal), but to get behavior
in Xen into an overall more consistent shape.

Jan
Roger Pau Monné May 19, 2020, 9:02 a.m. UTC | #3
On Fri, Dec 20, 2019 at 02:50:06PM +0100, Jan Beulich wrote:
> It is wrong for us to check the base address when there's no LDT in the
> first place. Once we don't do this check anymore we can also set the
> base address to a non-canonical value when the LDT is empty.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>

I wonder if a ldt_ents check should also be added to
pv_map_ldt_shadow_page in order to avoid trying to get the mapping of
the LDT.

Thanks, Roger.
Jan Beulich May 19, 2020, 9:12 a.m. UTC | #4
On 19.05.2020 11:02, Roger Pau Monné wrote:
> On Fri, Dec 20, 2019 at 02:50:06PM +0100, Jan Beulich wrote:
>> It is wrong for us to check the base address when there's no LDT in the
>> first place. Once we don't do this check anymore we can also set the
>> base address to a non-canonical value when the LDT is empty.
>>
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> 
> Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>

Thanks.

> I wonder if a ldt_ents check should also be added to
> pv_map_ldt_shadow_page in order to avoid trying to get the mapping of
> the LDT.

We already have

    if ( unlikely((offset >> 3) >= curr->arch.pv.ldt_ents) )
    {
        ASSERT_UNREACHABLE();
        return false;
    }

What else do you mean?

Jan
Roger Pau Monné May 19, 2020, 9:18 a.m. UTC | #5
On Tue, May 19, 2020 at 11:12:49AM +0200, Jan Beulich wrote:
> On 19.05.2020 11:02, Roger Pau Monné wrote:
> > On Fri, Dec 20, 2019 at 02:50:06PM +0100, Jan Beulich wrote:
> >> It is wrong for us to check the base address when there's no LDT in the
> >> first place. Once we don't do this check anymore we can also set the
> >> base address to a non-canonical value when the LDT is empty.
> >>
> >> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> > 
> > Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>
> 
> Thanks.
> 
> > I wonder if a ldt_ents check should also be added to
> > pv_map_ldt_shadow_page in order to avoid trying to get the mapping of
> > the LDT.
> 
> We already have
> 
>     if ( unlikely((offset >> 3) >= curr->arch.pv.ldt_ents) )
>     {
>         ASSERT_UNREACHABLE();
>         return false;
>     }
> 
> What else do you mean?

Oh, I've missed that. I was searching for a check before accessing
ldt_base, which is done at initialization time. That's indeed fine.

Roger.

Patch
diff mbox series

--- a/xen/arch/x86/domain.c
+++ b/xen/arch/x86/domain.c
@@ -959,8 +959,10 @@  int arch_set_info_guest(
         if ( !compat && !(flags & VGCF_in_kernel) && !c.nat->ctrlreg[1] )
             return -EINVAL;
 
-        v->arch.pv.ldt_base = c(ldt_base);
         v->arch.pv.ldt_ents = c(ldt_ents);
+        v->arch.pv.ldt_base = v->arch.pv.ldt_ents
+                              ? c(ldt_base)
+                              : (unsigned long)ZERO_BLOCK_PTR;
     }
     else
     {
@@ -989,8 +991,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;
--- a/xen/arch/x86/domctl.c
+++ b/xen/arch/x86/domctl.c
@@ -1563,7 +1563,7 @@  void arch_get_info_guest(struct vcpu *v,
     }
     else
     {
-        c(ldt_base = v->arch.pv.ldt_base);
+        c(ldt_base = v->arch.pv.ldt_ents ? v->arch.pv.ldt_base : 0);
         c(ldt_ents = v->arch.pv.ldt_ents);
         for ( i = 0; i < ARRAY_SIZE(v->arch.pv.gdt_frames); ++i )
             c(gdt_frames[i] = v->arch.pv.gdt_frames[i]);
--- a/xen/arch/x86/mm.c
+++ b/xen/arch/x86/mm.c
@@ -3702,14 +3702,15 @@  long do_mmuext_op(
         case MMUEXT_SET_LDT:
         {
             unsigned int ents = op.arg2.nr_ents;
-            unsigned long ptr = ents ? op.arg1.linear_addr : 0;
+            unsigned long ptr = ents ? op.arg1.linear_addr
+                                     : (unsigned long)ZERO_BLOCK_PTR;
 
             if ( unlikely(currd != pg_owner) )
                 rc = -EPERM;
             else if ( paging_mode_external(currd) )
                 rc = -EINVAL;
-            else if ( ((ptr & (PAGE_SIZE - 1)) != 0) || !__addr_ok(ptr) ||
-                      (ents > 8192) )
+            else if ( (ents > 8192) ||
+                      (ents && ((ptr & (PAGE_SIZE - 1)) || !__addr_ok(ptr))) )
             {
                 gdprintk(XENLOG_WARNING,
                          "Bad args to SET_LDT: ptr=%lx, ents=%x\n", ptr, ents);