Re: The 64-bit version of __access_ok is broken.

[Ralf Baechle <ralf@linux-mips.org>]

On Thu, Dec 05, 2002 at 04:28:07PM +0100, Carsten Langgaard wrote:

> I have addressed this issue before, and I do it again, because we have a
> potential kernel crash situation, if this isn't fixed.
> 
> The __access_ok macro in include/asm-mips64/uaccess.h and the check_axs
> macro in arch/mips64/kernel/unaligned.c need to be changed in order to
> work correctly, it's a copy from the 32-bit kernel. It's not good enough
> to simply check for the "sign bit" of the address.
> The area between USEG (XUSEG) and KSEG0 will in 64-bit addressing mode
> generate an address error, if accessed.
> The size of the area depend on the number of virtual addressing bits
> implemented in the CPU.
> 
> Please take a look at the patch below.
> I think Ralf had some objection the last time I send it, about the fix,
> not being efficient enough (performance vice), but I think we need to
> consider stability and functionality over performance. So until someone
> comes up with a better solution, I think we need this fix.

Standard Linux approach - a bad solution is worse than no solution.

Just to show the impact of your patch:

   text    data     bss     dec     hex filename
1817824 1079664  173664 3071152  2edcb0 vmlinux-cvs
1870752 1079664  173664 3124080  2fab70 vmlinux-carsten

So that's 52928 bytes of bloat.  __access_ok is one of those kernel
functions that are inlined so often that each extra instruction needs a
_very_ good justification.

The patch below adds 32 bytes.  It's still not the right thing though.  It's
not fixing all stuff in the assembler code.  I have a better patch but it
results in odd userspace behaviour.  Smells like a compiler problem ...

  Ralf

Index: arch/mips64/kernel/process.c
===================================================================
RCS file: /home/cvs/linux/arch/mips64/kernel/process.c,v
retrieving revision 1.18.2.9
diff -u -r1.18.2.9 process.c
--- arch/mips64/kernel/process.c	2 Dec 2002 00:24:52 -0000	1.18.2.9
+++ arch/mips64/kernel/process.c	7 Dec 2002 02:13:40 -0000
@@ -52,17 +52,19 @@
 void start_thread(struct pt_regs * regs, unsigned long pc, unsigned long sp)
 {
 	unsigned long status;
+	int compat32 = current->thread.mflags & MF_32BIT;
 
 	/* New thread looses kernel privileges. */
 	status = regs->cp0_status & ~(ST0_CU0|ST0_FR|ST0_KSU);
 	status |= KSU_USER;
-	status |= (current->thread.mflags & MF_32BIT) ? 0 : ST0_FR;
+	status |= compat32 ? 0 : ST0_FR;
 	regs->cp0_status = status;
 	current->used_math = 0;
 	loose_fpu();
 	regs->cp0_epc = pc;
 	regs->regs[29] = sp;
 	current->thread.current_ds = USER_DS;
+	current->thread.user_ds.seg = - (compat32 ? 0x80000000UL : TASK_SIZE);
 }
 
 void exit_thread(void)
Index: include/asm-mips64/processor.h
===================================================================
RCS file: /home/cvs/linux/include/asm-mips64/processor.h,v
retrieving revision 1.32.2.9
diff -u -r1.32.2.9 processor.h
--- include/asm-mips64/processor.h	4 Nov 2002 19:39:56 -0000	1.32.2.9
+++ include/asm-mips64/processor.h	7 Dec 2002 02:14:10 -0000
@@ -180,7 +180,7 @@
 #define MF_LOGADE 2			/* Log address errors to syslog */
 #define MF_32BIT  4			/* Process is in 32-bit compat mode */
 	unsigned long mflags;
-	mm_segment_t current_ds;
+	mm_segment_t current_ds, user_ds;
 	unsigned long irix_trampoline;  /* Wheee... */
 	unsigned long irix_oldctx;
 };
@@ -208,7 +208,7 @@
 	/* \
 	 * For now the default is to fix address errors \
 	 */ \
-	MF_FIXADE, { 0 }, 0, 0 \
+	MF_FIXADE, KERNEL_DS, { -TASK_SIZE }, 0, 0 \
 }
 
 #ifdef __KERNEL__
Index: include/asm-mips64/uaccess.h
===================================================================
RCS file: /home/cvs/linux/include/asm-mips64/uaccess.h,v
retrieving revision 1.13.2.1
diff -u -r1.13.2.1 uaccess.h
--- include/asm-mips64/uaccess.h	1 Jul 2002 15:27:31 -0000	1.13.2.1
+++ include/asm-mips64/uaccess.h	7 Dec 2002 02:14:10 -0000
@@ -23,7 +23,7 @@
  * For historical reasons, these macros are grossly misnamed.
  */
 #define KERNEL_DS	((mm_segment_t) { (unsigned long) 0L })
-#define USER_DS		((mm_segment_t) { (unsigned long) -1L })
+#define USER_DS		(current->thread.user_ds)
 
 #define VERIFY_READ    0
 #define VERIFY_WRITE   1
@@ -49,7 +49,7 @@
 	(__builtin_constant_p(size) && (signed long) (size) > 0 ? 0 : (size))
 
 #define __access_ok(addr,size,mask)					\
-	(((signed long)((mask)&(addr | (addr + size) | __ua_size(size)))) >= 0)
+	(((mask) & (addr | (addr + size) | __ua_size(size))) == 0)
 
 #define __access_mask ((long)(get_fs().seg))