| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <9A40E746-3BEA-4BCB-9860-A5789AE56B9C@zytor.com>
Date: Wed, 04 Nov 2015 18:29:34 -0800
From: "H. Peter Anvin" <hpa@...or.com>
To: "Amanieu d'Antras" <amanieu@...il.com>,
linux-kernel@...r.kernel.org
CC: Oleg Nesterov <oleg@...hat.com>,
Thomas Gleixner <tglx@...utronix.de>,
Ingo Molnar <mingo@...hat.com>, x86@...nel.org
Subject: Re: [PATCH v2 04/20] x86: Rewrite copy_siginfo_{to,from}_user32
On November 4, 2015 4:50:23 PM PST, Amanieu d'Antras <amanieu@...il.com> 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@...il.com>
>---
>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@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists