Re: [PATCH] LDT improvements

[Peter Zijlstra]

On Fri, Dec 08, 2017 at 03:06:54PM +0100, Peter Zijlstra wrote:
> On Fri, Dec 08, 2017 at 05:20:00AM -0800, Andy Lutomirski wrote:
> > > 
> > > The error code of such an access is always 0x03. So I added a special
> > > handler, which checks whether the address is in the LDT map range and
> > > verifies that the access bit in the descriptor is 0. If that's the case it
> > > sets it and returns. If not, the thing dies. That works.
> > 
> > What if you are in kernel mode and try to return to a context with SS
> > or CS pointing to a non-accessed segment?  Or what if you try to
> > schedule to a context with fs or, worse, gs pointing to such a
> > segment?
> 
> How would that be different from setting a 'crap' GS in modify_ldt() and
> then returning from the syscall? That is something we should be able to
> deal with already, no?
> 
> Is this something ldt_gdt.c already tests? The current "Test GS" is in
> test_gdt_invalidation() which seems to suggest not.
> 
> Could we get a testcase for the exact situation you worry about? I'm not
> sure I'd trust myself to get it right, all this LDT magic is new to me.

I ended up with the below; that loads something in LDT-2, sets GS to
LDT-2 and then again loads that same thing in LDT-2.

AFAIU calling modify_ldt will clear that ACCESSED bit, so this would
then trigger that on the return to user from modify_ldt, no?

Seems to work with tglx's patches.


diff --git a/tools/testing/selftests/x86/ldt_gdt.c b/tools/testing/selftests/x86/ldt_gdt.c
index 66e5ce5b91f0..d46a620c3734 100644
--- a/tools/testing/selftests/x86/ldt_gdt.c
+++ b/tools/testing/selftests/x86/ldt_gdt.c
@@ -242,6 +242,36 @@ static void fail_install(struct user_desc *desc)
 	}
 }
 
+static void do_ldt_gs_test(void)
+{
+	unsigned short prev_sel, sel = (2 << 3) | (1 << 2) | 3;
+
+	low_user_desc->entry_number = 2;
+
+	safe_modify_ldt(1, low_user_desc, sizeof(*low_user_desc));
+
+	/*
+	 * syscall (eax) 123 - modify_ldt
+	 *         (ebx)     - func
+	 *         (ecx)     - ptr
+	 *         (edx)     - bytecount
+	 */
+
+	int eax = 123;
+	int ebx = 1;
+	int ecx = (unsigned int)low_user_desc;
+	int edx = sizeof(struct user_desc);
+
+	asm volatile ("movw %%gs, %[prev_sel]\n\t"
+		      "movw %[sel], %%gs\n\t"
+		      "int $0x80\n\t"
+		      "mov %[prev_sel], %%gs"
+		      : [prev_sel] "=&R" (prev_sel), [sel] "+R" (sel), "+a" (eax)
+		      : "b" (ebx), "c" (ecx), "d" (edx)
+		      : INT80_CLOBBERS);
+
+}
+
 static void do_simple_tests(void)
 {
 	struct user_desc desc = {
@@ -919,6 +946,8 @@ int main(int argc, char **argv)
 	setup_counter_page();
 	setup_low_user_desc();
 
+	do_ldt_gs_test();
+
 	do_simple_tests();
 
 	do_multicpu_tests();