Subject: small optimisations

From: Philip Blundell <[email-masked]>
Date: Thu, 27 Jul 2000 21:59:34 +0100

Here's another patch I'd like to get some opinions on.

Thanks

p.

--- linux/arch/arm/lib/uaccess.S	Mon Jun 26 22:11:20 2000
+++ linux/arch/arm/lib/uaccess.S	Thu Jul 27 15:35:50 2000
@@ -597,7 +597,7 @@ USER(		strnebt	r2, [r0], #1)
  *	      or zero on exception, or n + 1 if too long
  */
 ENTRY(__arch_strnlen_user)
-		stmfd	sp!, {lr}
+		str	lr, [sp, #-4]!
 		mov	r2, r0
 1:
 USER(		ldrbt	r3, [r0], #1)
@@ -623,7 +623,7 @@ USER(		ldrbt	r3, [r0], #1)
  * Returns  : number of characters copied
  */
 ENTRY(__arch_strncpy_from_user)
-		stmfd	sp!, {lr}
+		str	lr, [sp, #-4]!
 		add	ip, r1, #1
 1:		subs	r2, r2, #1
 		bmi	2f
--- linux/include/asm-arm/uaccess.h	Sun Apr 30 11:00:03 2000
+++ linux/include/asm-arm/uaccess.h	Thu Jul 27 19:50:10 2000
@@ -23,6 +23,15 @@
  * on our cache or tlb entries.
  */
 
+/* 
+ * Functions that read from user space need to take care not to leak
+ * kernel data even if the calling code is buggy and fails to check
+ * the return value.  This means zeroing out the destination variable
+ * or buffer on an error.  Normally this is done out of line by the
+ * fixup code but there are a few places where it intrudes on the main
+ * code path.  When we only write to user space there is no problem.
+ */
+
 struct exception_table_entry
 {
 	unsigned long insn, fixup;
@@ -59,19 +68,18 @@ extern __inline__ int verify_area(int ty
 #define __get_user(x,p)		__get_user_nocheck((x),(p),sizeof(*(p)))
 #define get_user_ret(x,p,r)	({ if (get_user(x,p)) return r; })
 #define __get_user_ret(x,p,r)	({ if (__get_user(x,p)) return r; })
+#define __get_user3(x,p,l)	__get_user3_nocheck((x),(p),sizeof(*(p)),l)
 
 #define put_user(x,p)		__put_user_check((__typeof(*(p)))(x),(p),sizeof(*(p)))
 #define __put_user(x,p)		__put_user_nocheck((__typeof(*(p)))(x),(p),sizeof(*(p)))
+#define __put_user3(x,p,e)	__put_user3_nocheck((__typeof(*(p)))(x),(p),sizeof(*(p)),e)
 #define put_user_ret(x,p,r)	({ if (put_user(x,p)) return r; })
 #define __put_user_ret(x,p,r)	({ if (__put_user(x,p)) return r; })
 
 static __inline__ unsigned long copy_from_user(void *to, const void *from, unsigned long n)
 {
-	char *end = (char *)to + n;
-	if (access_ok(VERIFY_READ, from, n)) {
+	if (access_ok(VERIFY_READ, from, n))
 		__do_copy_from_user(to, from, n);
-		if (n) memset(end - n, 0, n);
-	}
 	return n;
 }
 
@@ -145,28 +153,41 @@ extern __inline__ long strnlen_user(cons
  */
 #define __get_user_check(x,ptr,size)					\
 ({									\
-	long __gu_err = -EFAULT, __gu_val = 0;				\
+	long __gu_err = 0, __gu_val;					\
 	const __typeof__(*(ptr)) *__gu_addr = (ptr);			\
 	if (access_ok(VERIFY_READ,__gu_addr,size))			\
 		__get_user_size(__gu_val,__gu_addr,(size),__gu_err);	\
+	else {								\
+		__gu_val = 0;						\
+		__gu_err = -EFAULT;					\
+	}								\
 	(x) = (__typeof__(*(ptr)))__gu_val;				\
 	__gu_err;							\
 })
 
 #define __get_user_nocheck(x,ptr,size)					\
 ({									\
-	long __gu_err = 0, __gu_val = 0;				\
+	long __gu_err = 0, __gu_val;					\
 	__get_user_size(__gu_val,(ptr),(size),__gu_err);		\
 	(x) = (__typeof__(*(ptr)))__gu_val;				\
 	__gu_err;							\
 })
 
+#define __get_user3_nocheck(x,ptr,size,lab)				\
+({									\
+	long __gu_val;							\
+	__get_user_size(__gu_val,(ptr),(size),lab);			\
+	(x) = (__typeof__(*(ptr)))__gu_val;				\
+})
+
 #define __put_user_check(x,ptr,size)					\
 ({									\
-	long __pu_err = -EFAULT;					\
+	long __pu_err = 0;						\
 	__typeof__(*(ptr)) *__pu_addr = (ptr);				\
 	if (access_ok(VERIFY_WRITE,__pu_addr,size))			\
 		__put_user_size((x),__pu_addr,(size),__pu_err);		\
+	else								\
+		__pu_err = -EFAULT;					\
 	__pu_err;							\
 })
 
@@ -177,11 +198,15 @@ extern __inline__ long strnlen_user(cons
 	__pu_err;							\
 })
 
+#define __put_user3_nocheck(x,ptr,size,err)				\
+({									\
+	__put_user_size((x),(ptr),(size),err);				\
+})
+
 extern long __get_user_bad(void);
 
 #define __get_user_size(x,ptr,size,retval)				\
 do {									\
-	retval = 0;							\
 	switch (size) {							\
 	case 1:	__get_user_asm_byte(x,ptr,retval);	break;		\
 	case 2:	__get_user_asm_half(x,ptr,retval);	break;		\
@@ -194,7 +219,6 @@ extern long __put_user_bad(void);
 
 #define __put_user_size(x,ptr,size,retval)				\
 do {									\
-	retval = 0;							\
 	switch (size) {							\
 	case 1: __put_user_asm_byte(x,ptr,retval);	break;		\
 	case 2: __put_user_asm_half(x,ptr,retval);	break;		\
--- linux/arch/arm/kernel/signal.c	Wed Jul 26 20:36:57 2000
+++ linux/arch/arm/kernel/signal.c	Thu Jul 27 17:51:02 2000
@@ -36,7 +37,8 @@ asmlinkage int do_signal(sigset_t *oldse
 int copy_siginfo_to_user(siginfo_t *to, siginfo_t *from)
 {
 	if (!access_ok (VERIFY_WRITE, to, sizeof(siginfo_t)))
-		return -EFAULT;
+		goto fault;
+
 	if (from->si_code < 0)
 		return __copy_to_user(to, from, sizeof(siginfo_t));
 	else {
@@ -66,6 +68,8 @@ int copy_siginfo_to_user(siginfo_t *to, 
 		}
 		return err;
 	}
+	fault:
+		return -EFAULT;
 }
 
 /*
@@ -175,24 +179,24 @@ restore_sigcontext(struct pt_regs *regs,
 {
 	int err = 0;
 
-	err |= __get_user(regs->ARM_r0, &sc->arm_r0);
-	err |= __get_user(regs->ARM_r1, &sc->arm_r1);
-	err |= __get_user(regs->ARM_r2, &sc->arm_r2);
-	err |= __get_user(regs->ARM_r3, &sc->arm_r3);
-	err |= __get_user(regs->ARM_r4, &sc->arm_r4);
-	err |= __get_user(regs->ARM_r5, &sc->arm_r5);
-	err |= __get_user(regs->ARM_r6, &sc->arm_r6);
-	err |= __get_user(regs->ARM_r7, &sc->arm_r7);
-	err |= __get_user(regs->ARM_r8, &sc->arm_r8);
-	err |= __get_user(regs->ARM_r9, &sc->arm_r9);
-	err |= __get_user(regs->ARM_r10, &sc->arm_r10);
-	err |= __get_user(regs->ARM_fp, &sc->arm_fp);
-	err |= __get_user(regs->ARM_ip, &sc->arm_ip);
-	err |= __get_user(regs->ARM_sp, &sc->arm_sp);
-	err |= __get_user(regs->ARM_lr, &sc->arm_lr);
-	err |= __get_user(regs->ARM_pc, &sc->arm_pc);
+	__get_user3(regs->ARM_r0, &sc->arm_r0, err);
+	__get_user3(regs->ARM_r1, &sc->arm_r1, err);
+	__get_user3(regs->ARM_r2, &sc->arm_r2, err);
+	__get_user3(regs->ARM_r3, &sc->arm_r3, err);
+	__get_user3(regs->ARM_r4, &sc->arm_r4, err);
+	__get_user3(regs->ARM_r5, &sc->arm_r5, err);
+	__get_user3(regs->ARM_r6, &sc->arm_r6, err);
+	__get_user3(regs->ARM_r7, &sc->arm_r7, err);
+	__get_user3(regs->ARM_r8, &sc->arm_r8, err);
+	__get_user3(regs->ARM_r9, &sc->arm_r9, err);
+	__get_user3(regs->ARM_r10, &sc->arm_r10, err);
+	__get_user3(regs->ARM_fp, &sc->arm_fp, err);
+	__get_user3(regs->ARM_ip, &sc->arm_ip, err);
+	__get_user3(regs->ARM_sp, &sc->arm_sp, err);
+	__get_user3(regs->ARM_lr, &sc->arm_lr, err);
+	__get_user3(regs->ARM_pc, &sc->arm_pc, err);
 #ifdef CONFIG_CPU_32
-	err |= __get_user(regs->ARM_cpsr, &sc->arm_cpsr);
+	__get_user3(regs->ARM_cpsr, &sc->arm_cpsr, err);
 #endif
 
 	err |= !valid_user_regs(regs);
@@ -289,30 +293,30 @@ setup_sigcontext(struct sigcontext *sc, 
 {
 	int err = 0;
 
-	err |= __put_user (regs->ARM_r0, &sc->arm_r0);
-	err |= __put_user (regs->ARM_r1, &sc->arm_r1);
-	err |= __put_user (regs->ARM_r2, &sc->arm_r2);
-	err |= __put_user (regs->ARM_r3, &sc->arm_r3);
-	err |= __put_user (regs->ARM_r4, &sc->arm_r4);
-	err |= __put_user (regs->ARM_r5, &sc->arm_r5);
-	err |= __put_user (regs->ARM_r6, &sc->arm_r6);
-	err |= __put_user (regs->ARM_r7, &sc->arm_r7);
-	err |= __put_user (regs->ARM_r8, &sc->arm_r8);
-	err |= __put_user (regs->ARM_r9, &sc->arm_r9);
-	err |= __put_user (regs->ARM_r10, &sc->arm_r10);
-	err |= __put_user (regs->ARM_fp, &sc->arm_fp);
-	err |= __put_user (regs->ARM_ip, &sc->arm_ip);
-	err |= __put_user (regs->ARM_sp, &sc->arm_sp);
-	err |= __put_user (regs->ARM_lr, &sc->arm_lr);
-	err |= __put_user (regs->ARM_pc, &sc->arm_pc);
+	__put_user3 (regs->ARM_r0, &sc->arm_r0, err);
+	__put_user3 (regs->ARM_r1, &sc->arm_r1, err);
+	__put_user3 (regs->ARM_r2, &sc->arm_r2, err);
+	__put_user3 (regs->ARM_r3, &sc->arm_r3, err);
+	__put_user3 (regs->ARM_r4, &sc->arm_r4, err);
+	__put_user3 (regs->ARM_r5, &sc->arm_r5, err);
+	__put_user3 (regs->ARM_r6, &sc->arm_r6, err);
+	__put_user3 (regs->ARM_r7, &sc->arm_r7, err);
+	__put_user3 (regs->ARM_r8, &sc->arm_r8, err);
+	__put_user3 (regs->ARM_r9, &sc->arm_r9, err);
+	__put_user3 (regs->ARM_r10, &sc->arm_r10, err);
+	__put_user3 (regs->ARM_fp, &sc->arm_fp, err);
+	__put_user3 (regs->ARM_ip, &sc->arm_ip, err);
+	__put_user3 (regs->ARM_sp, &sc->arm_sp, err);
+	__put_user3 (regs->ARM_lr, &sc->arm_lr, err);
+	__put_user3 (regs->ARM_pc, &sc->arm_pc, err);
 #ifdef CONFIG_CPU_32
-	err |= __put_user (regs->ARM_cpsr, &sc->arm_cpsr);
+	__put_user3 (regs->ARM_cpsr, &sc->arm_cpsr, err);
 #endif
 
-	err |= __put_user (current->thread.trap_no, &sc->trap_no);
-	err |= __put_user (current->thread.error_code, &sc->error_code);
-	err |= __put_user (current->thread.address, &sc->fault_address);
-	err |= __put_user (mask, &sc->oldmask);
+	__put_user3 (current->thread.trap_no, &sc->trap_no, err);
+	__put_user3 (current->thread.error_code, &sc->error_code, err);
+	__put_user3 (current->thread.address, &sc->fault_address, err);
+	__put_user3 (mask, &sc->oldmask, err);
 
 	return err;
 }

unsubscribe: body of `unsubscribe linux-arm-kernel' to [email-masked]

---

From: Russell King - ARM Linux Admin <[email-masked]>
Date: Thu, 27 Jul 2000 22:21:52 +0100 (BST)

>  ENTRY(__arch_strncpy_from_user)

These aren't particularly performance-related functions, and such a
change would be of little benefit.  Can you enlighten me as to the
exact reasoning behind this, eg listing which processors this has
an advantage on?

> -		if (n) memset(end - n, 0, n);

This opens a security hole.

>  	(x) = (__typeof__(*(ptr)))__gu_val;				\

I tested this one about a year ago, and it resulted in poorer code.
Please show a comparison between the assembler produced by the old
code and the new, specifing the compiler version.

>  	(x) = (__typeof__(*(ptr)))__gu_val;				\

Another security hole.  Hint: __get_user_nocheck() actually faults.
The user doesn't check the error code.  What value is left in __gu_val?
(Note: this is the exact stance taken by Linus here, and I agree with
him on these points - its better to be safer than sorry).

Comments on the rest depend on the answers to the above.
   _____
  |_____| ------------------------------------------------- ---+---+-
  |   |        Russell King       [email-masked]      --- ---
  | | | |            http://www.arm.linux.org.uk/            /  /  |
  | +-+-+                                                     --- -+-
  /   |               THE developer of ARM Linux              |+| /|\
 /  | | |                                                     ---  |
    +-+-+ -------------------------------------------------  /\\\  |

---

From: Philip Blundell <[email-masked]>
Date: Thu, 27 Jul 2000 23:16:48 +0100

>exact reasoning behind this, eg listing which processors this has

It's faster on StrongARM.

>This opens a security hole.

Why?  The copy_from_user code in uaccess.S already checks for exceptions and 
zeros the balance of the buffer, as far as I can see.

>The user doesn't check the error code.

Mmm.  Yes, I think you're right.

p.

---

From: Russell King - ARM Linux Admin <[email-masked]>
Date: Thu, 27 Jul 2000 23:18:17 +0100 (BST)

> It's faster on StrongARM.

I guessed that one.  Any others?

> zeros the balance of the buffer, as far as I can see.

You're right here.
   _____

---

From: Philip Blundell <[email-masked]>
Date: Thu, 27 Jul 2000 23:38:43 +0100

>code and the new, specifing the compiler version.

Having checked that one again, you're right that it does seem to make things 
worse.  I withdraw that part of the patch; maybe we can revisit it when gcc's 
support for predication is a bit better.

(Incidentally, it looks to me like the exception handlers in 
__get_user_asm_word and so on are actually loading -ENOTBLK rather than 
-EFAULT on error.)

p.

---

From: Philip Blundell <[email-masked]>
Date: Thu, 27 Jul 2000 23:40:52 +0100

>I guessed that one.  Any others?

I'm not sure.  On ARM6 and ARM7 the timings are identical.  I suspect ARM9 
and ARM10 behave like StrongARM but I haven't checked the documentation.  
As to ARM8, who knows.

p.

---

From: Russell King - ARM Linux Admin <[email-masked]>
Date: Thu, 27 Jul 2000 23:44:12 +0100 (BST)

> __get_user_asm_word and so on are actually loading -ENOTBLK rather than 

Right again, fixed.
   _____

---

From: Russell King - ARM Linux Admin <[email-masked]>
Date: Thu, 27 Jul 2000 23:48:13 +0100 (BST)

> and ARM10 behave like StrongARM but I haven't checked the documentation.  

Ok, I've applied that part of the patch, and only that part.  Please
submit the other bits that are still suitable to the patch system
once they're ready.
   _____
  |_____| ------------------------------------------------- ---+---+-
  |   |        Russell King       [email-masked]      --- ---
  | | | |            http://www.arm.linux.org.uk/            /  /  |
  | +-+-+                                                     --- -+-
  /   |               THE developer of ARM Linux              |+| /|\
 /  | | |                                                     ---  |
    +-+-+ -------------------------------------------------  /\\\  |