The 64-bit version of __access_ok is broken.

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.

Quoted 12 lines · Carsten Langgaard

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.

Quoted 5 lines

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

mips64/kernel/process.c 7 Dec 2002 02:13:40 -0000 renamed from mips64/kernel/process.c 2 Dec 2002 00:24:52 -0000 1.18.2.9 +3 -1

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

asm-mips64/processor.h 7 Dec 2002 02:14:10 -0000 renamed from asm-mips64/processor.h 4 Nov 2002 19:39:56 -0000 1.32.2.9 +2 -2

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

asm-mips64/uaccess.h 7 Dec 2002 02:14:10 -0000 renamed from asm-mips64/uaccess.h 1 Jul 2002 15:27:31 -0000 1.13.2.1 +1 -1

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

Quoted 7 lines

Quoted 7 lines

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.

Quoted 6 lines

Quoted 6 lines

Quoted 6 lines

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

Quoted 3 lines

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.

Quoted 7 lines · Dominic Sweetman

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.

Quoted 3 lines

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

Quoted 5 lines · Carsten Langgaard

Quoted 3 lines

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

mips64/kernel/scall_o32.S 8 Dec 2002 06:08:55 -0000 renamed from mips64/kernel/scall_o32.S 3 Dec 2002 14:23:05 -0000 1.48.2.21 +2 -2

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

mips64/lib/strlen_user.S 8 Dec 2002 06:08:55 -0000 renamed from mips64/lib/strlen_user.S 1 Jul 2002 15:27:29 -0000 1.4.2.1 +1 -1

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

mips64/lib/strncpy_user.S 8 Dec 2002 06:08:55 -0000 renamed from mips64/lib/strncpy_user.S 9 Jul 2001 00:25:37 -0000 1.4 +1 -1

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

mips64/lib/strnlen_user.S 8 Dec 2002 06:08:55 -0000 renamed from mips64/lib/strnlen_user.S 1 Jul 2002 15:27:29 -0000 1.2.2.2 +1 -1

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

asm-mips64/processor.h 8 Dec 2002 06:09:38 -0000 renamed from asm-mips64/processor.h 4 Nov 2002 19:39:56 -0000 1.32.2.9 +1 -1

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

asm-mips64/uaccess.h 8 Dec 2002 06:09:39 -0000 renamed from asm-mips64/uaccess.h 1 Jul 2002 15:27:31 -0000 1.13.2.1 +9 -9

@@ -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;
 }
 
 /*

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.

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?