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

[Xen-devel] [PATCH 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 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.

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 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.

Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
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.

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


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

[Xen-devel] [PATCH 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>

--- a/xen/arch/x86/domain.c
+++ b/xen/arch/x86/domain.c
@@ -1089,7 +1089,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)];
@@ -1100,7 +1100,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 1/3] x86: relax GDT check in arch_set_info_guest()

[Andrew Cooper]

On 06/12/2019 10:14, Jan Beulich wrote:
> It is wrong for us to check frames beyond the guest specified limit.
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

I don't completely agree.  The code has been like this since it was
introduced, and is used to check data from the domain builder (inc
migration), and from the guests.

At the moment, every caller is required not to pass junk in unused
frames, and I don't see an issue with keeping this behaviour.

~Andrew

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

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

[Andrew Cooper]

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 <jbeulich@suse.com>
> ---
> 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

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

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

[Andrew Cooper]

On 06/12/2019 10:15, Jan Beulich wrote:
> 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>

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

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

[Jan Beulich]

On 06.12.2019 11:25, Andrew Cooper wrote:
> On 06/12/2019 10:14, Jan Beulich wrote:
>> It is wrong for us to check frames beyond the guest specified limit.
>>
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> 
> I don't completely agree.  The code has been like this since it was
> introduced, and is used to check data from the domain builder (inc
> migration), and from the guests.
> 
> At the moment, every caller is required not to pass junk in unused
> frames, and I don't see an issue with keeping this behaviour.

Keeping the behavior isn't going to break anything, yes, but it
shouldn't have been this way to begin with. I simply don't see
the value of validating data we're not consuming anyway. Perhaps
I could say "not helpful" or "pointless" instead of "wrong" ...

Jan

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

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

[Jan Beulich]

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 <jbeulich@suse.com>
>> ---
>> 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

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

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

[Andrew Cooper]

On 06/12/2019 11:32, Jan Beulich wrote:
> On 06.12.2019 11:25, Andrew Cooper wrote:
>> On 06/12/2019 10:14, Jan Beulich wrote:
>>> It is wrong for us to check frames beyond the guest specified limit.
>>>
>>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>> I don't completely agree.  The code has been like this since it was
>> introduced, and is used to check data from the domain builder (inc
>> migration), and from the guests.
>>
>> At the moment, every caller is required not to pass junk in unused
>> frames, and I don't see an issue with keeping this behaviour.
> Keeping the behavior isn't going to break anything, yes, but it
> shouldn't have been this way to begin with. I simply don't see
> the value of validating data we're not consuming anyway. Perhaps
> I could say "not helpful" or "pointless" instead of "wrong" ...

But in other cases we go out of our way to check parameters (especially
reserved fields) even when they aren't presently consumed.

i.e. what do we gain (other than more complicated code) by relaxing a
restriction we know is obeyed by every caller?

~Andrew

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

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

[Jan Beulich]

On 06.12.2019 20:51, Andrew Cooper wrote:
> On 06/12/2019 11:32, Jan Beulich wrote:
>> On 06.12.2019 11:25, Andrew Cooper wrote:
>>> On 06/12/2019 10:14, Jan Beulich wrote:
>>>> It is wrong for us to check frames beyond the guest specified limit.
>>>>
>>>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>>> I don't completely agree.  The code has been like this since it was
>>> introduced, and is used to check data from the domain builder (inc
>>> migration), and from the guests.
>>>
>>> At the moment, every caller is required not to pass junk in unused
>>> frames, and I don't see an issue with keeping this behaviour.
>> Keeping the behavior isn't going to break anything, yes, but it
>> shouldn't have been this way to begin with. I simply don't see
>> the value of validating data we're not consuming anyway. Perhaps
>> I could say "not helpful" or "pointless" instead of "wrong" ...
> 
> But in other cases we go out of our way to check parameters (especially
> reserved fields) even when they aren't presently consumed.

Which we do to make sure we could use the fields down the road
without breaking existing callers. That's quite different from
the overzealous checking we do here.

> i.e. what do we gain (other than more complicated code) by relaxing a
> restriction we know is obeyed by every caller?

First - I don't think the code gets more complicated by this
change (nor the LDT counterpart). If anything I'm seeing a
really minor simplification (by consistently using a now
common variable). Further, if you look closely, you'll note
that the compat path is already only checking the specified
number of frames. Hence I'm bringing the non-compat one in
line, i.e. an improvement in consistency.

Jan

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