Re: [PATCH 7/14] i386 / Add some descriptor convenience functions

Re: [PATCH 7/14] i386 / Add some descriptor convenience functions

[Chuck Ebbert]

On Wed, 10 Aug 2005 at 21:56:20 -0700, zach@vmware.com wrote:

> Patch-base: 2.6.13-rc5-mm1
> Patch-keys: i386 desc cleanup
> Signed-off-by: Zachary Amsden <zach@vmware.com>
> Index: linux-2.6.13/include/asm-i386/desc.h
> ===================================================================
> --- linux-2.6.13.orig/include/asm-i386/desc.h 2005-08-09 19:43:38.000000000 -0700
> +++ linux-2.6.13/include/asm-i386/desc.h      2005-08-10 20:42:03.000000000 -0700
> @@ -14,6 +14,28 @@
>  
>  #include <asm/mmu.h>
>  
> +#define desc_empty(desc) \
> +             (!((desc)->a + (desc)->b))
> +

     I think that should be "|" instead of "+".

__
Chuck

Re: [PATCH 7/14] i386 / Add some descriptor convenience functions

[Zachary Amsden]

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

Chuck Ebbert wrote:

>On Wed, 10 Aug 2005 at 21:56:20 -0700, zach@vmware.com wrote:
>
>  
>
>>Patch-base: 2.6.13-rc5-mm1
>>Patch-keys: i386 desc cleanup
>>Signed-off-by: Zachary Amsden <zach@vmware.com>
>>Index: linux-2.6.13/include/asm-i386/desc.h
>>===================================================================
>>--- linux-2.6.13.orig/include/asm-i386/desc.h 2005-08-09 19:43:38.000000000 -0700
>>+++ linux-2.6.13/include/asm-i386/desc.h      2005-08-10 20:42:03.000000000 -0700
>>@@ -14,6 +14,28 @@
>> 
>> #include <asm/mmu.h>
>> 
>>+#define desc_empty(desc) \
>>+             (!((desc)->a + (desc)->b))
>>+
>>    
>>
>
>     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.

Zach

[-- Attachment #2: fix-desc-empty --]
[-- Type: text/plain, Size: 620 bytes --]

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

Signed-off-by: Zachary Amsden <zach@vmware.com>
Index: linux-2.6.13/include/asm-i386/desc.h
===================================================================
--- linux-2.6.13.orig/include/asm-i386/desc.h	2005-08-15 11:23:32.000000000 -0700
+++ linux-2.6.13/include/asm-i386/desc.h	2005-08-16 10:49:03.000000000 -0700
@@ -18,7 +18,7 @@
 #include <asm/mmu.h>
 
 #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 7/14] i386 / Add some descriptor convenience functions

[Andi Kleen]

x86-64 is buggy too. I fixed it in my tree.
-Andi

Re: [PATCH 7/14] i386 / Add some descriptor convenience functions

[Chris Wright]

* Zachary Amsden (zach@vmware.com) wrote:
> 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.

Thanks, I added this to virt tree, and will push baseline fix up.

thanks,
-chris

[PATCH 7/14] i386 / Add some descriptor convenience functions

[zach]

Add some convenient descriptor access functions and move them all into desc.h

Patch-base: 2.6.13-rc5-mm1
Patch-keys: i386 desc cleanup
Signed-off-by: Zachary Amsden <zach@vmware.com>
Index: linux-2.6.13/include/asm-i386/desc.h
===================================================================
--- linux-2.6.13.orig/include/asm-i386/desc.h	2005-08-09 19:43:38.000000000 -0700
+++ linux-2.6.13/include/asm-i386/desc.h	2005-08-10 20:42:03.000000000 -0700
@@ -14,6 +14,28 @@
 
 #include <asm/mmu.h>
 
+#define desc_empty(desc) \
+		(!((desc)->a + (desc)->b))
+
+#define desc_equal(desc1, desc2) \
+		(((desc1)->a == (desc2)->a) && ((desc1)->b == (desc2)->b))
+
+static inline unsigned long get_desc_base(struct desc_struct *desc)
+{
+	unsigned long base;
+	base = ((desc->a >> 16)  & 0x0000ffff) |
+		((desc->b << 16) & 0x00ff0000) |
+		(desc->b & 0xff000000);
+	return base;
+}
+
+static inline unsigned long get_desc_limit(struct desc_struct *desc)
+{
+	unsigned long limit;
+	limit = (desc->a & 0x0ffff) | (desc->b & 0xf0000);
+	return limit;
+}
+
 extern struct desc_struct cpu_gdt_table[GDT_ENTRIES];
 DECLARE_PER_CPU(struct desc_struct, cpu_gdt_table[GDT_ENTRIES]);
 
@@ -111,15 +133,5 @@
 	put_cpu();
 }
 
-static inline unsigned long get_desc_base(unsigned long *desc)
-{
-	unsigned long base;
-	base = ((desc[0] >> 16)  & 0x0000ffff) |
-		((desc[1] << 16) & 0x00ff0000) |
-		(desc[1] & 0xff000000);
-	return base;
-}
-
 #endif /* !__ASSEMBLY__ */
-
 #endif
Index: linux-2.6.13/include/asm-i386/processor.h
===================================================================
--- linux-2.6.13.orig/include/asm-i386/processor.h	2005-08-09 19:43:38.000000000 -0700
+++ linux-2.6.13/include/asm-i386/processor.h	2005-08-09 19:43:59.000000000 -0700
@@ -28,11 +28,6 @@
 	unsigned long a,b;
 };
 
-#define desc_empty(desc) \
-		(!((desc)->a + (desc)->b))
-
-#define desc_equal(desc1, desc2) \
-		(((desc1)->a == (desc2)->a) && ((desc1)->b == (desc2)->b))
 /*
  * Default implementation of macro that returns current
  * instruction pointer ("program counter").
Index: linux-2.6.13/arch/i386/mm/fault.c
===================================================================
--- linux-2.6.13.orig/arch/i386/mm/fault.c	2005-08-09 19:43:47.000000000 -0700
+++ linux-2.6.13/arch/i386/mm/fault.c	2005-08-10 20:42:04.000000000 -0700
@@ -75,7 +75,8 @@
 {
 	unsigned long eip = regs->eip;
 	unsigned seg = regs->xcs & 0xffff;
-	u32 seg_ar, seg_limit, base, *desc;
+	u32 seg_ar, seg_limit, base;
+	struct desc_struct *desc;
 
 	/* The standard kernel/user address space limit. */
 	*eip_limit = (seg & 3) ? USER_DS.seg : KERNEL_DS.seg;
@@ -108,12 +109,12 @@
 		desc = (void *)desc + (seg & ~7);
 	} else {
 		/* Must disable preemption while reading the GDT. */
-		desc = (u32 *)&per_cpu(cpu_gdt_table, get_cpu());
+		desc = per_cpu(cpu_gdt_table, get_cpu());
 		desc = (void *)desc + (seg & ~7);
 	}
 
 	/* Decode the code segment base from the descriptor */
-	base = get_desc_base((unsigned long *)desc);
+	base = get_desc_base(desc);
 
 	if (segment_from_ldt(seg)) { 
 		up(&current->mm->context.sem);
Index: linux-2.6.13/arch/i386/kernel/kprobes.c
===================================================================
--- linux-2.6.13.orig/arch/i386/kernel/kprobes.c	2005-08-09 19:43:47.000000000 -0700
+++ linux-2.6.13/arch/i386/kernel/kprobes.c	2005-08-09 19:54:16.000000000 -0700
@@ -156,7 +156,6 @@
 	struct kprobe *p;
 	int ret = 0;
 	kprobe_opcode_t *addr = NULL;
-	unsigned long *lp;
 
 	/* We're in an interrupt, but this is clear and BUG()-safe. */
 	preempt_disable();
@@ -164,9 +163,10 @@
 	 * calculate the address by reading the base address from the LDT entry.
 	 */
 	if (segment_from_ldt(regs->xcs) && (current->mm)) {
-		lp = (unsigned long *) ((unsigned long)(segment_index(regs->xcs) * 8)
-					+ (char *) current->mm->context.ldt);
-		addr = (kprobe_opcode_t *) (get_desc_base(lp) + regs->eip -
+		struct desc_struct *desc;
+		desc = (struct desc_struct *) ((char *) current->mm->context.ldt +
+						 (segment_index(regs->xcs) * 8));
+		addr = (kprobe_opcode_t *) (get_desc_base(desc) + regs->eip -
 						sizeof(kprobe_opcode_t));
 	} else {
 		addr = (kprobe_opcode_t *)(regs->eip - sizeof(kprobe_opcode_t));