The 64-bit version of __access_ok is broken.

The 64-bit version of __access_ok is broken.

[Carsten Langgaard]

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

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.

/Carsten




--
_    _ ____  ___   Carsten Langgaard   Mailto:carstenl@mips.com
|\  /|||___)(___   MIPS Denmark        Direct: +45 4486 5527
| \/ |||    ____)  Lautrupvang 4B      Switch: +45 4486 5555
  TECHNOLOGIES     2750 Ballerup       Fax...: +45 4486 5556
                   Denmark             http://www.mips.com



[-- Attachment #2: access_ok.patch --]
[-- Type: text/plain, Size: 2305 bytes --]

Index: arch/mips64/kernel/unaligned.c
===================================================================
RCS file: /home/cvs/linux/arch/mips64/kernel/unaligned.c,v
retrieving revision 1.6.2.7
diff -u -r1.6.2.7 unaligned.c
--- arch/mips64/kernel/unaligned.c	5 Dec 2002 03:09:58 -0000	1.6.2.7
+++ arch/mips64/kernel/unaligned.c	5 Dec 2002 15:06:59 -0000
@@ -89,11 +89,14 @@
 #define __STR(x)  #x
 
 /*
- * User code may only access USEG; kernel code may access the
- * entire address space.
+ * User code may only access USEG; 
+ * Kernel code may access the entire address space, except the area between
+ * USEG (XUSEG) and KSEG0.
  */
-#define check_axs(pc,a,s)				\
-	if ((long)(~(pc) & ((a) | ((a)+(s)))) < 0)	\
+#define check_axs(pc,a,s)						\
+        if (((pc < KUSIZE) && (((a) | ((a)+(s))) >= KUSIZE)) ||		\
+	    ((((a) | ((a)+(s))) < K0BASE) &&				\
+	     (((a) | ((a)+(s))) >= KUSIZE)))				\
 		goto sigbus;
 
 static inline int emulate_load_store_insn(struct pt_regs *regs,
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	5 Dec 2002 15:07:11 -0000
@@ -40,16 +40,23 @@
  * than tests.
  *
  * Address valid if:
- *  - "addr" doesn't have any high-bits set
- *  - AND "size" doesn't have any high-bits set
- *  - AND "addr+size" doesn't have any high-bits set
- *  - OR we are in kernel mode.
+ *  - In user mode and "addr" and "addr+size" in USEG (or XUSEG).
+ *  - OR we are in kernel mode and "addr" and "addr+size" isn't in the 
+ *    area between USEG (XUSEG) and KSEG0.
  */
 #define __ua_size(size)							\
 	(__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)
+static inline int 
+__access_ok(unsigned long addr, unsigned long size, long mask)
+{
+	if (((mask) && ((addr | (addr+size)) >= KUSIZE)) ||     
+	    (((addr | (addr+size)) < K0BASE) &&
+	     ((addr | (addr+size)) >= KUSIZE)))
+		return 0;
+	else
+		return 1;
+}
 
 #define __access_mask ((long)(get_fs().seg))

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

[Ralf Baechle]

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))
 

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

[Carsten Langgaard]

Ralf Baechle wrote:

> 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.

I really disagree.
My solution may impact the size of the kernel (the increase is less than 2
percent), but it prevent kernel crashes.
Sure we may be able to find a better solution, but it has been almost 1/2 year ago,
the last time I addressed this problem.
I know at least one other on the list was hit by this problem.


>
> 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 ...

I tried you patch below, but then nothing seems to work.


>
>   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))
>

--
_    _ ____  ___   Carsten Langgaard   Mailto:carstenl@mips.com
|\  /|||___)(___   MIPS Denmark        Direct: +45 4486 5527
| \/ |||    ____)  Lautrupvang 4B      Switch: +45 4486 5555
  TECHNOLOGIES     2750 Ballerup       Fax...: +45 4486 5556
                   Denmark             http://www.mips.com

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

[Dominic Sweetman]

> > > The __access_ok macro in include/asm-mips64/uaccess.h and the
> > > check_axs macro in arch/mips64/kernel/unaligned.c ... is a copy
> > > from the 32-bit kernel...
> > >
> > > The area between USEG (XUSEG) and KSEG0 will in 64-bit
> > > addressing mode generate an address error, if accessed.

I'd like to be clear about the consequences of this.  Presumably the
'access_ok()' macro is used to check addresses which were (originally)
provided by a user program's system call.

Carsten, are you saying that if such an address is set to say 2**41 in
a CPU supporting 40-bit user virtual addresses, that the kernel will
crash?

If so, that seems to require a fix, even if we don't know a very
efficient one.  But perhaps any problem is a bit more subtle than
that?

-- 
Dominic Sweetman
MIPS Technologies
The Fruit Farm, Ely Road, Chittering, CAMBS CB5 9PH, ENGLAND
phone +44 1223 706205/fax +44 1223 706250/swbrd +44 1223 706200
http://www.algor.co.uk

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

[Carsten Langgaard]

Dominic Sweetman wrote:

> > > > The __access_ok macro in include/asm-mips64/uaccess.h and the
> > > > check_axs macro in arch/mips64/kernel/unaligned.c ... is a copy
> > > > from the 32-bit kernel...
> > > >
> > > > The area between USEG (XUSEG) and KSEG0 will in 64-bit
> > > > addressing mode generate an address error, if accessed.
>
> I'd like to be clear about the consequences of this.  Presumably the
> 'access_ok()' macro is used to check addresses which were (originally)
> provided by a user program's system call.
>
> Carsten, are you saying that if such an address is set to say 2**41 in
> a CPU supporting 40-bit user virtual addresses, that the kernel will
> crash?

Yes, that's the case.
It's been a while since I fixed it locally, but if I ran something like
crashme, I could end up, in a situation where the kernel tries (on the
behalf of the user) to access an address like 2**41 in a CPU supporting
40-bit user virtual addresses, which generate an address error and
because we are in kernel mode we die.


>
> If so, that seems to require a fix, even if we don't know a very
> efficient one.  But perhaps any problem is a bit more subtle than
> that?
>
> --
> Dominic Sweetman
> MIPS Technologies
> The Fruit Farm, Ely Road, Chittering, CAMBS CB5 9PH, ENGLAND
> phone +44 1223 706205/fax +44 1223 706250/swbrd +44 1223 706200
> http://www.algor.co.uk

--
_    _ ____  ___   Carsten Langgaard   Mailto:carstenl@mips.com
|\  /|||___)(___   MIPS Denmark        Direct: +45 4486 5527
| \/ |||    ____)  Lautrupvang 4B      Switch: +45 4486 5555
  TECHNOLOGIES     2750 Ballerup       Fax...: +45 4486 5556
                   Denmark             http://www.mips.com

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

[Ralf Baechle]

On Mon, Dec 09, 2002 at 10:30:03AM +0100, Carsten Langgaard wrote:

> > 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 ...
> 
> I tried you patch below, but then nothing seems to work.

The reason for this problem (and a few others is the broken call to
__access_ok() in clear_user().  That should actually be access_ok().
Basically the kernel was only working so far because addresses were just
right ...

Below my working version.  I still needs to make TASK_SIZE variable but
with the clear_user thing fixed that should be easy.

  Ralf

Index: arch/mips64/kernel/scall_o32.S
===================================================================
RCS file: /home/cvs/linux/arch/mips64/kernel/scall_o32.S,v
retrieving revision 1.48.2.21
diff -u -r1.48.2.21 scall_o32.S
--- arch/mips64/kernel/scall_o32.S	3 Dec 2002 14:23:05 -0000	1.48.2.21
+++ arch/mips64/kernel/scall_o32.S	8 Dec 2002 06:08:55 -0000
@@ -209,7 +209,7 @@
 	daddiu	a0, a1, 4
 	or	a0, a0, a1
 	and	a0, a0, v1
-	bltz	a0, bad_address
+	bnez	a0, bad_address
 
 	/* Ok, this is the ll/sc case.  World is sane :-)  */
 1:	ll	v0, (a1)
@@ -273,7 +273,7 @@
 	ld	v1, THREAD_CURDS($28)
 	or	v0, v0, t1
 	and	v1, v1, v0
-	bltz	v1, efault
+	bnez	v1, efault
 
 	move	a0, a1			# shift argument registers
 	move	a1, a2
Index: arch/mips64/lib/strlen_user.S
===================================================================
RCS file: /home/cvs/linux/arch/mips64/lib/strlen_user.S,v
retrieving revision 1.4.2.1
diff -u -r1.4.2.1 strlen_user.S
--- arch/mips64/lib/strlen_user.S	1 Jul 2002 15:27:29 -0000	1.4.2.1
+++ arch/mips64/lib/strlen_user.S	8 Dec 2002 06:08:55 -0000
@@ -25,7 +25,7 @@
 LEAF(__strlen_user_asm)
 	ld	v0, THREAD_CURDS($28)			# pointer ok?
 	and	v0, a0
-	bltz	v0, fault
+	bnez	v0, fault
 
 FEXPORT(__strlen_user_nocheck_asm)
 	move	v0, a0
Index: arch/mips64/lib/strncpy_user.S
===================================================================
RCS file: /home/cvs/linux/arch/mips64/lib/strncpy_user.S,v
retrieving revision 1.4
diff -u -r1.4 strncpy_user.S
--- arch/mips64/lib/strncpy_user.S	9 Jul 2001 00:25:37 -0000	1.4
+++ arch/mips64/lib/strncpy_user.S	8 Dec 2002 06:08:55 -0000
@@ -30,7 +30,7 @@
 LEAF(__strncpy_from_user_asm)
 	ld	v0, THREAD_CURDS($28)		# pointer ok?
 	and	v0, a1
-	bltz	v0, fault
+	bnez	v0, fault
 
 FEXPORT(__strncpy_from_user_nocheck_asm)
 	move	v0, zero
Index: arch/mips64/lib/strnlen_user.S
===================================================================
RCS file: /home/cvs/linux/arch/mips64/lib/strnlen_user.S,v
retrieving revision 1.2.2.2
diff -u -r1.2.2.2 strnlen_user.S
--- arch/mips64/lib/strnlen_user.S	1 Jul 2002 15:27:29 -0000	1.2.2.2
+++ arch/mips64/lib/strnlen_user.S	8 Dec 2002 06:08:55 -0000
@@ -25,7 +25,7 @@
 LEAF(__strnlen_user_asm)
 	ld	v0, THREAD_CURDS($28)	# pointer ok?
 	and	v0, a0
-	bltz	v0, fault
+	bnez	v0, fault
 
 FEXPORT(__strnlen_user_nocheck_asm)
 	move	v0, a0
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	8 Dec 2002 06:09:38 -0000
@@ -208,7 +208,7 @@
 	/* \
 	 * For now the default is to fix address errors \
 	 */ \
-	MF_FIXADE, { 0 }, 0, 0 \
+	MF_FIXADE, KERNEL_DS, 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	8 Dec 2002 06:09:39 -0000
@@ -22,8 +22,8 @@
  *
  * 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 KERNEL_DS	((mm_segment_t) { 0UL })
+#define USER_DS		((mm_segment_t) { -TASK_SIZE })
 
 #define VERIFY_READ    0
 #define VERIFY_WRITE   1
@@ -46,19 +46,19 @@
  *  - OR we are in kernel mode.
  */
 #define __ua_size(size)							\
-	(__builtin_constant_p(size) && (signed long) (size) > 0 ? 0 : (size))
+	((__builtin_constant_p(size) && (size)) > 0 ? 0 : (size))
 
-#define __access_ok(addr,size,mask)					\
-	(((signed long)((mask)&(addr | (addr + size) | __ua_size(size)))) >= 0)
+#define __access_ok(addr, size, mask)					\
+	(((mask) & ((addr) | ((addr) + (size)) | __ua_size(size))) == 0)
 
-#define __access_mask ((long)(get_fs().seg))
+#define __access_mask get_fs().seg
 
-#define access_ok(type,addr,size) \
-	__access_ok(((unsigned long)(addr)),(size),__access_mask)
+#define access_ok(type, addr, size)					\
+	__access_ok((unsigned long)(addr), (size), __access_mask)
 
 static inline int verify_area(int type, const void * addr, unsigned long size)
 {
-	return access_ok(type,addr,size) ? 0 : -EFAULT;
+	return access_ok(type, addr, size) ? 0 : -EFAULT;
 }
 
 /*
@@ -340,8 +340,8 @@
 ({								\
 	void * __cl_addr = (addr);				\
 	unsigned long __cl_size = (n);				\
-	if (__cl_size && __access_ok(VERIFY_WRITE,		\
-	       ((unsigned long)(__cl_addr)), __cl_size))	\
+	if (__cl_size && access_ok(VERIFY_WRITE,		\
+		((unsigned long)(__cl_addr)), __cl_size))	\
 		__cl_size = __clear_user(__cl_addr, __cl_size);	\
 	__cl_size;						\
 })

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

[Ralf Baechle]

On Mon, Dec 09, 2002 at 11:54:20AM +0000, Dominic Sweetman wrote:

> I'd like to be clear about the consequences of this.  Presumably the
> 'access_ok()' macro is used to check addresses which were (originally)
> provided by a user program's system call.
> 
> Carsten, are you saying that if such an address is set to say 2**41 in
> a CPU supporting 40-bit user virtual addresses, that the kernel will
> crash?

That's correct.  The problem which Carsten diagnosed correctly was the
assumption which has been inherited from the 32-bit kernel that the sign-
bit makes the difference between valid userspace and kernelspace
addresses.

Linux doesn't use the supervisor mode so basically that assumption is still
true with the except of the area 2^PHYSBITS ... 2^63-1.

> If so, that seems to require a fix, even if we don't know a very
> efficient one.  But perhaps any problem is a bit more subtle than
> that?

Access_ok is a macro which depending on kernel configuration is expanded
hundreds, if not thousands of times throughout the kernel.  So every single
machine instruction in access_ok will make a size difference of several
kB.  Carsten's patch was performing pretty badly in that cathegory.  If
access_ok wasn't used that often the issue certainly wasn't worth the fuzz.

Access_ok is of course only usable in C code.  We also have a few piece of
assembler code that access userspace and need to perform the same kind of
address validation tests.  Carsten's patch was missing these completly.  As
such it did only reduce the window of this bug from huge to "just" big.

An efficient solution only requires fairly minor changes as you can see in
the patch I just posted.  It doesn't even require thinking, it can be
obtained by cut'n'paste from the Alpha code.  Alternatively the problem
could also have been solved by forwarding address errors for the address
range in question to the page fault handler which would have served the
same purpose, maybe even a tad more efficient but ofuscated ...

  Ralf

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

[Carsten Langgaard]

Ralf Baechle wrote:

> On Mon, Dec 09, 2002 at 11:54:20AM +0000, Dominic Sweetman wrote:
>
> > I'd like to be clear about the consequences of this.  Presumably the
> > 'access_ok()' macro is used to check addresses which were (originally)
> > provided by a user program's system call.
> >
> > Carsten, are you saying that if such an address is set to say 2**41 in
> > a CPU supporting 40-bit user virtual addresses, that the kernel will
> > crash?
>
> That's correct.  The problem which Carsten diagnosed correctly was the
> assumption which has been inherited from the 32-bit kernel that the sign-
> bit makes the difference between valid userspace and kernelspace
> addresses.
>
> Linux doesn't use the supervisor mode so basically that assumption is still
> true with the except of the area 2^PHYSBITS ... 2^63-1.
>
> > If so, that seems to require a fix, even if we don't know a very
> > efficient one.  But perhaps any problem is a bit more subtle than
> > that?
>
> Access_ok is a macro which depending on kernel configuration is expanded
> hundreds, if not thousands of times throughout the kernel.  So every single
> machine instruction in access_ok will make a size difference of several
> kB.  Carsten's patch was performing pretty badly in that cathegory.  If
> access_ok wasn't used that often the issue certainly wasn't worth the fuzz.
>
> Access_ok is of course only usable in C code.  We also have a few piece of
> assembler code that access userspace and need to perform the same kind of
> address validation tests.  Carsten's patch was missing these completly.  As
> such it did only reduce the window of this bug from huge to "just" big.
>

At least I haven't hit those holes, the would have been fixed otherwise, too
;-)


>
> An efficient solution only requires fairly minor changes as you can see in
> the patch I just posted.  It doesn't even require thinking, it can be
> obtained by cut'n'paste from the Alpha code.  Alternatively the problem
> could also have been solved by forwarding address errors for the address
> range in question to the page fault handler which would have served the
> same purpose, maybe even a tad more efficient but ofuscated ...
>

I absolutely agree that we should go for an optimized solution, but we discuss
this issue 1/2 year ago, none of us, had the time to come up with a better fix
than the one I send. I'm going through my to-do list and came across this
issue again, and I just wanted to reopen the case again.
This time it annoyed you enough, so you came up with a better solution and
I achieved what I came for, so that's great ;-)
Thanks a lot. I will try you patch right away.


>
>   Ralf

--
_    _ ____  ___   Carsten Langgaard   Mailto:carstenl@mips.com
|\  /|||___)(___   MIPS Denmark        Direct: +45 4486 5527
| \/ |||    ____)  Lautrupvang 4B      Switch: +45 4486 5555
  TECHNOLOGIES     2750 Ballerup       Fax...: +45 4486 5556
                   Denmark             http://www.mips.com

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

[Carsten Langgaard]

Your patch seems to do the job, thanks a lot.

/Carsten


Ralf Baechle wrote:

> On Mon, Dec 09, 2002 at 10:30:03AM +0100, Carsten Langgaard wrote:
>
> > > 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 ...
> >
> > I tried you patch below, but then nothing seems to work.
>
> The reason for this problem (and a few others is the broken call to
> __access_ok() in clear_user().  That should actually be access_ok().
> Basically the kernel was only working so far because addresses were just
> right ...
>
> Below my working version.  I still needs to make TASK_SIZE variable but
> with the clear_user thing fixed that should be easy.
>
>   Ralf
>
> Index: arch/mips64/kernel/scall_o32.S
> ===================================================================
> RCS file: /home/cvs/linux/arch/mips64/kernel/scall_o32.S,v
> retrieving revision 1.48.2.21
> diff -u -r1.48.2.21 scall_o32.S
> --- arch/mips64/kernel/scall_o32.S      3 Dec 2002 14:23:05 -0000       1.48.2.21
> +++ arch/mips64/kernel/scall_o32.S      8 Dec 2002 06:08:55 -0000
> @@ -209,7 +209,7 @@
>         daddiu  a0, a1, 4
>         or      a0, a0, a1
>         and     a0, a0, v1
> -       bltz    a0, bad_address
> +       bnez    a0, bad_address
>
>         /* Ok, this is the ll/sc case.  World is sane :-)  */
>  1:     ll      v0, (a1)
> @@ -273,7 +273,7 @@
>         ld      v1, THREAD_CURDS($28)
>         or      v0, v0, t1
>         and     v1, v1, v0
> -       bltz    v1, efault
> +       bnez    v1, efault
>
>         move    a0, a1                  # shift argument registers
>         move    a1, a2
> Index: arch/mips64/lib/strlen_user.S
> ===================================================================
> RCS file: /home/cvs/linux/arch/mips64/lib/strlen_user.S,v
> retrieving revision 1.4.2.1
> diff -u -r1.4.2.1 strlen_user.S
> --- arch/mips64/lib/strlen_user.S       1 Jul 2002 15:27:29 -0000       1.4.2.1
> +++ arch/mips64/lib/strlen_user.S       8 Dec 2002 06:08:55 -0000
> @@ -25,7 +25,7 @@
>  LEAF(__strlen_user_asm)
>         ld      v0, THREAD_CURDS($28)                   # pointer ok?
>         and     v0, a0
> -       bltz    v0, fault
> +       bnez    v0, fault
>
>  FEXPORT(__strlen_user_nocheck_asm)
>         move    v0, a0
> Index: arch/mips64/lib/strncpy_user.S
> ===================================================================
> RCS file: /home/cvs/linux/arch/mips64/lib/strncpy_user.S,v
> retrieving revision 1.4
> diff -u -r1.4 strncpy_user.S
> --- arch/mips64/lib/strncpy_user.S      9 Jul 2001 00:25:37 -0000       1.4
> +++ arch/mips64/lib/strncpy_user.S      8 Dec 2002 06:08:55 -0000
> @@ -30,7 +30,7 @@
>  LEAF(__strncpy_from_user_asm)
>         ld      v0, THREAD_CURDS($28)           # pointer ok?
>         and     v0, a1
> -       bltz    v0, fault
> +       bnez    v0, fault
>
>  FEXPORT(__strncpy_from_user_nocheck_asm)
>         move    v0, zero
> Index: arch/mips64/lib/strnlen_user.S
> ===================================================================
> RCS file: /home/cvs/linux/arch/mips64/lib/strnlen_user.S,v
> retrieving revision 1.2.2.2
> diff -u -r1.2.2.2 strnlen_user.S
> --- arch/mips64/lib/strnlen_user.S      1 Jul 2002 15:27:29 -0000       1.2.2.2
> +++ arch/mips64/lib/strnlen_user.S      8 Dec 2002 06:08:55 -0000
> @@ -25,7 +25,7 @@
>  LEAF(__strnlen_user_asm)
>         ld      v0, THREAD_CURDS($28)   # pointer ok?
>         and     v0, a0
> -       bltz    v0, fault
> +       bnez    v0, fault
>
>  FEXPORT(__strnlen_user_nocheck_asm)
>         move    v0, a0
> 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      8 Dec 2002 06:09:38 -0000
> @@ -208,7 +208,7 @@
>         /* \
>          * For now the default is to fix address errors \
>          */ \
> -       MF_FIXADE, { 0 }, 0, 0 \
> +       MF_FIXADE, KERNEL_DS, 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        8 Dec 2002 06:09:39 -0000
> @@ -22,8 +22,8 @@
>   *
>   * 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 KERNEL_DS      ((mm_segment_t) { 0UL })
> +#define USER_DS                ((mm_segment_t) { -TASK_SIZE })
>
>  #define VERIFY_READ    0
>  #define VERIFY_WRITE   1
> @@ -46,19 +46,19 @@
>   *  - OR we are in kernel mode.
>   */
>  #define __ua_size(size)                                                        \
> -       (__builtin_constant_p(size) && (signed long) (size) > 0 ? 0 : (size))
> +       ((__builtin_constant_p(size) && (size)) > 0 ? 0 : (size))
>
> -#define __access_ok(addr,size,mask)                                    \
> -       (((signed long)((mask)&(addr | (addr + size) | __ua_size(size)))) >= 0)
> +#define __access_ok(addr, size, mask)                                  \
> +       (((mask) & ((addr) | ((addr) + (size)) | __ua_size(size))) == 0)
>
> -#define __access_mask ((long)(get_fs().seg))
> +#define __access_mask get_fs().seg
>
> -#define access_ok(type,addr,size) \
> -       __access_ok(((unsigned long)(addr)),(size),__access_mask)
> +#define access_ok(type, addr, size)                                    \
> +       __access_ok((unsigned long)(addr), (size), __access_mask)
>
>  static inline int verify_area(int type, const void * addr, unsigned long size)
>  {
> -       return access_ok(type,addr,size) ? 0 : -EFAULT;
> +       return access_ok(type, addr, size) ? 0 : -EFAULT;
>  }
>
>  /*
> @@ -340,8 +340,8 @@
>  ({                                                             \
>         void * __cl_addr = (addr);                              \
>         unsigned long __cl_size = (n);                          \
> -       if (__cl_size && __access_ok(VERIFY_WRITE,              \
> -              ((unsigned long)(__cl_addr)), __cl_size))        \
> +       if (__cl_size && access_ok(VERIFY_WRITE,                \
> +               ((unsigned long)(__cl_addr)), __cl_size))       \
>                 __cl_size = __clear_user(__cl_addr, __cl_size); \
>         __cl_size;                                              \
>  })

--
_    _ ____  ___   Carsten Langgaard   Mailto:carstenl@mips.com
|\  /|||___)(___   MIPS Denmark        Direct: +45 4486 5527
| \/ |||    ____)  Lautrupvang 4B      Switch: +45 4486 5555
  TECHNOLOGIES     2750 Ballerup       Fax...: +45 4486 5556
                   Denmark             http://www.mips.com

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

[Ralf Baechle]

On Tue, Dec 10, 2002 at 08:50:55AM +0100, Carsten Langgaard wrote:

> I absolutely agree that we should go for an optimized solution, but we discuss
> this issue 1/2 year ago, none of us, had the time to come up with a better fix
> than the one I send. I'm going through my to-do list and came across this
> issue again, and I just wanted to reopen the case again.
> This time it annoyed you enough, so you came up with a better solution and
> I achieved what I came for, so that's great ;-)
> Thanks a lot. I will try you patch right away.

It's already in CVS.

Btw, a missconfigured firewall turned linux-mips.org into a heating for the
server room last night.  This should be fixed now.  The machine is now
hosted at MIPS UK, formerly Algorithmics.  There should be no user visible
changes.

  Ralf