lguest: Fix Malicious Guest GDT Host Crash

If a Guest makes hypercall which sets a GDT entry to not present, we currently set any segment registers using that GDT entry to 0. Unfortunately, this is not sufficient: there are other ways of altering GDT entries which will cause a fault. The correct solution to do what Linux does: let them set any GDT value they want and handle the #GP when popping causes a fault. This has the added benefit of making our Switcher slightly more robust in the case of any other bugs which cause it to fault. We kill the Guest if it causes a fault in the Switcher: it's the Guest's responsibility to make sure it's not using segments when it changes them. Signed-off-by: Rusty Russell <rusty@rustcorp.com.au> Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>

lguest: Fix Malicious Guest GDT Host Crash
If a Guest makes hypercall which sets a GDT entry to not present, we currently set any segment registers using that GDT entry to 0. Unfortunately, this is not sufficient: there are other ways of altering GDT entries which will cause a fault. The correct solution to do what Linux does: let them set any GDT value they want and handle the #GP when popping causes a fault. This has the added benefit of making our Switcher slightly more robust in the case of any other bugs which cause it to fault. We kill the Guest if it causes a fault in the Switcher: it's the Guest's responsibility to make sure it's not using segments when it changes them. Signed-off-by: Rusty Russell <rusty@rustcorp.com.au> Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>
Rusty Russell · Linus Torvalds
1 parent 37250097e1
Showing 5 changed files with 29 additions and 67 deletions Side-by-side Diff
drivers/lguest/core.c
drivers/lguest/interrupts_and_traps.c
drivers/lguest/lguest.c
drivers/lguest/segments.c
drivers/lguest/switcher.S
@@ -453,6 +453,11 @@
 	 * lguest_pages". */
 	copy_in_guest_info(lg, pages);
  
+	/* Set the trap number to 256 (impossible value).  If we fault while
+	 * switching to the Guest (bad segment registers or bug), this will
+	 * cause us to abort the Guest. */
+	lg->regs->trapnum = 256;
+
 	/* Now: we push the "eflags" register on the stack, then do an "lcall".
 	 * This is how we change from using the kernel code segment to using
 	 * the dedicated lguest code segment, as well as jumping into the
@@ -195,13 +195,16 @@
 /* deliver_trap() returns true if it could deliver the trap. */
 int deliver_trap(struct lguest *lg, unsigned int num)
 {
-	u32 lo = lg->idt[num].a, hi = lg->idt[num].b;
+	/* Trap numbers are always 8 bit, but we set an impossible trap number
+	 * for traps inside the Switcher, so check that here. */
+	if (num >= ARRAY_SIZE(lg->idt))
+		return 0;
  
 	/* Early on the Guest hasn't set the IDT entries (or maybe it put a
 	 * bogus one in): if we fail here, the Guest will be killed. */
-	if (!idt_present(lo, hi))
+	if (!idt_present(lg->idt[num].a, lg->idt[num].b))
 		return 0;
-	set_guest_interrupt(lg, lo, hi, has_err(num));
+	set_guest_interrupt(lg, lg->idt[num].a, lg->idt[num].b, has_err(num));
 	return 1;
 }
  
@@ -323,9 +323,12 @@
  * __thread variables).  So we have a hypercall specifically for this case. */
 static void lguest_load_tls(struct thread_struct *t, unsigned int cpu)
 {
+	/* There's one problem which normal hardware doesn't have: the Host
+	 * can't handle us removing entries we're currently using.  So we clear
+	 * the GS register here: if it's needed it'll be reloaded anyway. */
+	loadsegment(gs, 0);
 	lazy_hcall(LHCALL_LOAD_TLS, __pa(&t->tls_array), cpu, 0);
 }
-/*:*/
  
 /*G:038 That's enough excitement for now, back to ploughing through each of
  * the paravirt_ops (we're about 1/3 of the way through).
@@ -43,22 +43,6 @@
  * begin.
  */
  
-/* Is the descriptor the Guest wants us to put in OK?
- *
- * The flag which Intel says must be zero: must be zero.  The descriptor must
- * be present, (this is actually checked earlier but is here for thorougness),
- * and the descriptor type must be 1 (a memory segment).  */
-static int desc_ok(const struct desc_struct *gdt)
-{
-	return ((gdt->b & 0x00209000) == 0x00009000);
-}
-
-/* Is the segment present?  (Otherwise it can't be used by the Guest). */
-static int segment_present(const struct desc_struct *gdt)
-{
-	return gdt->b & 0x8000;
-}
-
 /* There are several entries we don't let the Guest set.  The TSS entry is the
  * "Task State Segment" which controls all kinds of delicate things.  The
  * LGUEST_CS and LGUEST_DS entries are reserved for the Switcher, and the
@@ -71,37 +55,11 @@
 		|| num == GDT_ENTRY_DOUBLEFAULT_TSS);
 }
  
-/* If the Guest asks us to remove an entry from the GDT, we have to be careful.
- * If one of the segment registers is pointing at that entry the Switcher will
- * crash when it tries to reload the segment registers for the Guest.
- *
- * It doesn't make much sense for the Guest to try to remove its own code, data
- * or stack segments while they're in use: assume that's a Guest bug.  If it's
- * one of the lesser segment registers using the removed entry, we simply set
- * that register to 0 (unusable). */
-static void check_segment_use(struct lguest *lg, unsigned int desc)
-{
-	/* GDT entries are 8 bytes long, so we divide to get the index and
-	 * ignore the bottom bits. */
-	if (lg->regs->gs / 8 == desc)
-		lg->regs->gs = 0;
-	if (lg->regs->fs / 8 == desc)
-		lg->regs->fs = 0;
-	if (lg->regs->es / 8 == desc)
-		lg->regs->es = 0;
-	if (lg->regs->ds / 8 == desc
-	    || lg->regs->cs / 8 == desc
-	    || lg->regs->ss / 8 == desc)
-		kill_guest(lg, "Removed live GDT entry %u", desc);
-}
-/*:*/
-/*M:009 We wouldn't need to check for removal of in-use segments if we handled
- * faults in the Switcher.  However, it's probably not a worthwhile
- * optimization. :*/
-
-/*H:610 Once the GDT has been changed, we look through the changed entries and
- * see if they're OK.  If not, we'll call kill_guest() and the Guest will never
- * get to use the invalid entries. */
+/*H:610 Once the GDT has been changed, we fix the new entries up a little.  We
+ * don't care if they're invalid: the worst that can happen is a General
+ * Protection Fault in the Switcher when it restores a Guest segment register
+ * which tries to use that entry.  Then we kill the Guest for causing such a
+ * mess: the message will be "unhandled trap 256". */
 static void fixup_gdt_table(struct lguest *lg, unsigned start, unsigned end)
 {
 	unsigned int i;
@@ -111,16 +69,6 @@
 		 * they say */
 		if (ignored_gdt(i))
 			continue;
-
-		/* We could fault in switch_to_guest if they are using
-		 * a removed segment. */
-		if (!segment_present(&lg->gdt[i])) {
-			check_segment_use(lg, i);
-			continue;
-		}
-
-		if (!desc_ok(&lg->gdt[i]))
-			kill_guest(lg, "Bad GDT descriptor %i", i);
  
 		/* Segment descriptors contain a privilege level: the Guest is
 		 * sometimes careless and leaves this as 0, even though it's
@@ -47,6 +47,7 @@
 // Down here in the depths of assembler code.
 #include <linux/linkage.h>
 #include <asm/asm-offsets.h>
+#include <asm/page.h>
 #include "lg.h"
  
 // We mark the start of the code to copy
  
@@ -182,13 +183,15 @@
 	movl	$(LGUEST_DS), %eax;					\
 	movl	%eax, %ds;						\
 	/* So where are we?  Which CPU, which struct?			\
-	 * The stack is our clue: our TSS sets				\
-	 * It at the end of "struct lguest_pages"			\
-	 * And we then pushed and pushed and pushed Guest regs:		\
-	 * Now stack points atop the "struct lguest_regs".   		\
-	 * Subtract that offset, and we find our struct. */		\
+	 * The stack is our clue: our TSS starts			\
+	 * It at the end of "struct lguest_pages".			\
+	 * Or we may have stumbled while restoring			\
+	 * Our Guest segment regs while in switch_to_guest,		\
+	 * The fault pushed atop that part-unwound stack.		\
+	 * If we round the stack down to the page start			\
+	 * We're at the start of "struct lguest_pages". */		\
 	movl	%esp, %eax;						\
-	subl	$LGUEST_PAGES_regs, %eax;				\
+	andl	$(~(1 << PAGE_SHIFT - 1)), %eax;			\
 	/* Save our trap number: the switch will obscure it		\
 	 * (The Guest regs are not mapped here in the Host)		\
 	 * %ebx holds it safe for deliver_to_host */			\
...	...	@@ -453,6 +453,11 @@
453	453	* lguest_pages". */
454	454	copy_in_guest_info(lg, pages);
455	455
	456	+ /* Set the trap number to 256 (impossible value). If we fault while
	457	+ * switching to the Guest (bad segment registers or bug), this will
	458	+ * cause us to abort the Guest. */
	459	+ lg->regs->trapnum = 256;
	460	+
456	461	/* Now: we push the "eflags" register on the stack, then do an "lcall".
457	462	* This is how we change from using the kernel code segment to using
458	463	* the dedicated lguest code segment, as well as jumping into the
...	...	@@ -195,13 +195,16 @@
195	195	/* deliver_trap() returns true if it could deliver the trap. */
196	196	int deliver_trap(struct lguest *lg, unsigned int num)
197	197	{
198		- u32 lo = lg->idt[num].a, hi = lg->idt[num].b;
	198	+ /* Trap numbers are always 8 bit, but we set an impossible trap number
	199	+ * for traps inside the Switcher, so check that here. */
	200	+ if (num >= ARRAY_SIZE(lg->idt))
	201	+ return 0;
199	202
200	203	/* Early on the Guest hasn't set the IDT entries (or maybe it put a
201	204	* bogus one in): if we fail here, the Guest will be killed. */
202		- if (!idt_present(lo, hi))
	205	+ if (!idt_present(lg->idt[num].a, lg->idt[num].b))
203	206	return 0;
204		- set_guest_interrupt(lg, lo, hi, has_err(num));
	207	+ set_guest_interrupt(lg, lg->idt[num].a, lg->idt[num].b, has_err(num));
205	208	return 1;
206	209	}
207	210
...	...	@@ -323,9 +323,12 @@
323	323	* __thread variables). So we have a hypercall specifically for this case. */
324	324	static void lguest_load_tls(struct thread_struct *t, unsigned int cpu)
325	325	{
	326	+ /* There's one problem which normal hardware doesn't have: the Host
	327	+ * can't handle us removing entries we're currently using. So we clear
	328	+ * the GS register here: if it's needed it'll be reloaded anyway. */
	329	+ loadsegment(gs, 0);
326	330	lazy_hcall(LHCALL_LOAD_TLS, __pa(&t->tls_array), cpu, 0);
327	331	}
328		-/:/
329	332
330	333	/*G:038 That's enough excitement for now, back to ploughing through each of
331	334	* the paravirt_ops (we're about 1/3 of the way through).
...	...	@@ -43,22 +43,6 @@
43	43	* begin.
44	44	*/
45	45
46		-/* Is the descriptor the Guest wants us to put in OK?
47		- *
48		- * The flag which Intel says must be zero: must be zero. The descriptor must
49		- * be present, (this is actually checked earlier but is here for thorougness),
50		- * and the descriptor type must be 1 (a memory segment). */
51		-static int desc_ok(const struct desc_struct *gdt)
52		-{
53		- return ((gdt->b & 0x00209000) == 0x00009000);
54		-}
55		-
56		-/* Is the segment present? (Otherwise it can't be used by the Guest). */
57		-static int segment_present(const struct desc_struct *gdt)
58		-{
59		- return gdt->b & 0x8000;
60		-}
61		-
62	46	/* There are several entries we don't let the Guest set. The TSS entry is the
63	47	* "Task State Segment" which controls all kinds of delicate things. The
64	48	* LGUEST_CS and LGUEST_DS entries are reserved for the Switcher, and the
...	...	@@ -71,37 +55,11 @@
71	55	\|\| num == GDT_ENTRY_DOUBLEFAULT_TSS);
72	56	}
73	57
74		-/* If the Guest asks us to remove an entry from the GDT, we have to be careful.
75		- * If one of the segment registers is pointing at that entry the Switcher will
76		- * crash when it tries to reload the segment registers for the Guest.
77		- *
78		- * It doesn't make much sense for the Guest to try to remove its own code, data
79		- * or stack segments while they're in use: assume that's a Guest bug. If it's
80		- * one of the lesser segment registers using the removed entry, we simply set
81		- * that register to 0 (unusable). */
82		-static void check_segment_use(struct lguest *lg, unsigned int desc)
83		-{
84		- /* GDT entries are 8 bytes long, so we divide to get the index and
85		- * ignore the bottom bits. */
86		- if (lg->regs->gs / 8 == desc)
87		- lg->regs->gs = 0;
88		- if (lg->regs->fs / 8 == desc)
89		- lg->regs->fs = 0;
90		- if (lg->regs->es / 8 == desc)
91		- lg->regs->es = 0;
92		- if (lg->regs->ds / 8 == desc
93		- \|\| lg->regs->cs / 8 == desc
94		- \|\| lg->regs->ss / 8 == desc)
95		- kill_guest(lg, "Removed live GDT entry %u", desc);
96		-}
97		-/:/
98		-/*M:009 We wouldn't need to check for removal of in-use segments if we handled
99		- * faults in the Switcher. However, it's probably not a worthwhile
100		- * optimization. :*/
101		-
102		-/*H:610 Once the GDT has been changed, we look through the changed entries and
103		- * see if they're OK. If not, we'll call kill_guest() and the Guest will never
104		- * get to use the invalid entries. */
	58	+/*H:610 Once the GDT has been changed, we fix the new entries up a little. We
	59	+ * don't care if they're invalid: the worst that can happen is a General
	60	+ * Protection Fault in the Switcher when it restores a Guest segment register
	61	+ * which tries to use that entry. Then we kill the Guest for causing such a
	62	+ * mess: the message will be "unhandled trap 256". */
105	63	static void fixup_gdt_table(struct lguest *lg, unsigned start, unsigned end)
106	64	{
107	65	unsigned int i;
...	...	@@ -111,16 +69,6 @@
111	69	* they say */
112	70	if (ignored_gdt(i))
113	71	continue;
114		-
115		- /* We could fault in switch_to_guest if they are using
116		- * a removed segment. */
117		- if (!segment_present(&lg->gdt[i])) {
118		- check_segment_use(lg, i);
119		- continue;
120		- }
121		-
122		- if (!desc_ok(&lg->gdt[i]))
123		- kill_guest(lg, "Bad GDT descriptor %i", i);
124	72
125	73	/* Segment descriptors contain a privilege level: the Guest is
126	74	* sometimes careless and leaves this as 0, even though it's
...	...	@@ -47,6 +47,7 @@
47	47	// Down here in the depths of assembler code.
48	48	#include <linux/linkage.h>
49	49	#include <asm/asm-offsets.h>
	50	+#include <asm/page.h>
50	51	#include "lg.h"
51	52
52	53	// We mark the start of the code to copy
53	54
...	...	@@ -182,13 +183,15 @@
182	183	movl $(LGUEST_DS), %eax; \
183	184	movl %eax, %ds; \
184	185	/* So where are we? Which CPU, which struct? \
185		- * The stack is our clue: our TSS sets \
186		- * It at the end of "struct lguest_pages" \
187		- * And we then pushed and pushed and pushed Guest regs: \
188		- * Now stack points atop the "struct lguest_regs". \
189		- * Subtract that offset, and we find our struct. */ \
	186	+ * The stack is our clue: our TSS starts \
	187	+ * It at the end of "struct lguest_pages". \
	188	+ * Or we may have stumbled while restoring \
	189	+ * Our Guest segment regs while in switch_to_guest, \
	190	+ * The fault pushed atop that part-unwound stack. \
	191	+ * If we round the stack down to the page start \
	192	+ * We're at the start of "struct lguest_pages". */ \
190	193	movl %esp, %eax; \
191		- subl $LGUEST_PAGES_regs, %eax; \
	194	+ andl $(~(1 << PAGE_SHIFT - 1)), %eax; \
192	195	/* Save our trap number: the switch will obscure it \
193	196	* (The Guest regs are not mapped here in the Host) \
194	197	* %ebx holds it safe for deliver_to_host */ \