From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1031626AbbKEC3z (ORCPT ); Wed, 4 Nov 2015 21:29:55 -0500 Received: from terminus.zytor.com ([198.137.202.10]:51805 "EHLO mail.zytor.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1030403AbbKEC3y (ORCPT ); Wed, 4 Nov 2015 21:29:54 -0500 User-Agent: K-9 Mail for Android In-Reply-To: <1446684640-4112-5-git-send-email-amanieu@gmail.com> References: <1446684640-4112-1-git-send-email-amanieu@gmail.com> <1446684640-4112-5-git-send-email-amanieu@gmail.com> MIME-Version: 1.0 Content-Transfer-Encoding: 8bit Content-Type: text/plain; charset=UTF-8 Subject: Re: [PATCH v2 04/20] x86: Rewrite copy_siginfo_{to,from}_user32 From: "H. Peter Anvin" Date: Wed, 04 Nov 2015 18:29:34 -0800 To: "Amanieu d'Antras" , linux-kernel@vger.kernel.org CC: Oleg Nesterov , Thomas Gleixner , Ingo Molnar , x86@kernel.org Message-ID: <9A40E746-3BEA-4BCB-9860-A5789AE56B9C@zytor.com> Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On November 4, 2015 4:50:23 PM PST, Amanieu d'Antras 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 >--- >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.