Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1247163 > unrolled thread

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

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH 04/20] x86: Rewrite copy_siginfo_{to,from}_user32 Amanieu d'Antras <amanieu@gmail.com> - 2015-10-14 23:10 +0200
    Re: [PATCH 04/20] x86: Rewrite copy_siginfo_{to,from}_user32 kbuild test robot <lkp@intel.com> - 2015-10-15 00:50 +0200
    Re: [PATCH 04/20] x86: Rewrite copy_siginfo_{to,from}_user32 Oleg Nesterov <oleg@redhat.com> - 2015-10-15 20:50 +0200
      Re: [PATCH 04/20] x86: Rewrite copy_siginfo_{to,from}_user32 "Amanieu d'Antras" <amanieu@gmail.com> - 2015-10-15 21:00 +0200

#1247163 — [PATCH 04/20] x86: Rewrite copy_siginfo_{to,from}_user32

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.

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@gmail.com>
---
 arch/x86/kernel/signal_compat.c | 269 +++++++++++++++++++++++++++++-----------
 1 file changed, 194 insertions(+), 75 deletions(-)

diff --git a/arch/x86/kernel/signal_compat.c b/arch/x86/kernel/signal_compat.c
index dc3c0b1..e6f7e76 100644
--- a/arch/x86/kernel/signal_compat.c
+++ b/arch/x86/kernel/signal_compat.c
@@ -1,95 +1,214 @@
 #include <linux/compat.h>
 #include <linux/uaccess.h>
 
-int copy_siginfo_to_user32(compat_siginfo_t __user *to, const siginfo_t *from)
+int copy_siginfo_to_user32(struct compat_siginfo __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)))
+	if (!access_ok(VERIFY_WRITE, to, sizeof(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) {
+		err |= __copy_to_user(to->_sifields._pad, from->_sifields._pad, SI_PAD_SIZE * sizeof(int))
+			? -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 copy_siginfo_from_user32(siginfo_t *to, struct compat_siginfo __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)))
+	if (!access_ok(VERIFY_READ, from, sizeof(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. The copying here causes us to lose the last 4 bytes of
+		 * data, but this shouldn't be too much of a problem in practice.
+		 */
+		err |= __copy_from_user(to->_sifields._pad, from->_sifields._pad, SI_PAD_SIZE * sizeof(int))
+			? -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 = 0; /* 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 = 0; /* 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;
 }
-- 
2.6.1

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [next] | [standalone]

#1247234

Hi Amanieu,

[auto build test WARNING on arm64/for-next/core -- if it's inappropriate base, please suggest rules for selecting the more suitable base]

url:    https://github.com/0day-ci/linux/commits/Amanieu-d-Antras/Fix-handling-of-compat_siginfo_t/20151015-051137
reproduce:
        # apt-get install sparse
        make ARCH=x86_64 allmodconfig
        make C=1 CF=-D__CHECK_ENDIAN__


sparse warnings: (new ones prefixed by >>)

>> arch/x86/kernel/signal_compat.c:158:30: sparse: Using plain integer as NULL pointer
   arch/x86/kernel/signal_compat.c:197:30: sparse: Using plain integer as NULL pointer

vim +158 arch/x86/kernel/signal_compat.c

   142			err |= __copy_from_user(to->_sifields._pad, from->_sifields._pad, SI_PAD_SIZE * sizeof(int))
   143				? -EFAULT : 0;
   144			return err;
   145		}
   146		switch (to->si_code & __SI_MASK) {
   147		case __SI_KILL:
   148			err |= __get_user(to->si_pid, &from->si_pid);
   149			err |= __get_user(to->si_uid, &from->si_uid);
   150			break;
   151		case __SI_TIMER:
   152			err |= __get_user(to->si_tid, &from->si_tid);
   153			err |= __get_user(to->si_overrun, &from->si_overrun);
   154			/*
   155			 * Put the sigval in si_int, which matches the convention
   156			 * used in get_compat_sigevent.
   157			 */
 > 158			to->si_ptr = 0; /* Avoid uninitialized bits in the union */
   159			err |= __get_user(to->si_int, &from->si_int);
   160			break;
   161		case __SI_POLL:
   162			err |= __get_user(to->si_band, &from->si_band);
   163			err |= __get_user(to->si_fd, &from->si_fd);
   164			break;
   165		case __SI_FAULT:
   166			err |= __get_user(ptr32, &from->si_addr);

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1248076

OOH ;) I'll try to look at this patch and the changes in the generic
code later. A couple of nits right now.

Please CC x86 maintainers, not only x86@kernel.org.

Please do not remove get/put_user_ex from this code. And this reminds
me that we can improve *user_try/*user_catch ...

On 10/14, Amanieu d'Antras wrote:
>
> -int copy_siginfo_to_user32(compat_siginfo_t __user *to, const siginfo_t *from)
> +int copy_siginfo_to_user32(struct compat_siginfo __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)))
> +	if (!access_ok(VERIFY_WRITE, to, sizeof(siginfo_t)))

Why? This looks wrong.

> +	if (from->si_code < 0) {
> +		err |= __copy_to_user(to->_sifields._pad, from->_sifields._pad, SI_PAD_SIZE * sizeof(int))
> +			? -EFAULT : 0;
> +		return err;

I think you should split this patch. And this change (don't interpet,
just copy) should go as a separate change.

> +	switch (from->si_code & __SI_MASK) {
> +	case __SI_KILL:

I agree, this looks better than ">> 16", but I'd suggest a separate
change too.

[...snip...]

the rest looks unreviewable because you didn't split it and because
you removed try/catch ;) The same for copy-from-user.

Please help us to understand these changes and make the more reviewable
patches if possible. Personally I think you have a point.

Oleg.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1248086

On Thu, Oct 15, 2015 at 7:41 PM, Oleg Nesterov <oleg@redhat.com> wrote:
> OOH ;) I'll try to look at this patch and the changes in the generic
> code later. A couple of nits right now.
>
> Please CC x86 maintainers, not only x86@kernel.org.
>
> Please do not remove get/put_user_ex from this code. And this reminds
> me that we can improve *user_try/*user_catch ...
>
> [...snip...]
>
> the rest looks unreviewable because you didn't split it and because
> you removed try/catch ;) The same for copy-from-user.
>
> Please help us to understand these changes and make the more reviewable
> patches if possible. Personally I think you have a point.

What I did here was replace the old x86-specific version with the
generic version, which I then modified to support x32. If you compare
it with the generic version, the only difference is the addition of
the ia32 flag. The intent was to make it as close as possible to the
generic version, which makes it easier to update both at the same time
when a new siginfo_t fields is added.

This is also why I didn't use put_user_try/get_user_try: it would make
the x86 version unnecessarily diverge from the generic version, since
those macros only exist in the x86 architecture.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web