[Xen-devel] [PATCH v2 0/3] x86: XSA-298 follow-up

[Xen-devel] [PATCH v2 0/3] x86: XSA-298 follow-up

[Jan Beulich]

A few things stumbled across while doing the investigations.

1: relax GDT check in arch_set_info_guest()
2: relax LDT check in arch_set_info_guest()
3: PV: polish pv_set_gdt()

Jan

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[Xen-devel] [PATCH v2 1/3] x86: relax GDT check in arch_set_info_guest()

[Jan Beulich]

It is wrong for us to check frames beyond the guest specified limit
(in the native case, other than in the compat one).

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/arch/x86/domain.c
+++ b/xen/arch/x86/domain.c
@@ -840,6 +840,7 @@ int arch_set_info_guest(
 #ifdef CONFIG_PV
     mfn_t cr3_mfn;
     struct page_info *cr3_page = NULL;
+    unsigned int nr_gdt_frames;
     int rc = 0;
 #endif
 
@@ -951,6 +952,8 @@ int arch_set_info_guest(
     /* Ensure real hardware interrupts are enabled. */
     v->arch.user_regs.eflags |= X86_EFLAGS_IF;
 
+    nr_gdt_frames = DIV_ROUND_UP(c(gdt_ents), 512);
+
     if ( !v->is_initialised )
     {
         if ( !compat && !(flags & VGCF_in_kernel) && !c.nat->ctrlreg[1] )
@@ -982,9 +985,9 @@ int arch_set_info_guest(
             fail = compat_pfn_to_cr3(pfn) != c.cmp->ctrlreg[3];
         }
 
-        for ( i = 0; i < ARRAY_SIZE(v->arch.pv.gdt_frames); ++i )
-            fail |= v->arch.pv.gdt_frames[i] != c(gdt_frames[i]);
         fail |= v->arch.pv.gdt_ents != c(gdt_ents);
+        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);
@@ -1089,12 +1092,11 @@ int arch_set_info_guest(
     else
     {
         unsigned long gdt_frames[ARRAY_SIZE(v->arch.pv.gdt_frames)];
-        unsigned int nr_frames = DIV_ROUND_UP(c.cmp->gdt_ents, 512);
 
-        if ( nr_frames > ARRAY_SIZE(v->arch.pv.gdt_frames) )
+        if ( nr_gdt_frames > ARRAY_SIZE(v->arch.pv.gdt_frames) )
             return -EINVAL;
 
-        for ( i = 0; i < nr_frames; ++i )
+        for ( i = 0; i < nr_gdt_frames; ++i )
             gdt_frames[i] = c.cmp->gdt_frames[i];
 
         rc = (int)pv_set_gdt(v, gdt_frames, c.cmp->gdt_ents);


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[Xen-devel] [PATCH v2 2/3] x86: relax LDT check in arch_set_info_guest()

[Jan Beulich]

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.

--- 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);


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[Xen-devel] [PATCH v2 3/3] x86/PV: polish pv_set_gdt()

[Jan Beulich]

There's no need to invoke get_page_from_gfn(), and there's also no need
to update the passed in frames[]. Invoke get_page_and_type() directly.

Also make the function's frames[] parameter const, change its return
type to int, and drop the bogus casts from two of its invocations.

Finally a little bit of cosmetics.

Signed-off-by: Jan Beulich <jbeulich@suse.com>
Acked-by: Andrew Cooper <andrew.cooper3@citrix.com>

--- a/xen/arch/x86/domain.c
+++ b/xen/arch/x86/domain.c
@@ -1091,7 +1091,7 @@ int arch_set_info_guest(
         return rc;
 
     if ( !compat )
-        rc = (int)pv_set_gdt(v, c.nat->gdt_frames, c.nat->gdt_ents);
+        rc = pv_set_gdt(v, c.nat->gdt_frames, c.nat->gdt_ents);
     else
     {
         unsigned long gdt_frames[ARRAY_SIZE(v->arch.pv.gdt_frames)];
@@ -1102,7 +1102,7 @@ int arch_set_info_guest(
         for ( i = 0; i < nr_gdt_frames; ++i )
             gdt_frames[i] = c.cmp->gdt_frames[i];
 
-        rc = (int)pv_set_gdt(v, gdt_frames, c.cmp->gdt_ents);
+        rc = pv_set_gdt(v, gdt_frames, c.cmp->gdt_ents);
     }
     if ( rc != 0 )
         return rc;
--- a/xen/arch/x86/pv/descriptor-tables.c
+++ b/xen/arch/x86/pv/descriptor-tables.c
@@ -96,7 +96,8 @@ void pv_destroy_gdt(struct vcpu *v)
     }
 }
 
-long pv_set_gdt(struct vcpu *v, unsigned long *frames, unsigned int entries)
+int pv_set_gdt(struct vcpu *v, const unsigned long frames[],
+               unsigned int entries)
 {
     struct domain *d = v->domain;
     l1_pgentry_t *pl1e;
@@ -110,17 +111,11 @@ long pv_set_gdt(struct vcpu *v, unsigned
     /* Check the pages in the new GDT. */
     for ( i = 0; i < nr_frames; i++ )
     {
-        struct page_info *page;
+        mfn_t mfn = _mfn(frames[i]);
 
-        page = get_page_from_gfn(d, frames[i], NULL, P2M_ALLOC);
-        if ( !page )
+        if ( !mfn_valid(mfn) ||
+             !get_page_and_type(mfn_to_page(mfn), d, PGT_seg_desc_page) )
             goto fail;
-        if ( !get_page_type(page, PGT_seg_desc_page) )
-        {
-            put_page(page);
-            goto fail;
-        }
-        frames[i] = mfn_x(page_to_mfn(page));
     }
 
     /* Tear down the old GDT. */
@@ -139,9 +134,8 @@ long pv_set_gdt(struct vcpu *v, unsigned
 
  fail:
     while ( i-- > 0 )
-    {
         put_page_and_type(mfn_to_page(_mfn(frames[i])));
-    }
+
     return -EINVAL;
 }
 
--- a/xen/include/asm-x86/pv/mm.h
+++ b/xen/include/asm-x86/pv/mm.h
@@ -25,7 +25,8 @@
 
 int pv_ro_page_fault(unsigned long addr, struct cpu_user_regs *regs);
 
-long pv_set_gdt(struct vcpu *v, unsigned long *frames, unsigned int entries);
+int pv_set_gdt(struct vcpu *v, const unsigned long frames[],
+               unsigned int entries);
 void pv_destroy_gdt(struct vcpu *v);
 
 bool pv_map_ldt_shadow_page(unsigned int off);
@@ -43,8 +44,8 @@ static inline int pv_ro_page_fault(unsig
     return 0;
 }
 
-static inline long pv_set_gdt(struct vcpu *v, unsigned long *frames,
-                              unsigned int entries)
+static inline int pv_set_gdt(struct vcpu *v, const unsigned long frames[],
+                             unsigned int entries)
 { ASSERT_UNREACHABLE(); return -EINVAL; }
 static inline void pv_destroy_gdt(struct vcpu *v) { ASSERT_UNREACHABLE(); }
 


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH v2 1/3] x86: relax GDT check in arch_set_info_guest()

[Andrew Cooper]

On 20/12/2019 13:49, Jan Beulich wrote:
> It is wrong for us to check frames beyond the guest specified limit
> (in the native case, other than in the compat one).
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Just like the restriction on sharing L2's, no guest is ever going to be
able to not zero all of this to operate on older hypervisors.

I agree that it is not ideal that this got into the ABI to begin with,
but as I said before, all you are doing is complicating
arch_set_info_guest() for a relaxation which no guest can use.

~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH v2 2/3] x86: relax LDT check in arch_set_info_guest()

[Andrew Cooper]

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

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH v2 1/3] x86: relax GDT check in arch_set_info_guest()

[Jan Beulich]

On 27.12.2019 16:09, Andrew Cooper wrote:
> On 20/12/2019 13:49, Jan Beulich wrote:
>> It is wrong for us to check frames beyond the guest specified limit
>> (in the native case, other than in the compat one).
>>
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> 
> Just like the restriction on sharing L2's, no guest is ever going to be
> able to not zero all of this to operate on older hypervisors.
> 
> I agree that it is not ideal that this got into the ABI to begin with,
> but as I said before, all you are doing is complicating
> arch_set_info_guest() for a relaxation which no guest can use.

As asked before - would you mind clarifying where I'm complicating
things? I think I'm rather simplifying matters, by
- pulling out a calculation, storing the result into a now common
  (between native and compat cases) local variable,
- as a result making native and compat cases behave consistently,
  eliminating the need for reader to (try to) figure out why there
  is a difference in behavior.

Jan

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH v2 2/3] x86: relax LDT check in arch_set_info_guest()

[Jan Beulich]

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

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH v2 1/3] x86: relax GDT check in arch_set_info_guest()

[Roger Pau Monné]

On Fri, Dec 20, 2019 at 02:49:48PM +0100, Jan Beulich wrote:
> It is wrong for us to check frames beyond the guest specified limit
> (in the native case, other than in the compat one).

Wouldn't this result in arch_set_info_guest failing if gdt_ents was
smaller than the maximum? Or all callers always pass gdt_ents set to
the maximum?

> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> 
> --- a/xen/arch/x86/domain.c
> +++ b/xen/arch/x86/domain.c
> @@ -840,6 +840,7 @@ int arch_set_info_guest(
>  #ifdef CONFIG_PV
>      mfn_t cr3_mfn;
>      struct page_info *cr3_page = NULL;
> +    unsigned int nr_gdt_frames;
>      int rc = 0;
>  #endif
>  
> @@ -951,6 +952,8 @@ int arch_set_info_guest(
>      /* Ensure real hardware interrupts are enabled. */
>      v->arch.user_regs.eflags |= X86_EFLAGS_IF;
>  
> +    nr_gdt_frames = DIV_ROUND_UP(c(gdt_ents), 512);
> +
>      if ( !v->is_initialised )
>      {
>          if ( !compat && !(flags & VGCF_in_kernel) && !c.nat->ctrlreg[1] )
> @@ -982,9 +985,9 @@ int arch_set_info_guest(
>              fail = compat_pfn_to_cr3(pfn) != c.cmp->ctrlreg[3];
>          }
>  
> -        for ( i = 0; i < ARRAY_SIZE(v->arch.pv.gdt_frames); ++i )
> -            fail |= v->arch.pv.gdt_frames[i] != c(gdt_frames[i]);
>          fail |= v->arch.pv.gdt_ents != c(gdt_ents);
> +        for ( i = 0; !fail && i < nr_gdt_frames; ++i )
> +            fail |= v->arch.pv.gdt_frames[i] != c(gdt_frames[i]);

fail doesn't need to be OR'ed anymore here, since you check for it in
the loop condition.

>  
>          fail |= v->arch.pv.ldt_base != c(ldt_base);
>          fail |= v->arch.pv.ldt_ents != c(ldt_ents);
> @@ -1089,12 +1092,11 @@ int arch_set_info_guest(
>      else
>      {
>          unsigned long gdt_frames[ARRAY_SIZE(v->arch.pv.gdt_frames)];
> -        unsigned int nr_frames = DIV_ROUND_UP(c.cmp->gdt_ents, 512);
>  
> -        if ( nr_frames > ARRAY_SIZE(v->arch.pv.gdt_frames) )
> +        if ( nr_gdt_frames > ARRAY_SIZE(v->arch.pv.gdt_frames) )
>              return -EINVAL;

Shouldn't this check be performed when nr_gdt_frames is initialized
instead of here? (as nr_gdt_frames is already used as a limit to
iterate over gdt_frames).

Thanks, Roger.

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

[Roger Pau Monné]

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.

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

[Jan Beulich]

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

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

[Roger Pau Monné]

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.

Re: [PATCH v2 1/3] x86: relax GDT check in arch_set_info_guest()

[Jan Beulich]

On 19.05.2020 10:42, Roger Pau Monné wrote:
> On Fri, Dec 20, 2019 at 02:49:48PM +0100, Jan Beulich wrote:
>> It is wrong for us to check frames beyond the guest specified limit
>> (in the native case, other than in the compat one).
> 
> Wouldn't this result in arch_set_info_guest failing if gdt_ents was
> smaller than the maximum? Or all callers always pass gdt_ents set to
> the maximum?

Since the array is embedded in the struct, I suppose callers simply
start out from a zero-initialized array, in which case the actual
count given doesn't matter. Additionally I think it is uncommon for
the function to get called on a vCPU with v->is_initialised already
set.

>> @@ -982,9 +985,9 @@ int arch_set_info_guest(
>>              fail = compat_pfn_to_cr3(pfn) != c.cmp->ctrlreg[3];
>>          }
>>  
>> -        for ( i = 0; i < ARRAY_SIZE(v->arch.pv.gdt_frames); ++i )
>> -            fail |= v->arch.pv.gdt_frames[i] != c(gdt_frames[i]);
>>          fail |= v->arch.pv.gdt_ents != c(gdt_ents);
>> +        for ( i = 0; !fail && i < nr_gdt_frames; ++i )
>> +            fail |= v->arch.pv.gdt_frames[i] != c(gdt_frames[i]);
> 
> fail doesn't need to be OR'ed anymore here, since you check for it in
> the loop condition.

Ah yes, changed.

>> @@ -1089,12 +1092,11 @@ int arch_set_info_guest(
>>      else
>>      {
>>          unsigned long gdt_frames[ARRAY_SIZE(v->arch.pv.gdt_frames)];
>> -        unsigned int nr_frames = DIV_ROUND_UP(c.cmp->gdt_ents, 512);
>>  
>> -        if ( nr_frames > ARRAY_SIZE(v->arch.pv.gdt_frames) )
>> +        if ( nr_gdt_frames > ARRAY_SIZE(v->arch.pv.gdt_frames) )
>>              return -EINVAL;
> 
> Shouldn't this check be performed when nr_gdt_frames is initialized
> instead of here? (as nr_gdt_frames is already used as a limit to
> iterate over gdt_frames).

Oh, yes, of course. Thanks for spotting.

Jan