Re: [PATCH 3/6] i386 virtualization - Make ldt a desc struct

Re: [PATCH 3/6] i386 virtualization - Make ldt a desc struct

[Chuck Ebbert]

On Mon, 15 Aug 2005 at 15:59:39 -0700, zach@vmware.com wrote:

> --- linux-2.6.13.orig/include/asm-i386/mmu_context.h  2005-08-15 11:16:59.000000000 -0700
> +++ linux-2.6.13/include/asm-i386/mmu_context.h       2005-08-15 11:19:49.000000000 -0700
> @@ -19,7 +19,7 @@
>       memset(&mm->context, 0, sizeof(mm->context));
>       init_MUTEX(&mm->context.sem);
>       old_mm = current->mm;
> -     if (old_mm && unlikely(old_mm->context.size > 0)) {
> +     if (old_mm && unlikely(old_mm->context.ldt)) {  <==================
>               retval = copy_ldt(&mm->context, &old_mm->context);
>       }
>       if (retval == 0)
> @@ -32,7 +32,7 @@
>   */
>  static inline void destroy_context(struct mm_struct *mm)
>  {
> -     if (unlikely(mm->context.size))
> +     if (unlikely(mm->context.ldt))    <==================
>               destroy_ldt(mm);
>       del_lazy_mm(mm);
>  }

  Here you changed the code so it no longer tests the size field, however:

> --- linux-2.6.13.orig/arch/i386/kernel/ldt.c  2005-08-15 11:16:59.000000000 -0700
> +++ linux-2.6.13/arch/i386/kernel/ldt.c       2005-08-15 11:19:49.000000000 -0700
<--SNIP-->
> @@ -97,14 +96,16 @@
>  
>  void destroy_ldt(struct mm_struct *mm)
>  {
> +     int pages = mm->context.ldt_pages;
> +
>       if (mm == current->active_mm)
>               clear_LDT();
> -     ClearPagesLDT(mm->context.ldt, (mm->context.size * LDT_ENTRY_SIZE) / PAGE_SIZE);
> -     if (mm->context.size*LDT_ENTRY_SIZE > PAGE_SIZE)
> +     ClearPagesLDT(mm->context.ldt, pages);
> +     if (pages > 1)
>               vfree(mm->context.ldt);
>       else
>               kfree(mm->context.ldt);
> -     mm->context.size = 0;
> +     mm->context.ldt_pages = 0;   <====================
>  }
>  
>  static int read_ldt(void __user * ptr, unsigned long bytecount)

  destroy_ldt does not zero "ldt", just the size.  Potential bug?

  Also:

> --- linux-2.6.13.orig/arch/i386/kernel/ldt.c  2005-08-15 11:16:59.000000000 -0700
> +++ linux-2.6.13/arch/i386/kernel/ldt.c       2005-08-15 11:19:49.000000000 -0700
> @@ -28,28 +28,27 @@
>  }
>  #endif
>  
> -static inline int alloc_ldt(mm_context_t *pc, const int oldsize, int mincount, const int reload)
> +static inline int alloc_ldt(mm_context_t *pc, const int old_pages, int new_pages, const int reload)
>  {
> -     void *oldldt;
> -     void *newldt;
> +     struct desc_struct *oldldt;
> +     struct desc_struct *newldt;

  Can't this be declared on one line?


  ...and BTW could you add:

        QUILT_DIFF_OPTS=-p

to your shell env?  It makes the patches much easier to review.

__
Chuck

Re: [PATCH 3/6] i386 virtualization - Make ldt a desc struct

[Zachary Amsden]

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

Chuck Ebbert wrote:

>
>>@@ -97,14 +96,16 @@
>> 
>> void destroy_ldt(struct mm_struct *mm)
>> {
>>+     int pages = mm->context.ldt_pages;
>>+
>>      if (mm == current->active_mm)
>>              clear_LDT();
>>-     ClearPagesLDT(mm->context.ldt, (mm->context.size * LDT_ENTRY_SIZE) / PAGE_SIZE);
>>-     if (mm->context.size*LDT_ENTRY_SIZE > PAGE_SIZE)
>>+     ClearPagesLDT(mm->context.ldt, pages);
>>+     if (pages > 1)
>>              vfree(mm->context.ldt);
>>      else
>>              kfree(mm->context.ldt);
>>-     mm->context.size = 0;
>>+     mm->context.ldt_pages = 0;   <====================
>> }
>> 
>> static int read_ldt(void __user * ptr, unsigned long bytecount)
>>    
>>
>
>  destroy_ldt does not zero "ldt", just the size.  Potential bug?
>  
>

Not a bug, truly unnecessary at all.


[-- Attachment #2: remove-useless-zeroing --]
[-- Type: text/plain, Size: 1231 bytes --]

Several reviewers noticed that initialization and destruction of the
mm->context is unnecessary, since the entire MM struct is zeroed on
allocation anyways.

Verified with BUG_ON(mm->context.ldt || mm->context.ldt_pages);

Signed-off-by: Zachary Amsden <zach@vmware.com>
Index: linux-2.6.13/include/asm-i386/mmu_context.h
===================================================================
--- linux-2.6.13.orig/include/asm-i386/mmu_context.h	2005-08-15 11:23:32.000000000 -0700
+++ linux-2.6.13/include/asm-i386/mmu_context.h	2005-08-16 11:35:11.000000000 -0700
@@ -16,7 +16,6 @@
 	struct mm_struct * old_mm;
 	int retval = 0;
 
-	memset(&mm->context, 0, sizeof(mm->context));
 	init_MUTEX(&mm->context.sem);
 	old_mm = current->mm;
 	if (old_mm && unlikely(old_mm->context.ldt)) {
Index: linux-2.6.13/arch/i386/kernel/ldt.c
===================================================================
--- linux-2.6.13.orig/arch/i386/kernel/ldt.c	2005-08-15 11:23:32.000000000 -0700
+++ linux-2.6.13/arch/i386/kernel/ldt.c	2005-08-16 11:12:59.000000000 -0700
@@ -105,7 +105,6 @@
 		vfree(mm->context.ldt);
 	else
 		kfree(mm->context.ldt);
-	mm->context.ldt_pages = 0;
 }
 
 static int read_ldt(void __user * ptr, unsigned long bytecount)

[PATCH] i386 / desc_empty macro is incorrect

[Chris Wright]

From: Zachary Amsden <zach@vmware.com>

Chuck Ebbert wrote:
>     I think that should be "|" instead of "+".

I think so too.  I merely moved the code here and didn't notice it in 
all this excitement.

0x00cf9a000xff306600  =>

Present CPL-0 32-bit code segment, base 0x0000ff30, limit 0xf6601 pages, 
for which desc_empty(desc) is true.

Thankfully, this is not used as a security check, but it can falsely 
overwrite TLS segments with carefully chosen base / limits.  I do not 
believe this is an issue in practice, but it is a kernel bug.

Nice catch.  Looks like it affects all 2.6.X kernels.

Chuck Ebbert noticed that the desc_empty macro is incorrect.  Fix it.

Signed-off-by: Zachary Amsden <zach@vmware.com>
Signed-off-by: Chris Wright <chrisw@osdl.org>
---

diff --git a/include/asm-i386/processor.h b/include/asm-i386/processor.h
--- a/include/asm-i386/processor.h
+++ b/include/asm-i386/processor.h
@@ -29,7 +29,7 @@ struct desc_struct {
 };
 
 #define desc_empty(desc) \
-		(!((desc)->a + (desc)->b))
+		(!((desc)->a | (desc)->b))
 
 #define desc_equal(desc1, desc2) \
 		(((desc1)->a == (desc2)->a) && ((desc1)->b == (desc2)->b))

Re: [PATCH] i386 / desc_empty macro is incorrect

[Linus Torvalds]

On Tue, 16 Aug 2005, Chris Wright wrote:
> 
> Chuck Ebbert noticed that the desc_empty macro is incorrect.  Fix it.

x86-64 had the same thing. I added that to the fix as obvious.

		Linus

Re: [PATCH 3/6] i386 virtualization - Make ldt a desc struct

[Chris Wright]

* Zachary Amsden (zach@vmware.com) wrote:
> Several reviewers noticed that initialization and destruction of the
> mm->context is unnecessary, since the entire MM struct is zeroed on
> allocation anyways.

well, on fork it should be just shallow copied rather than zeroed.

Re: [PATCH 3/6] i386 virtualization - Make ldt a desc struct

[Zachary Amsden]

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

Chris Wright wrote:

>* Zachary Amsden (zach@vmware.com) wrote:
>  
>
>>Several reviewers noticed that initialization and destruction of the
>>mm->context is unnecessary, since the entire MM struct is zeroed on
>>allocation anyways.
>>    
>>
>
>well, on fork it should be just shallow copied rather than zeroed.
>  
>

Right you are.  That turned out to be a really bad idea (TM).  Updated 
my ldt test and got the expected panic with the BUG_ON left in.

Zach

[-- Attachment #2: bobo.gif --]
[-- Type: image/gif, Size: 9944 bytes --]

[-- Attachment #3: ldt.c --]
[-- Type: text/plain, Size: 3663 bytes --]

/*
 * Copyright (c) 2005, Zachary Amsden (zach@vmware.com)
 * This is licensed under the GPL.
 */

#include <stdio.h>
#include <signal.h>
#include <asm/ldt.h>
#include <asm/segment.h>
#include <sys/types.h>
#include <unistd.h>
#include <sys/mman.h>
#include <sched.h>
#define __KERNEL__
#include <asm/page.h>

/*
 * Spin modifying LDT entry 1 to get contention on the mm->context
 * semaphore.
 */
void evil_child(void *addr)
{
	struct user_desc desc;

	while (1) {
		desc.entry_number = 1;
		desc.base_addr = addr;
		desc.limit = 1;
		desc.seg_32bit = 1;
		desc.contents = MODIFY_LDT_CONTENTS_CODE;
		desc.read_exec_only = 0;
		desc.limit_in_pages = 1;
		desc.seg_not_present = 0;
		desc.useable = 1;
		if (modify_ldt(1, &desc, sizeof(desc)) != 0) {
			perror("modify_ldt");
			abort();
		}
	}
	exit(0);
}

void catch_sig(int signo, struct sigcontext ctx)
{
	return;
}

void main(void)
{
        struct user_desc desc;
        char *code;
        unsigned long long tsc;
	char *stack;
	pid_t child; 
	int i;
	unsigned long long lasttsc = 0;

        code = (char *)mmap(0, 8192, PROT_EXEC|PROT_READ|PROT_WRITE,
                                 MAP_PRIVATE | MAP_ANONYMOUS, -1, 0);

	/* Test 1 - CODE, 32-BIT, 2 page limit */
        desc.entry_number = 0;
        desc.base_addr = code;
        desc.limit = 1;
        desc.seg_32bit = 1;
        desc.contents = MODIFY_LDT_CONTENTS_CODE;
        desc.read_exec_only = 0;
        desc.limit_in_pages = 1;
        desc.seg_not_present = 0;
        desc.useable = 1;
        if (modify_ldt(1, &desc, sizeof(desc)) != 0) {
                perror("modify_ldt");
		abort();
        }
        printf("INFO: code base is 0x%08x\n", (unsigned)code);
        code[0x0ffe] = 0x0f;  /* rdtsc */
        code[0x0fff] = 0x31;
        code[0x1000] = 0xcb;  /* lret */
        __asm__ __volatile("lcall $7,$0xffe" : "=A" (tsc));
        printf("INFO: TSC is 0x%016llx\n", tsc);

	/*
	 * Fork an evil child that shares the same MM context
	 */
	stack = malloc(8192);
	child = clone(evil_child, stack, CLONE_VM, 0xb0b0);
	if (child == -1) {
		perror("clone");
		abort();
	}

	/* Test 2 - CODE, 32-BIT, 4097 byte limit */
        desc.entry_number = 512;
        desc.base_addr = code;
        desc.limit = 4096;
        desc.seg_32bit = 1;
        desc.contents = MODIFY_LDT_CONTENTS_CODE;
        desc.read_exec_only = 0;
        desc.limit_in_pages = 0;
        desc.seg_not_present = 0;
        desc.useable = 1;
        if (modify_ldt(1, &desc, sizeof(desc)) != 0) {
                perror("modify_ldt");
		abort();
        }
        code[0x0ffe] = 0x0f;  /* rdtsc */
        code[0x0fff] = 0x31;
        code[0x1000] = 0xcb;  /* lret */
        __asm__ __volatile("lcall $0x1007,$0xffe" : "=A" (tsc));

	/*
	 * Test 3 - CODE, 32-BIT, maximal LDT.  Race against evil
	 * child while taking debug traps on LDT CS.
	 */
	for (i = 0; i < 1000; i++) {
		signal(SIGTRAP, catch_sig);
		desc.entry_number = 8191;
		desc.base_addr = code;
		desc.limit = 4097;
		desc.seg_32bit = 1;
		desc.contents = MODIFY_LDT_CONTENTS_CODE;
		desc.read_exec_only = 0;
		desc.limit_in_pages = 0;
		desc.seg_not_present = 0;
		desc.useable = 1;
		if (modify_ldt(1, &desc, sizeof(desc)) != 0) {
			perror("modify_ldt");
			abort();
		}
		code[0x0ffe] = 0x0f;  /* rdtsc */
		code[0x0fff] = 0x31;
		code[0x1000] = 0xcc;  /* int3 */
        	code[0x1001] = 0xcb;  /* lret */
		__asm__ __volatile("lcall $0xffff,$0xffe" : "=A" (tsc));
		if (tsc < lasttsc) {
			printf("WARNING: TSC went backwards\n");
		}
		lasttsc = tsc;
	}
	if (kill(child, SIGTERM) != 0) {
		perror("kill");
		abort();
	}
	if (fork() == 0) {
		printf("PASS: LDT code segment\n");
	}
}