access_ok and CONFIG_MIPS32 for 2.6

access_ok and CONFIG_MIPS32 for 2.6

[Dimitri Torfs]

Hi,

  the mask used in access_ok to check the validity of an address range
  evaluates to -TASK_SIZE for user processes. In case of
  CONFIG_MIPS32, TASK_SIZE is defined as 0x7fff8000UL, so -TASK_SIZE
  evaluates to 0x80008000, making access_ok return false for all
  addresses with bit 15 and 31 set. Surely the mask should be 0x80000000. 

  Does anybody know why TASK_SIZE is set to 0x7fff8000 and not
  0x80000000 ?


  Dimitri 


-- 
Dimitri Torfs             |  NSCE 
dimitri.torfs@sonycom.com |  Sint Stevens Woluwestraat 55
tel: +32 2 2908451        |  1130 Brussel
fax: +32 2 7262686        |  Belgium

Re: access_ok and CONFIG_MIPS32 for 2.6

[Ralf Baechle]

On Fri, Jan 02, 2004 at 03:59:41PM +0100, Dimitri Torfs wrote:

>   the mask used in access_ok to check the validity of an address range
>   evaluates to -TASK_SIZE for user processes. In case of
>   CONFIG_MIPS32, TASK_SIZE is defined as 0x7fff8000UL, so -TASK_SIZE
>   evaluates to 0x80008000, making access_ok return false for all
>   addresses with bit 15 and 31 set. Surely the mask should be 0x80000000. 
> 
>   Does anybody know why TASK_SIZE is set to 0x7fff8000 and not
>   0x80000000 ?

There is a weird special case were 32-bit code running on a 64-bit kernel
with c0_status.ux set will behave differently than on a 32-bit processor
or with c0_status.ux clear.  The workaround for 64-bit kernels is to
leave the top 32kB of the 2GB user virtual address space unused.  For
sake of symmetry we do this on both 32-bit and 64-bit kernels.

  Ralf

Re: access_ok and CONFIG_MIPS32 for 2.6

[Atsushi Nemoto]

>>>>> On Fri, 2 Jan 2004 20:44:03 +0100, Ralf Baechle <ralf@linux-mips.org> said:

>> Does anybody know why TASK_SIZE is set to 0x7fff8000 and not
>> 0x80000000 ?

ralf> There is a weird special case were 32-bit code running on a
ralf> 64-bit kernel with c0_status.ux set will behave differently than
ralf> on a 32-bit processor or with c0_status.ux clear.  The
ralf> workaround for 64-bit kernels is to leave the top 32kB of the
ralf> 2GB user virtual address space unused.  For sake of symmetry we
ralf> do this on both 32-bit and 64-bit kernels.

Then, access_ok in 2.6 tree is broken, isn't it?

2.4 mips:
#define TASK_SIZE	0x7fff8000UL
#define USER_DS		((mm_segment_t) { (unsigned long) -1L })

2.4 mips64:
#define TASK_SIZE32	   0x7fff8000UL
#define TASK_SIZE	0x10000000000UL
#define USER_DS		((mm_segment_t) { -TASK_SIZE })

2.6:
#ifdef CONFIG_MIPS32
#define TASK_SIZE	0x7fff8000UL
#else
#define TASK_SIZE32	0x7fff8000UL
#define TASK_SIZE	0x10000000000UL
#endif
#define USER_DS		((mm_segment_t) { -TASK_SIZE })

It seems there should be another definition of USER_DS for
CONFIG_MIPS32 in 2.6.


BTW, there are another problems in uaccess.h as I wrote in
http://www.linux-mips.org/archives/linux-mips/2003-09/msg00035.html.

First, 2.4 mips64 __ua_size is broken.

2.4 mips:
#define __ua_size(size)							\
	(__builtin_constant_p(size) && (signed long) (size) > 0 ? 0 : (size))
2.4 mips64:
#define __ua_size(size)							\
	((__builtin_constant_p(size) && (size)) > 0 ? 0 : (size))
2.6:
#define __ua_size(size)							\
	((__builtin_constant_p(size) && (signed long) (size) > 0) ? 0 : (size))

2.4 mips and 2.6 are identical except for parenthesis.


Second, __access_ok for 64bit kernel is broken both 2.4 and 2.6.  It
returns 0 if 'addr' + 'size' == TASK_SIZE (which should be OK).

2.4 mips64:
#define __access_ok(addr, size, mask)					\
	(((mask) & ((addr) | ((addr) + (size)) | __ua_size(size))) == 0)
2.6:
#define __access_ok(addr, size, mask)					\
	(((signed long)((mask) & ((addr) | ((addr) + (size)) | __ua_size(size)))) == 0)

I think these macros should be:

2.4 mips64:
#define __access_ok(addr, size, mask)					\
	(((mask) & ((addr) | ((addr) + (size) - 1) | __ua_size(size))) == 0)
2.6:
#define __access_ok(addr, size, mask)					\
	(((signed long)((mask) & ((addr) | ((addr) + (size) - 1) | __ua_size(size)))) == 0)

---
Atsushi Nemoto

Re: access_ok and CONFIG_MIPS32 for 2.6

[Dimitri Torfs]

On Sun, Jan 04, 2004 at 09:05:32PM +0900, Atsushi Nemoto wrote:
> >>>>> On Fri, 2 Jan 2004 20:44:03 +0100, Ralf Baechle <ralf@linux-mips.org> said:
> 
> >> Does anybody know why TASK_SIZE is set to 0x7fff8000 and not
> >> 0x80000000 ?
> 
> ralf> There is a weird special case were 32-bit code running on a
> ralf> 64-bit kernel with c0_status.ux set will behave differently than
> ralf> on a 32-bit processor or with c0_status.ux clear.  The
> ralf> workaround for 64-bit kernels is to leave the top 32kB of the
> ralf> 2GB user virtual address space unused.  For sake of symmetry we
> ralf> do this on both 32-bit and 64-bit kernels.
> 
> Then, access_ok in 2.6 tree is broken, isn't it?
> 
> It seems there should be another definition of USER_DS for
> CONFIG_MIPS32 in 2.6.

Yes, I'm setting USER_DS to 0x80000000 for CONFIG_MIPS32:


--- linux-mips-2.6.orig/include/asm-mips/uaccess.h	2003-11-30 13:59:06.000000000 +0100
+++ linux.work/include/asm-mips/uaccess.h	2004-01-04 21:22:23.000000000 +0100
@@ -42,7 +42,12 @@
 #endif /* CONFIG_MIPS64 */
 
 #define KERNEL_DS	((mm_segment_t) { 0UL })
+
+#ifdef CONFIG_MIPS32
+#define USER_DS		((mm_segment_t) { 0x80000000UL })
+#else
 #define USER_DS		((mm_segment_t) { -TASK_SIZE })
+#endif
 
 #define VERIFY_READ    0
 #define VERIFY_WRITE   1


The USER_DS mask is also used in scall32-o32.S, scall64-64.S and
scall64-032.S. It think it would be cleaner if we replace there also
the ">= 0" check with the "== 0" check and add the correct size as you
suggested:


--- linux-mips-2.6.orig/arch/mips/kernel/scall64-64.S	2003-10-11 00:58:55.000000000 +0200
+++ linux.work/arch/mips/kernel/scall64-64.S	2004-01-04 21:45:45.000000000 +0100
@@ -119,10 +119,10 @@
 	bnez	v0, bad_alignment
 
 	LONG_L	v1, TI_ADDR_LIMIT($28)		# in legal address range?
-	LONG_ADDIU	a0, a1, 4
+	LONG_ADDIU	a0, a1, 3 
 	or	a0, a0, a1
 	and	a0, a0, v1
-	bltz	a0, bad_address
+	bnez	a0, bad_address
 
 #ifdef CONFIG_CPU_HAS_LLSC
 	/* Ok, this is the ll/sc case.  World is sane :-)  */


--- linux-mips-2.6.orig/arch/mips/kernel/scall32-o32.S	2003-08-26 02:28:51.000000000 +0200
+++ linux.work/arch/mips/kernel/scall32-o32.S	2004-01-04 21:48:39.000000000 +0100
@@ -187,10 +187,10 @@
 	bnez	v0, bad_alignment
 
 	lw	v1, TI_ADDR_LIMIT($28)		# in legal address range?
-	addiu	a0, a1, 4
+	addiu	a0, a1, 3 
 	or	a0, a0, a1
 	and	a0, a0, v1
-	bltz	a0, bad_address
+	bnez	a0, bad_address
 
 #ifdef CONFIG_CPU_HAS_LLSC
 	/* Ok, this is the ll/sc case.  World is sane :-)  */
@@ -280,11 +280,11 @@
 	bnez	v0, sigsegv
 
 	addu	v0, t0, 16			# v0 = usp + 16
-	addu	t1, v0, 12			# 3 32-bit arguments
+	addu	t1, v0, 11			# 3 32-bit arguments
 	lw	v1, TI_ADDR_LIMIT($28)
 	or	v0, v0, t1
 	and	v1, v1, v0
-	bltz	v1, efault
+	bnez	v1, efault
 
 	move	a0, a1				# shift argument registers
 	move	a1, a2



--- linux-mips-2.6.orig/arch/mips/kernel/scall64-64.S	2003-10-11 00:58:55.000000000 +0200
+++ linux.work/arch/mips/kernel/scall64-64.S	2004-01-04 21:45:45.000000000 +0100
@@ -119,10 +119,10 @@
 	bnez	v0, bad_alignment
 
 	LONG_L	v1, TI_ADDR_LIMIT($28)		# in legal address range?
-	LONG_ADDIU	a0, a1, 4
+	LONG_ADDIU	a0, a1, 3 
 	or	a0, a0, a1
 	and	a0, a0, v1
-	bltz	a0, bad_address
+	bnez	a0, bad_address
 
 #ifdef CONFIG_CPU_HAS_LLSC
 	/* Ok, this is the ll/sc case.  World is sane :-)  */



Dimitri

Re: access_ok and CONFIG_MIPS32 for 2.6

[Atsushi Nemoto]

>>>>> On Sun, 4 Jan 2004 22:03:27 +0100, Dimitri Torfs <dimitri@sonycom.com> said:
>> It seems there should be another definition of USER_DS for
>> CONFIG_MIPS32 in 2.6.

dimitri> Yes, I'm setting USER_DS to 0x80000000 for CONFIG_MIPS32:

Now we can see this fix in CVS 2.6 tree (Thank you, Ralf).

Then, how about this one?

>>>>> On Sun, 04 Jan 2004 21:05:32 +0900 (JST), Atsushi Nemoto <anemo@mba.ocn.ne.jp> said:
> Second, __access_ok for 64bit kernel is broken both 2.4 and 2.6.  It
> returns 0 if 'addr' + 'size' == TASK_SIZE (which should be OK).
> 
> 2.4 mips64:
> #define __access_ok(addr, size, mask)					\
> 	(((mask) & ((addr) | ((addr) + (size)) | __ua_size(size))) == 0)
> 2.6:
> #define __access_ok(addr, size, mask)					\
> 	(((signed long)((mask) & ((addr) | ((addr) + (size)) | __ua_size(size)))) == 0)
> 
> I think these macros should be:
> 
> 2.4 mips64:
> #define __access_ok(addr, size, mask)					\
> 	(((mask) & ((addr) | ((addr) + (size) - 1) | __ua_size(size))) == 0)
> 2.6:
> #define __access_ok(addr, size, mask)					\
> 	(((signed long)((mask) & ((addr) | ((addr) + (size) - 1) | __ua_size(size)))) == 0)


For example, currently, access_ok(0xfffffff000UL, 0x1000) will return
0.  This must be legal (and this is a real problem for n64 mount
syscall which may grab user stack.  See copy_mount_option()).

---
Atsushi Nemoto