Re: [PATCH v2 04/20] x86: Rewrite copy_siginfo_{to,from}_user32

[H. Peter Anvin]

On November 4, 2015 4:50:23 PM PST, Amanieu d'Antras <amanieu@xxxxxxxxx> wrote:
>x86 can't use the generic versions because it needs to support
>x32, so we replace the ad-hoc implementations with something
>that is closer to the generic versions.
>
>This is done by completely replacing the existing code with the
>generic version, with some minor modifications to support x32.
>The new code is kept as close as possible to the generic version
>to make future changes to both functions easier.
>
>Unlike the previous implementation, this one guarantees that the
>compat behavior is identical to that of a 32-bit kernel.
>
>Signed-off-by: Amanieu d'Antras <amanieu@xxxxxxxxx>
>---
>arch/x86/kernel/signal_compat.c | 285
>++++++++++++++++++++++++++++++----------
> 1 file changed, 214 insertions(+), 71 deletions(-)
>
>diff --git a/arch/x86/kernel/signal_compat.c
>b/arch/x86/kernel/signal_compat.c
>index dc3c0b1..cdbb538 100644
>--- a/arch/x86/kernel/signal_compat.c
>+++ b/arch/x86/kernel/signal_compat.c
>@@ -3,93 +3,236 @@
> 
>int copy_siginfo_to_user32(compat_siginfo_t __user *to, const siginfo_t
>*from)
> {
>-	int err = 0;
>+	int err, si_code;
> 	bool ia32 = test_thread_flag(TIF_IA32);
> 
> 	if (!access_ok(VERIFY_WRITE, to, sizeof(compat_siginfo_t)))
> 		return -EFAULT;
> 
>-	put_user_try {
>-		/* If you change siginfo_t structure, please make sure that
>-		   this code is fixed accordingly.
>-		   It should never copy any pad contained in the structure
>-		   to avoid security leaks, but must copy the generic
>-		   3 ints plus the relevant union member.  */
>-		put_user_ex(from->si_signo, &to->si_signo);
>-		put_user_ex(from->si_errno, &to->si_errno);
>-		put_user_ex((short)from->si_code, &to->si_code);
>+	/*
>+	 * Get the user-visible si_code by hiding the top 16 bits if this is
>a
>+	 * kernel-generated signal.
>+	 */
>+	si_code = from->si_code < 0 ? from->si_code : (short)from->si_code;
> 
>-		if (from->si_code < 0) {
>-			put_user_ex(from->si_pid, &to->si_pid);
>-			put_user_ex(from->si_uid, &to->si_uid);
>-			put_user_ex(ptr_to_compat(from->si_ptr), &to->si_ptr);
>+	/*
>+	 * If you change siginfo_t structure, please be sure that
>+	 * all these functions are fixed accordingly:
>+	 * copy_siginfo_to_user
>+	 * copy_siginfo_to_user32
>+	 * copy_siginfo_from_user32
>+	 * signalfd_copyinfo
>+	 * They should never copy any pad contained in the structure
>+	 * to avoid security leaks, but must copy the generic
>+	 * 3 ints plus the relevant union member.
>+	 */
>+	err = __put_user(from->si_signo, &to->si_signo);
>+	err |= __put_user(from->si_errno, &to->si_errno);
>+	err |= __put_user(si_code, &to->si_code);
>+	if (from->si_code < 0) {
>+		/*
>+		 * Copy the tail bytes of the union from the padding, see the
>+		 * comment in copy_siginfo_from_user32. Note that this padding
>+		 * is always initialized when si_code < 0.
>+		 */
>+		BUILD_BUG_ON(sizeof(to->_sifields._pad) !=
>+			sizeof(from->_sifields._pad) + sizeof(from->_pad2));
>+		err |= __copy_to_user(to->_sifields._pad, from->_sifields._pad,
>+			sizeof(from->_sifields._pad)) ? -EFAULT : 0;
>+		err |= __copy_to_user(to->_sifields._pad + SI_PAD_SIZE,
>+			from->_pad2, sizeof(from->_pad2)) ? -EFAULT : 0;
>+		return err;
>+	}
>+	switch (from->si_code & __SI_MASK) {
>+	case __SI_KILL:
>+		err |= __put_user(from->si_pid, &to->si_pid);
>+		err |= __put_user(from->si_uid, &to->si_uid);
>+		break;
>+	case __SI_TIMER:
>+		err |= __put_user(from->si_tid, &to->si_tid);
>+		err |= __put_user(from->si_overrun, &to->si_overrun);
>+		/*
>+		 * Get the sigval from si_int, which matches the convention
>+		 * used in get_compat_sigevent.
>+		 */
>+		err |= __put_user(from->si_int, &to->si_int);
>+		break;
>+	case __SI_POLL:
>+		err |= __put_user(from->si_band, &to->si_band);
>+		err |= __put_user(from->si_fd, &to->si_fd);
>+		break;
>+	case __SI_FAULT:
>+		err |= __put_user(ptr_to_compat(from->si_addr), &to->si_addr);
>+#ifdef __ARCH_SI_TRAPNO
>+		err |= __put_user(from->si_trapno, &to->si_trapno);
>+#endif
>+#ifdef BUS_MCEERR_AO
>+		/*
>+		 * Other callers might not initialize the si_lsb field,
>+		 * so check explicitly for the right codes here.
>+		 */
>+		if (from->si_signo == SIGBUS &&
>+		    (from->si_code == BUS_MCEERR_AR ||
>+		     from->si_code == BUS_MCEERR_AO))
>+			err |= __put_user(from->si_addr_lsb, &to->si_addr_lsb);
>+#endif
>+#ifdef SEGV_BNDERR
>+		if (from->si_signo == SIGSEGV && from->si_code == SEGV_BNDERR) {
>+			err |= __put_user(ptr_to_compat(from->si_lower),
>+				&to->si_lower);
>+			err |= __put_user(ptr_to_compat(from->si_upper),
>+				&to->si_upper);
>+		}
>+#endif
>+		break;
>+	case __SI_CHLD:
>+		err |= __put_user(from->si_pid, &to->si_pid);
>+		err |= __put_user(from->si_uid, &to->si_uid);
>+		err |= __put_user(from->si_status, &to->si_status);
>+		if (ia32) {
>+			err |= __put_user(from->si_utime, &to->si_utime);
>+			err |= __put_user(from->si_stime, &to->si_stime);
> 		} else {
>-			/*
>-			 * First 32bits of unions are always present:
>-			 * si_pid === si_band === si_tid === si_addr(LS half)
>-			 */
>-			put_user_ex(from->_sifields._pad[0],
>-					  &to->_sifields._pad[0]);
>-			switch (from->si_code >> 16) {
>-			case __SI_FAULT >> 16:
>-				break;
>-			case __SI_SYS >> 16:
>-				put_user_ex(from->si_syscall, &to->si_syscall);
>-				put_user_ex(from->si_arch, &to->si_arch);
>-				break;
>-			case __SI_CHLD >> 16:
>-				if (ia32) {
>-					put_user_ex(from->si_utime, &to->si_utime);
>-					put_user_ex(from->si_stime, &to->si_stime);
>-				} else {
>-					put_user_ex(from->si_utime, &to->_sifields._sigchld_x32._utime);
>-					put_user_ex(from->si_stime, &to->_sifields._sigchld_x32._stime);
>-				}
>-				put_user_ex(from->si_status, &to->si_status);
>-				/* FALL THROUGH */
>-			default:
>-			case __SI_KILL >> 16:
>-				put_user_ex(from->si_uid, &to->si_uid);
>-				break;
>-			case __SI_POLL >> 16:
>-				put_user_ex(from->si_fd, &to->si_fd);
>-				break;
>-			case __SI_TIMER >> 16:
>-				put_user_ex(from->si_overrun, &to->si_overrun);
>-				put_user_ex(ptr_to_compat(from->si_ptr),
>-					    &to->si_ptr);
>-				break;
>-				 /* This is not generated by the kernel as of now.  */
>-			case __SI_RT >> 16:
>-			case __SI_MESGQ >> 16:
>-				put_user_ex(from->si_uid, &to->si_uid);
>-				put_user_ex(from->si_int, &to->si_int);
>-				break;
>-			}
>+			err |= __put_user(from->si_utime,
>+				&to->_sifields._sigchld_x32._utime);
>+			err |= __put_user(from->si_stime,
>+				&to->_sifields._sigchld_x32._stime);
> 		}
>-	} put_user_catch(err);
>-
>+		break;
>+	case __SI_RT: /* This is not generated by the kernel as of now. */
>+	case __SI_MESGQ: /* But this is */
>+		err |= __put_user(from->si_pid, &to->si_pid);
>+		err |= __put_user(from->si_uid, &to->si_uid);
>+		/*
>+		 * Get the sigval from si_int, which matches the convention
>+		 * used in get_compat_sigevent.
>+		 */
>+		err |= __put_user(from->si_int, &to->si_int);
>+		break;
>+#ifdef __ARCH_SIGSYS
>+	case __SI_SYS:
>+		err |= __put_user(ptr_to_compat(from->si_call_addr),
>+			&to->si_call_addr);
>+		err |= __put_user(from->si_syscall, &to->si_syscall);
>+		err |= __put_user(from->si_arch, &to->si_arch);
>+		break;
>+#endif
>+	default: /* this is just in case for now ... */
>+		err |= __put_user(from->si_pid, &to->si_pid);
>+		err |= __put_user(from->si_uid, &to->si_uid);
>+		break;
>+	}
> 	return err;
> }
> 
>int copy_siginfo_from_user32(siginfo_t *to, compat_siginfo_t __user
>*from)
> {
>-	int err = 0;
>-	u32 ptr32;
>+	int err;
>+	compat_uptr_t ptr32;
>+	bool ia32 = test_thread_flag(TIF_IA32);
> 
> 	if (!access_ok(VERIFY_READ, from, sizeof(compat_siginfo_t)))
> 		return -EFAULT;
> 
>-	get_user_try {
>-		get_user_ex(to->si_signo, &from->si_signo);
>-		get_user_ex(to->si_errno, &from->si_errno);
>-		get_user_ex(to->si_code, &from->si_code);
>-
>-		get_user_ex(to->si_pid, &from->si_pid);
>-		get_user_ex(to->si_uid, &from->si_uid);
>-		get_user_ex(ptr32, &from->si_ptr);
>-		to->si_ptr = compat_ptr(ptr32);
>-	} get_user_catch(err);
>-
>+	/*
>+	 * If you change siginfo_t structure, please be sure that
>+	 * all these functions are fixed accordingly:
>+	 * copy_siginfo_to_user
>+	 * copy_siginfo_to_user32
>+	 * copy_siginfo_from_user32
>+	 * signalfd_copyinfo
>+	 * They should never copy any pad contained in the structure
>+	 * to avoid security leaks, but must copy the generic
>+	 * 3 ints plus the relevant union member.
>+	 */
>+	err = __get_user(to->si_signo, &from->si_signo);
>+	err |= __get_user(to->si_errno, &from->si_errno);
>+	err |= __get_user(to->si_code, &from->si_code);
>+	if (to->si_code < 0) {
>+		/*
>+		 * Note that the compat union may be larger than the normal one
>+		 * due to alignment. We work around this by copying any data
>+		 * that doesn't fit in the normal union into the padding before
>+		 * the union.
>+		 */
>+		BUILD_BUG_ON(sizeof(to->_sifields._pad) + sizeof(to->_pad2) !=
>+			sizeof(from->_sifields._pad));
>+		err |= __copy_from_user(to->_sifields._pad,
>+			from->_sifields._pad,
>+			sizeof(to->_sifields._pad)) ? -EFAULT : 0;
>+		err |= __copy_from_user(to->_pad2,
>+			from->_sifields._pad + SI_PAD_SIZE, sizeof(to->_pad2))
>+			? -EFAULT : 0;
>+		return err;
>+	}
>+	switch (to->si_code & __SI_MASK) {
>+	case __SI_KILL:
>+		err |= __get_user(to->si_pid, &from->si_pid);
>+		err |= __get_user(to->si_uid, &from->si_uid);
>+		break;
>+	case __SI_TIMER:
>+		err |= __get_user(to->si_tid, &from->si_tid);
>+		err |= __get_user(to->si_overrun, &from->si_overrun);
>+		/*
>+		 * Put the sigval in si_int, which matches the convention
>+		 * used in get_compat_sigevent.
>+		 */
>+		to->si_ptr = NULL; /* Avoid uninitialized bits in the union */
>+		err |= __get_user(to->si_int, &from->si_int);
>+		break;
>+	case __SI_POLL:
>+		err |= __get_user(to->si_band, &from->si_band);
>+		err |= __get_user(to->si_fd, &from->si_fd);
>+		break;
>+	case __SI_FAULT:
>+		err |= __get_user(ptr32, &from->si_addr);
>+		to->si_addr = compat_ptr(ptr32);
>+#ifdef __ARCH_SI_TRAPNO
>+		err |= __get_user(to->si_trapno, &from->si_trapno);
>+#endif
>+		err |= __get_user(to->si_addr_lsb, &from->si_addr_lsb);
>+		err |= __get_user(ptr32, &from->si_lower);
>+		to->si_lower = compat_ptr(ptr32);
>+		err |= __get_user(ptr32, &from->si_upper);
>+		to->si_upper = compat_ptr(ptr32);
>+		break;
>+	case __SI_CHLD:
>+		err |= __get_user(to->si_pid, &from->si_pid);
>+		err |= __get_user(to->si_uid, &from->si_uid);
>+		err |= __get_user(to->si_status, &from->si_status);
>+		if (ia32) {
>+			err |= __get_user(to->si_utime, &from->si_utime);
>+			err |= __get_user(to->si_stime, &from->si_stime);
>+		} else {
>+			err |= __get_user(to->si_utime,
>+				&from->_sifields._sigchld_x32._utime);
>+			err |= __get_user(to->si_stime,
>+				&from->_sifields._sigchld_x32._stime);
>+		}
>+		break;
>+	case __SI_RT: /* This is not generated by the kernel as of now. */
>+	case __SI_MESGQ: /* But this is */
>+		err |= __get_user(to->si_pid, &from->si_pid);
>+		err |= __get_user(to->si_uid, &from->si_uid);
>+		/*
>+		 * Put the sigval in si_int, which matches the convention
>+		 * used in get_compat_sigevent.
>+		 */
>+		to->si_ptr = NULL; /* Avoid uninitialized bits in the union */
>+		err |= __get_user(to->si_int, &from->si_int);
>+		break;
>+#ifdef __ARCH_SIGSYS
>+	case __SI_SYS:
>+		err |= __get_user(ptr32, &from->si_call_addr);
>+		to->si_call_addr = compat_ptr(ptr32);
>+		err |= __get_user(to->si_syscall, &from->si_syscall);
>+		err |= __get_user(to->si_arch, &from->si_arch);
>+		break;
>+#endif
>+	default: /* this is just in case for now ... */
>+		err |= __get_user(to->si_pid, &from->si_pid);
>+		err |= __get_user(to->si_uid, &from->si_uid);
>+		break;
>+	}
> 	return err;
> }

Once again, NAK on removing get/user_put_ex for performance reasons (10x slowdown on SMAP-enabled hardware.)

DO NOT resubmit without addressing that issue.  Period.
-- 
Sent from my Android device with K-9 Mail. Please excuse my brevity.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/