Re: [PATCH v7 3/3] x86: Make the GDT remapping read-only on 64-bit

[H. Peter Anvin]

,"Luis R . Rodriguez" ,Stanislaw Gruszka 
,Peter Zijlstra ,Josh Poimboeuf 
,Vitaly Kuznetsov ,Tim Chen 
,Joerg Roedel 
,TF-8?B?UmFkaW0gS3LEjW3DocWZ?From: h...@zytor.com
Message-ID: <550f6209-025a-45e2-84e2-f00a3771c...@zytor.com>

On March 14, 2017 2:20:19 PM PDT, Thomas Garnier  wrote:
>On Tue, Mar 14, 2017 at 2:04 PM, Pavel Machek  wrote:
>> On Tue 2017-03-14 10:05:08, Thomas Garnier wrote:
>>> This patch makes the GDT remapped pages read-only to prevent
>corruption.
>>> This change is done only on 64-bit.
>>>
>>> The native_load_tr_desc function was adapted to correctly handle a
>>> read-only GDT. The LTR instruction always writes to the GDT TSS
>entry.
>>> This generates a page fault if the GDT is read-only. This change
>checks
>>> if the current GDT is a remap and swap GDTs as needed. This function
>was
>>> tested by booting multiple machines and checking hibernation works
>>> properly.
>>>
>>> KVM SVM and VMX were adapted to use the writeable GDT. On VMX, the
>>> per-cpu variable was removed for functions to fetch the original
>GDT.
>>> Instead of reloading the previous GDT, VMX will reload the fixmap
>GDT as
>>> expected. For testing, VMs were started and restored on multiple
>>> configurations.
>>>
>>> Signed-off-by: Thomas Garnier 
>>
>> Can we get the same change for 32-bit, too? Growing differences
>> between 32 and 64 bit are a bit of a problem...
>> Pavel
>
>It was discussed on previous versions that 32-bit read-only support
>would create issues that why it was favor for 64-bit only right now.
>
>>
>> --
>> (english) http://www.livejournal.com/~pavelmachek
>> (cesky, pictures)
>http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

We can't make the GDT read-only on 32 bits since we use task switches for 
last-resort recovery.  64 bits has IST instead.
-- 
Sent from my Android device with K-9 Mail. Please excuse my brevity.

Re: [PATCH v7 3/3] x86: Make the GDT remapping read-only on 64-bit

[H. Peter Anvin]

,"Luis R . Rodriguez" ,Stanislaw Gruszka 
,Peter Zijlstra ,Josh Poimboeuf 
,Vitaly Kuznetsov ,Tim Chen 
,Joerg Roedel 
,TF-8?B?UmFkaW0gS3LEjW3DocWZ?From: h...@zytor.com
Message-ID: <550f6209-025a-45e2-84e2-f00a3771c...@zytor.com>

On March 14, 2017 2:20:19 PM PDT, Thomas Garnier  wrote:
>On Tue, Mar 14, 2017 at 2:04 PM, Pavel Machek  wrote:
>> On Tue 2017-03-14 10:05:08, Thomas Garnier wrote:
>>> This patch makes the GDT remapped pages read-only to prevent
>corruption.
>>> This change is done only on 64-bit.
>>>
>>> The native_load_tr_desc function was adapted to correctly handle a
>>> read-only GDT. The LTR instruction always writes to the GDT TSS
>entry.
>>> This generates a page fault if the GDT is read-only. This change
>checks
>>> if the current GDT is a remap and swap GDTs as needed. This function
>was
>>> tested by booting multiple machines and checking hibernation works
>>> properly.
>>>
>>> KVM SVM and VMX were adapted to use the writeable GDT. On VMX, the
>>> per-cpu variable was removed for functions to fetch the original
>GDT.
>>> Instead of reloading the previous GDT, VMX will reload the fixmap
>GDT as
>>> expected. For testing, VMs were started and restored on multiple
>>> configurations.
>>>
>>> Signed-off-by: Thomas Garnier 
>>
>> Can we get the same change for 32-bit, too? Growing differences
>> between 32 and 64 bit are a bit of a problem...
>> Pavel
>
>It was discussed on previous versions that 32-bit read-only support
>would create issues that why it was favor for 64-bit only right now.
>
>>
>> --
>> (english) http://www.livejournal.com/~pavelmachek
>> (cesky, pictures)
>http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

We can't make the GDT read-only on 32 bits since we use task switches for 
last-resort recovery.  64 bits has IST instead.
-- 
Sent from my Android device with K-9 Mail. Please excuse my brevity.

Re: [PATCH v7 3/3] x86: Make the GDT remapping read-only on 64-bit

[Pavel Machek]

On Tue 2017-03-14 10:05:08, Thomas Garnier wrote:
> This patch makes the GDT remapped pages read-only to prevent corruption.
> This change is done only on 64-bit.
> 
> The native_load_tr_desc function was adapted to correctly handle a
> read-only GDT. The LTR instruction always writes to the GDT TSS entry.
> This generates a page fault if the GDT is read-only. This change checks
> if the current GDT is a remap and swap GDTs as needed. This function was
> tested by booting multiple machines and checking hibernation works
> properly.
> 
> KVM SVM and VMX were adapted to use the writeable GDT. On VMX, the
> per-cpu variable was removed for functions to fetch the original GDT.
> Instead of reloading the previous GDT, VMX will reload the fixmap GDT as
> expected. For testing, VMs were started and restored on multiple
> configurations.
> 
> Signed-off-by: Thomas Garnier 

Can we get the same change for 32-bit, too? Growing differences
between 32 and 64 bit are a bit of a problem...
Pavel

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) 
http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html


signature.asc
Description: Digital signature

Re: [PATCH v7 3/3] x86: Make the GDT remapping read-only on 64-bit

[Pavel Machek]

On Tue 2017-03-14 10:05:08, Thomas Garnier wrote:
> This patch makes the GDT remapped pages read-only to prevent corruption.
> This change is done only on 64-bit.
> 
> The native_load_tr_desc function was adapted to correctly handle a
> read-only GDT. The LTR instruction always writes to the GDT TSS entry.
> This generates a page fault if the GDT is read-only. This change checks
> if the current GDT is a remap and swap GDTs as needed. This function was
> tested by booting multiple machines and checking hibernation works
> properly.
> 
> KVM SVM and VMX were adapted to use the writeable GDT. On VMX, the
> per-cpu variable was removed for functions to fetch the original GDT.
> Instead of reloading the previous GDT, VMX will reload the fixmap GDT as
> expected. For testing, VMs were started and restored on multiple
> configurations.
> 
> Signed-off-by: Thomas Garnier 

Can we get the same change for 32-bit, too? Growing differences
between 32 and 64 bit are a bit of a problem...
Pavel

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) 
http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html


signature.asc
Description: Digital signature

[PATCH v7 3/3] x86: Make the GDT remapping read-only on 64-bit

[Thomas Garnier]

This patch makes the GDT remapped pages read-only to prevent corruption.
This change is done only on 64-bit.

The native_load_tr_desc function was adapted to correctly handle a
read-only GDT. The LTR instruction always writes to the GDT TSS entry.
This generates a page fault if the GDT is read-only. This change checks
if the current GDT is a remap and swap GDTs as needed. This function was
tested by booting multiple machines and checking hibernation works
properly.

KVM SVM and VMX were adapted to use the writeable GDT. On VMX, the
per-cpu variable was removed for functions to fetch the original GDT.
Instead of reloading the previous GDT, VMX will reload the fixmap GDT as
expected. For testing, VMs were started and restored on multiple
configurations.

Signed-off-by: Thomas Garnier 
---
Based on next-20170308
---
 arch/x86/include/asm/desc.h  | 106 +--
 arch/x86/include/asm/processor.h |   1 +
 arch/x86/kernel/cpu/common.c |  28 ---
 arch/x86/kvm/svm.c   |   4 +-
 arch/x86/kvm/vmx.c   |  12 ++---
 5 files changed, 96 insertions(+), 55 deletions(-)

diff --git a/arch/x86/include/asm/desc.h b/arch/x86/include/asm/desc.h
index 4b5ef0c64291..ec05f9c1a62c 100644
--- a/arch/x86/include/asm/desc.h
+++ b/arch/x86/include/asm/desc.h
@@ -248,9 +248,77 @@ static inline void native_set_ldt(const void *addr, 
unsigned int entries)
}
 }
 
+static inline void native_load_gdt(const struct desc_ptr *dtr)
+{
+   asm volatile("lgdt %0"::"m" (*dtr));
+}
+
+static inline void native_load_idt(const struct desc_ptr *dtr)
+{
+   asm volatile("lidt %0"::"m" (*dtr));
+}
+
+static inline void native_store_gdt(struct desc_ptr *dtr)
+{
+   asm volatile("sgdt %0":"=m" (*dtr));
+}
+
+static inline void native_store_idt(struct desc_ptr *dtr)
+{
+   asm volatile("sidt %0":"=m" (*dtr));
+}
+
+/*
+ * The LTR instruction marks the TSS GDT entry as busy. On 64-bit, the GDT is
+ * a read-only remapping. To prevent a page fault, the GDT is switched to the
+ * original writeable version when needed.
+ */
+#ifdef CONFIG_X86_64
 static inline void native_load_tr_desc(void)
 {
+   struct desc_ptr gdt;
+   int cpu = raw_smp_processor_id();
+   bool restore = 0;
+   struct desc_struct *fixmap_gdt;
+
+   native_store_gdt();
+   fixmap_gdt = get_cpu_gdt_ro(cpu);
+
+   /*
+* If the current GDT is the read-only fixmap, swap to the original
+* writeable version. Swap back at the end.
+*/
+   if (gdt.address == (unsigned long)fixmap_gdt) {
+   load_direct_gdt(cpu);
+   restore = 1;
+   }
asm volatile("ltr %w0"::"q" (GDT_ENTRY_TSS*8));
+   if (restore)
+   load_fixmap_gdt(cpu);
+}
+#else
+static inline void native_load_tr_desc(void)
+{
+   asm volatile("ltr %w0"::"q" (GDT_ENTRY_TSS*8));
+}
+#endif
+
+static inline unsigned long native_store_tr(void)
+{
+   unsigned long tr;
+
+   asm volatile("str %0":"=r" (tr));
+
+   return tr;
+}
+
+static inline void native_load_tls(struct thread_struct *t, unsigned int cpu)
+{
+   struct desc_struct *gdt = get_cpu_gdt_rw(cpu);
+   unsigned int i;
+
+   for (i = 0; i < GDT_ENTRY_TLS_ENTRIES; i++)
+   gdt[GDT_ENTRY_TLS_MIN + i] = t->tls_array[i];
 }
 
 DECLARE_PER_CPU(bool, __tss_limit_invalid);
@@ -305,44 +373,6 @@ static inline void invalidate_tss_limit(void)
this_cpu_write(__tss_limit_invalid, true);
 }
 
-static inline void native_load_gdt(const struct desc_ptr *dtr)
-{
-   asm volatile("lgdt %0"::"m" (*dtr));
-}
-
-static inline void native_load_idt(const struct desc_ptr *dtr)
-{
-   asm volatile("lidt %0"::"m" (*dtr));
-}
-
-static inline void native_store_gdt(struct desc_ptr *dtr)
-{
-   asm volatile("sgdt %0":"=m" (*dtr));
-}
-
-static inline void native_store_idt(struct desc_ptr *dtr)
-{
-   asm volatile("sidt %0":"=m" (*dtr));
-}
-
-static inline unsigned long native_store_tr(void)
-{
-   unsigned long tr;
-
-   asm volatile("str %0":"=r" (tr));
-
-   return tr;
-}
-
-static inline void native_load_tls(struct thread_struct *t, unsigned int cpu)
-{
-   struct desc_struct *gdt = get_cpu_gdt_rw(cpu);
-   unsigned int i;
-
-   for (i = 0; i < GDT_ENTRY_TLS_ENTRIES; i++)
-   gdt[GDT_ENTRY_TLS_MIN + i] = t->tls_array[i];
-}
-
 /* This intentionally ignores lm, since 32-bit apps don't have that field. */
 #define LDT_empty(info)\
((info)->base_addr  == 0&&  \
diff --git a/arch/x86/include/asm/processor.h b/arch/x86/include/asm/processor.h
index 2ec4d2dc559b..28828f1f99a4 100644
--- a/arch/x86/include/asm/processor.h
+++ b/arch/x86/include/asm/processor.h
@@ -716,6 +716,7 @@ extern struct desc_ptr  early_gdt_descr;
 
 extern void cpu_set_gdt(int);
 extern void switch_to_new_gdt(int);
+extern void 

[PATCH v7 3/3] x86: Make the GDT remapping read-only on 64-bit

[Thomas Garnier]

This patch makes the GDT remapped pages read-only to prevent corruption.
This change is done only on 64-bit.

The native_load_tr_desc function was adapted to correctly handle a
read-only GDT. The LTR instruction always writes to the GDT TSS entry.
This generates a page fault if the GDT is read-only. This change checks
if the current GDT is a remap and swap GDTs as needed. This function was
tested by booting multiple machines and checking hibernation works
properly.

KVM SVM and VMX were adapted to use the writeable GDT. On VMX, the
per-cpu variable was removed for functions to fetch the original GDT.
Instead of reloading the previous GDT, VMX will reload the fixmap GDT as
expected. For testing, VMs were started and restored on multiple
configurations.

Signed-off-by: Thomas Garnier 
---
Based on next-20170308
---
 arch/x86/include/asm/desc.h  | 106 +--
 arch/x86/include/asm/processor.h |   1 +
 arch/x86/kernel/cpu/common.c |  28 ---
 arch/x86/kvm/svm.c   |   4 +-
 arch/x86/kvm/vmx.c   |  12 ++---
 5 files changed, 96 insertions(+), 55 deletions(-)

diff --git a/arch/x86/include/asm/desc.h b/arch/x86/include/asm/desc.h
index 4b5ef0c64291..ec05f9c1a62c 100644
--- a/arch/x86/include/asm/desc.h
+++ b/arch/x86/include/asm/desc.h
@@ -248,9 +248,77 @@ static inline void native_set_ldt(const void *addr, 
unsigned int entries)
}
 }
 
+static inline void native_load_gdt(const struct desc_ptr *dtr)
+{
+   asm volatile("lgdt %0"::"m" (*dtr));
+}
+
+static inline void native_load_idt(const struct desc_ptr *dtr)
+{
+   asm volatile("lidt %0"::"m" (*dtr));
+}
+
+static inline void native_store_gdt(struct desc_ptr *dtr)
+{
+   asm volatile("sgdt %0":"=m" (*dtr));
+}
+
+static inline void native_store_idt(struct desc_ptr *dtr)
+{
+   asm volatile("sidt %0":"=m" (*dtr));
+}
+
+/*
+ * The LTR instruction marks the TSS GDT entry as busy. On 64-bit, the GDT is
+ * a read-only remapping. To prevent a page fault, the GDT is switched to the
+ * original writeable version when needed.
+ */
+#ifdef CONFIG_X86_64
 static inline void native_load_tr_desc(void)
 {
+   struct desc_ptr gdt;
+   int cpu = raw_smp_processor_id();
+   bool restore = 0;
+   struct desc_struct *fixmap_gdt;
+
+   native_store_gdt();
+   fixmap_gdt = get_cpu_gdt_ro(cpu);
+
+   /*
+* If the current GDT is the read-only fixmap, swap to the original
+* writeable version. Swap back at the end.
+*/
+   if (gdt.address == (unsigned long)fixmap_gdt) {
+   load_direct_gdt(cpu);
+   restore = 1;
+   }
asm volatile("ltr %w0"::"q" (GDT_ENTRY_TSS*8));
+   if (restore)
+   load_fixmap_gdt(cpu);
+}
+#else
+static inline void native_load_tr_desc(void)
+{
+   asm volatile("ltr %w0"::"q" (GDT_ENTRY_TSS*8));
+}
+#endif
+
+static inline unsigned long native_store_tr(void)
+{
+   unsigned long tr;
+
+   asm volatile("str %0":"=r" (tr));
+
+   return tr;
+}
+
+static inline void native_load_tls(struct thread_struct *t, unsigned int cpu)
+{
+   struct desc_struct *gdt = get_cpu_gdt_rw(cpu);
+   unsigned int i;
+
+   for (i = 0; i < GDT_ENTRY_TLS_ENTRIES; i++)
+   gdt[GDT_ENTRY_TLS_MIN + i] = t->tls_array[i];
 }
 
 DECLARE_PER_CPU(bool, __tss_limit_invalid);
@@ -305,44 +373,6 @@ static inline void invalidate_tss_limit(void)
this_cpu_write(__tss_limit_invalid, true);
 }
 
-static inline void native_load_gdt(const struct desc_ptr *dtr)
-{
-   asm volatile("lgdt %0"::"m" (*dtr));
-}
-
-static inline void native_load_idt(const struct desc_ptr *dtr)
-{
-   asm volatile("lidt %0"::"m" (*dtr));
-}
-
-static inline void native_store_gdt(struct desc_ptr *dtr)
-{
-   asm volatile("sgdt %0":"=m" (*dtr));
-}
-
-static inline void native_store_idt(struct desc_ptr *dtr)
-{
-   asm volatile("sidt %0":"=m" (*dtr));
-}
-
-static inline unsigned long native_store_tr(void)
-{
-   unsigned long tr;
-
-   asm volatile("str %0":"=r" (tr));
-
-   return tr;
-}
-
-static inline void native_load_tls(struct thread_struct *t, unsigned int cpu)
-{
-   struct desc_struct *gdt = get_cpu_gdt_rw(cpu);
-   unsigned int i;
-
-   for (i = 0; i < GDT_ENTRY_TLS_ENTRIES; i++)
-   gdt[GDT_ENTRY_TLS_MIN + i] = t->tls_array[i];
-}
-
 /* This intentionally ignores lm, since 32-bit apps don't have that field. */
 #define LDT_empty(info)\
((info)->base_addr  == 0&&  \
diff --git a/arch/x86/include/asm/processor.h b/arch/x86/include/asm/processor.h
index 2ec4d2dc559b..28828f1f99a4 100644
--- a/arch/x86/include/asm/processor.h
+++ b/arch/x86/include/asm/processor.h
@@ -716,6 +716,7 @@ extern struct desc_ptr  early_gdt_descr;
 
 extern void cpu_set_gdt(int);
 extern void switch_to_new_gdt(int);
+extern void load_direct_gdt(int);
 extern