[PATCH] x86/PV: properly populate descriptor tables

[PATCH] x86/PV: properly populate descriptor tables

[Jan Beulich]

[-- Attachment #1: Type: text/plain, Size: 3001 bytes --]

Us extending the GDT limit past the Xen descriptors so far meant that
guests (including user mode programs) accessing any descriptor table
slot above the original OS'es limit but below the first Xen descriptor
caused a #PF, converted to a #GP in our #PF handler. Which is quite
different from the native behavior, where some of such accesses (LAR
and LSL) don't fault. Mimic that behavior by mapping a blank page into
unused slots.

While not strictly required, treat the LDT the same for consistency.

Reported-by: Andrew Cooper <andrew.cooper3@citrix.com>
Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
Not sure about 4.6 here: Beyond Andrew noticing I don't think anyone
ran into this issue in a real world environment, and hence it doesn't
seem to be too critical to get this fixed.

--- a/xen/arch/x86/mm.c
+++ b/xen/arch/x86/mm.c
@@ -502,12 +502,13 @@ void update_cr3(struct vcpu *v)
     make_cr3(v, cr3_mfn);
 }
 
+static const char __section(".bss.page_aligned") zero_page[PAGE_SIZE];
 
 static void invalidate_shadow_ldt(struct vcpu *v, int flush)
 {
     l1_pgentry_t *pl1e;
-    int i;
-    unsigned long pfn;
+    unsigned int i;
+    unsigned long pfn, zero_pfn = PFN_DOWN(__pa(zero_page));
     struct page_info *page;
 
     BUG_ON(unlikely(in_irq()));
@@ -523,8 +524,9 @@ static void invalidate_shadow_ldt(struct
     for ( i = 16; i < 32; i++ )
     {
         pfn = l1e_get_pfn(pl1e[i]);
-        if ( pfn == 0 ) continue;
-        l1e_write(&pl1e[i], l1e_empty());
+        if ( !(l1e_get_flags(pl1e[i]) & _PAGE_PRESENT) || pfn == zero_pfn )
+            continue;
+        l1e_write(&pl1e[i], l1e_from_pfn(zero_pfn, __PAGE_HYPERVISOR_RO));
         page = mfn_to_page(pfn);
         ASSERT_PAGE_IS_TYPE(page, PGT_seg_desc_page);
         ASSERT_PAGE_IS_DOMAIN(page, v->domain);
@@ -4420,16 +4422,17 @@ long do_update_va_mapping_otherdomain(un
 void destroy_gdt(struct vcpu *v)
 {
     l1_pgentry_t *pl1e;
-    int i;
-    unsigned long pfn;
+    unsigned int i;
+    unsigned long pfn, zero_pfn = PFN_DOWN(__pa(zero_page));
 
     v->arch.pv_vcpu.gdt_ents = 0;
     pl1e = gdt_ldt_ptes(v->domain, v);
     for ( i = 0; i < FIRST_RESERVED_GDT_PAGE; i++ )
     {
-        if ( (pfn = l1e_get_pfn(pl1e[i])) != 0 )
+        pfn = l1e_get_pfn(pl1e[i]);
+        if ( (l1e_get_flags(pl1e[i]) & _PAGE_PRESENT) && pfn != zero_pfn )
             put_page_and_type(mfn_to_page(pfn));
-        l1e_write(&pl1e[i], l1e_empty());
+        l1e_write(&pl1e[i], l1e_from_pfn(zero_pfn, __PAGE_HYPERVISOR_RO));
         v->arch.pv_vcpu.gdt_frames[i] = 0;
     }
 }
@@ -4442,7 +4445,7 @@ long set_gdt(struct vcpu *v, 
     struct domain *d = v->domain;
     l1_pgentry_t *pl1e;
     /* NB. There are 512 8-byte entries per GDT page. */
-    int i, nr_pages = (entries + 511) / 512;
+    unsigned int i, nr_pages = (entries + 511) / 512;
 
     if ( entries > FIRST_RESERVED_GDT_ENTRY )
         return -EINVAL;




[-- Attachment #2: x86-zero-fill-dt.patch --]
[-- Type: text/plain, Size: 3042 bytes --]

x86/PV: properly populate descriptor tables

Us extending the GDT limit past the Xen descriptors so far meant that
guests (including user mode programs) accessing any descriptor table
slot above the original OS'es limit but below the first Xen descriptor
caused a #PF, converted to a #GP in our #PF handler. Which is quite
different from the native behavior, where some of such accesses (LAR
and LSL) don't fault. Mimic that behavior by mapping a blank page into
unused slots.

While not strictly required, treat the LDT the same for consistency.

Reported-by: Andrew Cooper <andrew.cooper3@citrix.com>
Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
Not sure about 4.6 here: Beyond Andrew noticing I don't think anyone
ran into this issue in a real world environment, and hence it doesn't
seem to be too critical to get this fixed.

--- a/xen/arch/x86/mm.c
+++ b/xen/arch/x86/mm.c
@@ -502,12 +502,13 @@ void update_cr3(struct vcpu *v)
     make_cr3(v, cr3_mfn);
 }
 
+static const char __section(".bss.page_aligned") zero_page[PAGE_SIZE];
 
 static void invalidate_shadow_ldt(struct vcpu *v, int flush)
 {
     l1_pgentry_t *pl1e;
-    int i;
-    unsigned long pfn;
+    unsigned int i;
+    unsigned long pfn, zero_pfn = PFN_DOWN(__pa(zero_page));
     struct page_info *page;
 
     BUG_ON(unlikely(in_irq()));
@@ -523,8 +524,9 @@ static void invalidate_shadow_ldt(struct
     for ( i = 16; i < 32; i++ )
     {
         pfn = l1e_get_pfn(pl1e[i]);
-        if ( pfn == 0 ) continue;
-        l1e_write(&pl1e[i], l1e_empty());
+        if ( !(l1e_get_flags(pl1e[i]) & _PAGE_PRESENT) || pfn == zero_pfn )
+            continue;
+        l1e_write(&pl1e[i], l1e_from_pfn(zero_pfn, __PAGE_HYPERVISOR_RO));
         page = mfn_to_page(pfn);
         ASSERT_PAGE_IS_TYPE(page, PGT_seg_desc_page);
         ASSERT_PAGE_IS_DOMAIN(page, v->domain);
@@ -4420,16 +4422,17 @@ long do_update_va_mapping_otherdomain(un
 void destroy_gdt(struct vcpu *v)
 {
     l1_pgentry_t *pl1e;
-    int i;
-    unsigned long pfn;
+    unsigned int i;
+    unsigned long pfn, zero_pfn = PFN_DOWN(__pa(zero_page));
 
     v->arch.pv_vcpu.gdt_ents = 0;
     pl1e = gdt_ldt_ptes(v->domain, v);
     for ( i = 0; i < FIRST_RESERVED_GDT_PAGE; i++ )
     {
-        if ( (pfn = l1e_get_pfn(pl1e[i])) != 0 )
+        pfn = l1e_get_pfn(pl1e[i]);
+        if ( (l1e_get_flags(pl1e[i]) & _PAGE_PRESENT) && pfn != zero_pfn )
             put_page_and_type(mfn_to_page(pfn));
-        l1e_write(&pl1e[i], l1e_empty());
+        l1e_write(&pl1e[i], l1e_from_pfn(zero_pfn, __PAGE_HYPERVISOR_RO));
         v->arch.pv_vcpu.gdt_frames[i] = 0;
     }
 }
@@ -4442,7 +4445,7 @@ long set_gdt(struct vcpu *v, 
     struct domain *d = v->domain;
     l1_pgentry_t *pl1e;
     /* NB. There are 512 8-byte entries per GDT page. */
-    int i, nr_pages = (entries + 511) / 512;
+    unsigned int i, nr_pages = (entries + 511) / 512;
 
     if ( entries > FIRST_RESERVED_GDT_ENTRY )
         return -EINVAL;

[-- Attachment #3: Type: text/plain, Size: 126 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [PATCH] x86/PV: properly populate descriptor tables

[Andrew Cooper]

On 23/09/15 16:34, Jan Beulich wrote:
> Us extending the GDT limit past the Xen descriptors so far meant that
> guests (including user mode programs) accessing any descriptor table
> slot above the original OS'es limit but below the first Xen descriptor
> caused a #PF, converted to a #GP in our #PF handler. Which is quite
> different from the native behavior, where some of such accesses (LAR
> and LSL) don't fault. Mimic that behavior by mapping a blank page into
> unused slots.
>
> While not strictly required, treat the LDT the same for consistency.
>
> Reported-by: Andrew Cooper <andrew.cooper3@citrix.com>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Oh - thanks for doing this.  (Luckily, I hadn't yet had enough time to 
look into it.)

Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>

> ---
> Not sure about 4.6 here: Beyond Andrew noticing I don't think anyone
> ran into this issue in a real world environment, and hence it doesn't
> seem to be too critical to get this fixed.

This bug causes unexpected #GP faults being handed to PV guests which 
would not occur on native, when using the `lsl` and `lsr` instructions.  
I expect it is the rarity of those instructions which is why this has 
gone unnoticed for so long.

It probably should be backported.

~Andrew

Re: [PATCH] x86/PV: properly populate descriptor tables

[Wei Liu]

On Wed, Sep 23, 2015 at 09:34:16AM -0600, Jan Beulich wrote:
> Us extending the GDT limit past the Xen descriptors so far meant that
> guests (including user mode programs) accessing any descriptor table
> slot above the original OS'es limit but below the first Xen descriptor
> caused a #PF, converted to a #GP in our #PF handler. Which is quite
> different from the native behavior, where some of such accesses (LAR
> and LSL) don't fault. Mimic that behavior by mapping a blank page into
> unused slots.
> 
> While not strictly required, treat the LDT the same for consistency.
> 
> Reported-by: Andrew Cooper <andrew.cooper3@citrix.com>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> ---
> Not sure about 4.6 here: Beyond Andrew noticing I don't think anyone
> ran into this issue in a real world environment, and hence it doesn't
> seem to be too critical to get this fixed.
> 

Given that this is not absolutely essential (fix critical bug or
regressions) I would like it to go in after 4.6.

Wei.

Re: [PATCH] x86/PV: properly populate descriptor tables

[David Vrabel]

On 23/09/15 16:34, Jan Beulich wrote:
> Us extending the GDT limit past the Xen descriptors so far meant that
> guests (including user mode programs) accessing any descriptor table
> slot above the original OS'es limit but below the first Xen descriptor
> caused a #PF, converted to a #GP in our #PF handler. Which is quite
> different from the native behavior, where some of such accesses (LAR
> and LSL) don't fault. Mimic that behavior by mapping a blank page into
> unused slots.
> 
> While not strictly required, treat the LDT the same for consistency.

This change causes a 32-bit userspace process running in a 32-bit PV
guest to segfault.

The process is a Go program and it is using the modify_ldt() system call
(which is successful) but loading %gs with the new descriptor causes a
fault.  Even a minimal (empty main()) go program faults.

# strace ./go-crash
execve("./go-crash", ["./go-crash"], [/* 21 vars */]) = 0
modify_ldt(1, {entry_number:7, base_addr:0x80a0b28, limit:1048575,
seg_32bit:1, contents:0, read_exec_only:0, limit_in_pages:1,
seg_not_present:0, useable:1}, 16) = 0
--- SIGSEGV (Segmentation fault) @ 0 (0) ---

# gdb ./go-crash
...
(gdb) run
Starting program: /root/go-crash

Program received signal SIGSEGV, Segmentation fault.
runtime.setldt () at /usr/local/go/src/runtime/sys_linux_386.s:426
426	/usr/local/go/src/runtime/sys_linux_386.s: No such file or directory.
	in /usr/local/go/src/runtime/sys_linux_386.s
(gdb) info registers
eax            0x3f	63
ecx            0xbfffe4f8	-1073748744
edx            0x10	16
ebx            0x1	1
esp            0xbfffe4e8	0xbfffe4e8
ebp            0x809b180	0x809b180
esi            0x0	0
edi            0x0	0
eip            0x80700a1	0x80700a1 <runtime.setldt+81>
eflags         0x10206	[ PF IF RF ]
cs             0x73	115
ss             0x7b	123
ds             0x7b	123
es             0x7b	123
fs             0x0	0
gs             0x0	0

(gdb) disassem
Dump of assembler code for function runtime.setldt:
...
0x08070060 <runtime.setldt+16>:	lea    0x10(%esp),%eax
0x08070064 <runtime.setldt+20>:	mov    %ebx,(%eax)
0x08070066 <runtime.setldt+22>:	mov    %ecx,0x4(%eax)
0x08070069 <runtime.setldt+25>:	movl   $0xfffff,0x8(%eax)
0x08070070 <runtime.setldt+32>:	movl   $0x51,0xc(%eax)
0x08070077 <runtime.setldt+39>:	mov    $0x1,%ebx
0x0807007c <runtime.setldt+44>:	mov    %eax,%ecx
0x0807007e <runtime.setldt+46>:	mov    $0x10,%edx
0x08070083 <runtime.setldt+51>:	mov    $0x7b,%eax
0x08070088 <runtime.setldt+56>:	call   *0x809800c
0x0807008e <runtime.setldt+62>:	cmp    $0xfffff001,%eax
0x08070093 <runtime.setldt+67>:	jbe    0x8070097 <runtime.setldt+71>
0x08070095 <runtime.setldt+69>:	int    $0x3
0x08070097 <runtime.setldt+71>:	mov    0x24(%esp),%eax
0x0807009b <runtime.setldt+75>:	shl    $0x3,%eax
0x0807009e <runtime.setldt+78>:	add    $0x7,%eax
0x080700a1 <runtime.setldt+81>:	mov    %eax,%gs <-- FAULT
0x080700a3 <runtime.setldt+83>:	add    $0x20,%esp
0x080700a6 <runtime.setldt+86>:	ret

David

Re: [PATCH] x86/PV: properly populate descriptor tables

[Andrew Cooper]

On 26/10/15 14:43, David Vrabel wrote:
> On 23/09/15 16:34, Jan Beulich wrote:
>> Us extending the GDT limit past the Xen descriptors so far meant that
>> guests (including user mode programs) accessing any descriptor table
>> slot above the original OS'es limit but below the first Xen descriptor
>> caused a #PF, converted to a #GP in our #PF handler. Which is quite
>> different from the native behavior, where some of such accesses (LAR
>> and LSL) don't fault. Mimic that behavior by mapping a blank page into
>> unused slots.
>>
>> While not strictly required, treat the LDT the same for consistency.
> This change causes a 32-bit userspace process running in a 32-bit PV
> guest to segfault.
>
> The process is a Go program and it is using the modify_ldt() system call
> (which is successful) but loading %gs with the new descriptor causes a
> fault.  Even a minimal (empty main()) go program faults.

D'uh - its obvious now you point it out.

By filling the shadow ldt slots as present, zero entries, we break their
demand-faulting.

We can't be safe to incorrect faults from LAR/LSL, *and* perform demand
faulting of the LDT.

Reverting hunk 2 for now is the best course of action.

~Andrew

Re: [PATCH] x86/PV: properly populate descriptor tables

[Andrew Cooper]

On 26/10/15 14:55, Andrew Cooper wrote:
> On 26/10/15 14:43, David Vrabel wrote:
>> On 23/09/15 16:34, Jan Beulich wrote:
>>> Us extending the GDT limit past the Xen descriptors so far meant that
>>> guests (including user mode programs) accessing any descriptor table
>>> slot above the original OS'es limit but below the first Xen descriptor
>>> caused a #PF, converted to a #GP in our #PF handler. Which is quite
>>> different from the native behavior, where some of such accesses (LAR
>>> and LSL) don't fault. Mimic that behavior by mapping a blank page into
>>> unused slots.
>>>
>>> While not strictly required, treat the LDT the same for consistency.
>> This change causes a 32-bit userspace process running in a 32-bit PV
>> guest to segfault.
>>
>> The process is a Go program and it is using the modify_ldt() system call
>> (which is successful) but loading %gs with the new descriptor causes a
>> fault.  Even a minimal (empty main()) go program faults.
> D'uh - its obvious now you point it out.
>
> By filling the shadow ldt slots as present, zero entries, we break their
> demand-faulting.
>
> We can't be safe to incorrect faults from LAR/LSL, *and* perform demand
> faulting of the LDT.

Wait.  Yes we can.  I am talking nonsense.

Hunk 2 should be reverted, and the demand fault handler should populate
a zero entry rather than passing #GP back to the guest.

~Andrew

Re: [PATCH] x86/PV: properly populate descriptor tables

[Jan Beulich]

>>> On 26.10.15 at 15:58, <andrew.cooper3@citrix.com> wrote:
> On 26/10/15 14:55, Andrew Cooper wrote:
>> On 26/10/15 14:43, David Vrabel wrote:
>>> On 23/09/15 16:34, Jan Beulich wrote:
>>>> Us extending the GDT limit past the Xen descriptors so far meant that
>>>> guests (including user mode programs) accessing any descriptor table
>>>> slot above the original OS'es limit but below the first Xen descriptor
>>>> caused a #PF, converted to a #GP in our #PF handler. Which is quite
>>>> different from the native behavior, where some of such accesses (LAR
>>>> and LSL) don't fault. Mimic that behavior by mapping a blank page into
>>>> unused slots.
>>>>
>>>> While not strictly required, treat the LDT the same for consistency.
>>> This change causes a 32-bit userspace process running in a 32-bit PV
>>> guest to segfault.
>>>
>>> The process is a Go program and it is using the modify_ldt() system call
>>> (which is successful) but loading %gs with the new descriptor causes a
>>> fault.  Even a minimal (empty main()) go program faults.
>> D'uh - its obvious now you point it out.
>>
>> By filling the shadow ldt slots as present, zero entries, we break their
>> demand-faulting.
>>
>> We can't be safe to incorrect faults from LAR/LSL, *and* perform demand
>> faulting of the LDT.
> 
> Wait.  Yes we can.  I am talking nonsense.
> 
> Hunk 2 should be reverted, and the demand fault handler should populate
> a zero entry rather than passing #GP back to the guest.

Considering this

"While not strictly required, treat the LDT the same for consistency."

in the changelog, simply reverting the LDT part would seem
sufficient to me (albeit that's more than just hunk 2 afaics).

Jan

Re: [PATCH] x86/PV: properly populate descriptor tables

[David Vrabel]

On 26/10/15 15:08, Jan Beulich wrote:
>>>> On 26.10.15 at 15:58, <andrew.cooper3@citrix.com> wrote:
>> On 26/10/15 14:55, Andrew Cooper wrote:
>>> On 26/10/15 14:43, David Vrabel wrote:
>>>> On 23/09/15 16:34, Jan Beulich wrote:
>>>>> Us extending the GDT limit past the Xen descriptors so far meant that
>>>>> guests (including user mode programs) accessing any descriptor table
>>>>> slot above the original OS'es limit but below the first Xen descriptor
>>>>> caused a #PF, converted to a #GP in our #PF handler. Which is quite
>>>>> different from the native behavior, where some of such accesses (LAR
>>>>> and LSL) don't fault. Mimic that behavior by mapping a blank page into
>>>>> unused slots.
>>>>>
>>>>> While not strictly required, treat the LDT the same for consistency.
>>>> This change causes a 32-bit userspace process running in a 32-bit PV
>>>> guest to segfault.
>>>>
>>>> The process is a Go program and it is using the modify_ldt() system call
>>>> (which is successful) but loading %gs with the new descriptor causes a
>>>> fault.  Even a minimal (empty main()) go program faults.
>>> D'uh - its obvious now you point it out.
>>>
>>> By filling the shadow ldt slots as present, zero entries, we break their
>>> demand-faulting.
>>>
>>> We can't be safe to incorrect faults from LAR/LSL, *and* perform demand
>>> faulting of the LDT.
>>
>> Wait.  Yes we can.  I am talking nonsense.
>>
>> Hunk 2 should be reverted, and the demand fault handler should populate
>> a zero entry rather than passing #GP back to the guest.
> 
> Considering this
> 
> "While not strictly required, treat the LDT the same for consistency."
> 
> in the changelog, simply reverting the LDT part would seem
> sufficient to me (albeit that's more than just hunk 2 afaics).

Apply this partial revert fixes the problem for me.

8<------------------------
--- a/xen/arch/x86/mm.c
+++ b/xen/arch/x86/mm.c
@@ -502,8 +502,8 @@ void update_cr3(struct vcpu *v)
 static void invalidate_shadow_ldt(struct vcpu *v, int flush)
 {
     l1_pgentry_t *pl1e;
-    unsigned int i;
-    unsigned long pfn, zero_pfn = PFN_DOWN(__pa(zero_page));
+    int i;
+    unsigned long pfn;
     struct page_info *page;
 
     BUG_ON(unlikely(in_irq()));
@@ -524,9 +523,8 @@ static void invalidate_shadow_ldt(struct vcpu *v, int flush)
     for ( i = 16; i < 32; i++ )
     {
         pfn = l1e_get_pfn(pl1e[i]);
-        if ( !(l1e_get_flags(pl1e[i]) & _PAGE_PRESENT) || pfn == zero_pfn )
-            continue;
-        l1e_write(&pl1e[i], l1e_from_pfn(zero_pfn, __PAGE_HYPERVISOR_RO));
+        if ( pfn == 0 ) continue;
+        l1e_write(&pl1e[i], l1e_empty());
         page = mfn_to_page(pfn);
         ASSERT_PAGE_IS_TYPE(page, PGT_seg_desc_page);
         ASSERT_PAGE_IS_DOMAIN(page, v->domain);

Re: [PATCH] x86/PV: properly populate descriptor tables

[Jan Beulich]

>>> On 26.10.15 at 16:41, <david.vrabel@citrix.com> wrote:
> Apply this partial revert fixes the problem for me.

This would need some cleaning up, but I could do that. May I put your
S-o-b underneath it (with or without the extra cleanup)?

Jan

> --- a/xen/arch/x86/mm.c
> +++ b/xen/arch/x86/mm.c
> @@ -502,8 +502,8 @@ void update_cr3(struct vcpu *v)
>  static void invalidate_shadow_ldt(struct vcpu *v, int flush)
>  {
>      l1_pgentry_t *pl1e;
> -    unsigned int i;
> -    unsigned long pfn, zero_pfn = PFN_DOWN(__pa(zero_page));
> +    int i;
> +    unsigned long pfn;
>      struct page_info *page;
>  
>      BUG_ON(unlikely(in_irq()));
> @@ -524,9 +523,8 @@ static void invalidate_shadow_ldt(struct vcpu *v, int flush)
>      for ( i = 16; i < 32; i++ )
>      {
>          pfn = l1e_get_pfn(pl1e[i]);
> -        if ( !(l1e_get_flags(pl1e[i]) & _PAGE_PRESENT) || pfn == zero_pfn )
> -            continue;
> -        l1e_write(&pl1e[i], l1e_from_pfn(zero_pfn, __PAGE_HYPERVISOR_RO));
> +        if ( pfn == 0 ) continue;
> +        l1e_write(&pl1e[i], l1e_empty());
>          page = mfn_to_page(pfn);
>          ASSERT_PAGE_IS_TYPE(page, PGT_seg_desc_page);
>          ASSERT_PAGE_IS_DOMAIN(page, v->domain);

Re: [PATCH] x86/PV: properly populate descriptor tables

[David Vrabel]

On 26/10/15 15:58, Jan Beulich wrote:
>>>> On 26.10.15 at 16:41, <david.vrabel@citrix.com> wrote:
>> Apply this partial revert fixes the problem for me.
> 
> This would need some cleaning up, but I could do that. May I put your
> S-o-b underneath it (with or without the extra cleanup)?

If you want, but I don't think I really did any work here so feel free
to just put your own name on it.

David