[PATCH v2 00/20] Fix handling of compat_siginfo_t

[PATCH v2 00/20] Fix handling of compat_siginfo_t

[Amanieu d'Antras]

The current handling of compat_siginfo_t is a mess: each architecture has its
own implementation, all of which are incorrect in different ways. This patch
series replaces all of the arch-specific versions with a single generic one that
is guaranteed to produce the same results as a 32-bit kernel.

Most architectures are able to use the generic compat_siginfo_t, except x86 and
MIPS. MIPS uses a slightly different compat_siginfo_t structure for ABI reasons
but can still use the generic copy_siginfo_{to,from}_user32. x86 can't use the
generic versions because it needs special handling for __SI_CHLD for x32 tasks.

One issue that isn't resolved in this series is sending signals between a 32-bit
process and 64-bit process. Sending a si_int will work correctly, but a si_ptr
value will likely get corrupted due to the different layouts of the 32-bit and
64-bit siginfo_t structures.

signalfd_copyinfo was also modified to properly generate data for compat tasks.
In particular the ssi_ptr and ssi_data members need to be sign-extended to 64
bits rather than zero-extended, since that is the behavior in 32-bit kernels.

This series has been tested on x86_64 and arm64.

Changes since v1:
- Properly copy padding bytes and avoid leaking uninitialized data to userspace
- Fixed compile errors on mips and powerpc
- Fixed some compiler warnings
- Fixed some formatting issues

Amanieu d'Antras (20):
  compat: Add generic compat_siginfo_t
  compat: Add generic copy_siginfo_{to,from}_user32
  x86: Update compat_siginfo_t to be closer to the generic version
  x86: Rewrite copy_siginfo_{to,from}_user32
  mips: Clean up compat_siginfo_t
  mips: Use generic copy_siginfo_{to,from}_user32
  arm64: Use generic compat_siginfo_t
  arm64: Use generic copy_siginfo_{to,from}_user32
  parisc: Use generic compat_siginfo_t
  parsic: Use generic copy_siginfo_{to,from}_user32
  s390: Use generic compat_siginfo_t
  s390: Use generic copy_siginfo_{to,from}_user32
  powerpc: Use generic compat_siginfo_t
  powerpc: Use generic copy_siginfo_{to,from}_user32
  tile: Use generic compat_siginfo_t
  tile: Use generic copy_siginfo_{to,from}_user32
  sparc: Use generic compat_siginfo_t
  sparc: Use generic copy_siginfo_{to,from}_user32
  signalfd: Fix some issues in signalfd_copyinfo
  signal: Remove unnecessary zero-initialization of siginfo_t

 arch/arm64/include/asm/compat.h    |  59 --------
 arch/arm64/kernel/signal32.c       |  85 -----------
 arch/mips/include/asm/compat.h     |  63 ++++----
 arch/mips/kernel/signal32.c        |  62 --------
 arch/parisc/include/asm/compat.h   |  52 -------
 arch/parisc/kernel/signal32.c      | 102 -------------
 arch/powerpc/include/asm/compat.h  |  60 --------
 arch/powerpc/kernel/signal_32.c    |  72 +---------
 arch/s390/include/asm/compat.h     |  51 -------
 arch/s390/kernel/compat_signal.c   | 102 -------------
 arch/sparc/include/asm/compat.h    |  54 -------
 arch/sparc/kernel/signal32.c       |  69 ---------
 arch/tile/include/asm/compat.h     |  57 --------
 arch/tile/kernel/compat_signal.c   |  75 ----------
 arch/x86/include/asm/compat.h      |  39 +++--
 arch/x86/kernel/signal_compat.c    | 285 ++++++++++++++++++++++++++++---------
 fs/signalfd.c                      |  58 +++++---
 include/linux/compat.h             |  66 ++++++++-
 include/uapi/asm-generic/siginfo.h |   1 +
 kernel/compat.c                    | 224 +++++++++++++++++++++++++++++
 kernel/ptrace.c                    |   1 -
 kernel/signal.c                    |  16 ++-
 22 files changed, 615 insertions(+), 1038 deletions(-)

-- 
2.6.2

[PATCH v2 19/20] signalfd: Fix some issues in signalfd_copyinfo

[Amanieu d'Antras]

There are several issues here:
1) The value of ssi_ptr was incorrect for compat tasks. It was
   previously copied directly from si_ptr, which contains garbage
   for 32-bit processes, especially on big-endian architectures.

2) A SIGSYS would cause various fields to be filled with incorrect
   values. SIGSYS fields are not in signalfd_siginfo, and it should
   avoid filling in unrelated fields.

3) ssi_ptr and ssi_int should not be filled in for any unrecognized
   si_code, but only for those generated by sigqueue. The si_ptr
   and si_int fields in siginfo_t may not be initialized otherwise.

4) ssi_ptr and ssi_addr values for compat tasks did not match those
   generated by 32-bit kernels. The values need to be sign-extended
   to 64 bits rather than zero-extended.

Signed-off-by: Amanieu d'Antras <amanieu@gmail.com>
---
 fs/signalfd.c | 58 ++++++++++++++++++++++++++++++++++++++++++----------------
 1 file changed, 42 insertions(+), 16 deletions(-)

diff --git a/fs/signalfd.c b/fs/signalfd.c
index 270221f..1c0bde7 100644
--- a/fs/signalfd.c
+++ b/fs/signalfd.c
@@ -80,39 +80,66 @@ static unsigned int signalfd_poll(struct file *file, poll_table *wait)
 static int signalfd_copyinfo(struct signalfd_siginfo __user *uinfo,
 			     siginfo_t const *kinfo)
 {
-	long err;
+	long err, ssi_ptr, ssi_addr;
 
 	BUILD_BUG_ON(sizeof(struct signalfd_siginfo) != 128);
 
 	/*
+	 * ssi_ptr for a compat task should be sourced from si_int instead
+	 * of si_ptr since that is what copy_siginfo_from_user32 and
+	 * get_compat_sigevent use. 32-bit pointer values are sign-extended
+	 * to 64 bits when written to ssi_ptr, which matches the behavior of
+	 * 32-bit kernels.
+	 */
+	ssi_ptr = is_compat_task() ? kinfo->si_int : (long) kinfo->si_ptr;
+
+	/*
 	 * Unused members should be zero ...
 	 */
 	err = __clear_user(uinfo, sizeof(*uinfo));
 
 	/*
-	 * If you change siginfo_t structure, please be sure
-	 * this code is fixed accordingly.
+	 * 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(kinfo->si_signo, &uinfo->ssi_signo);
 	err |= __put_user(kinfo->si_errno, &uinfo->ssi_errno);
 	err |= __put_user((short) kinfo->si_code, &uinfo->ssi_code);
+	if (kinfo->si_code < 0) {
+		/* Grab some standard fields for sigqueue()-generated signals */
+		err |= __put_user(kinfo->si_pid, &uinfo->ssi_pid);
+		err |= __put_user(kinfo->si_uid, &uinfo->ssi_uid);
+		err |= __put_user(ssi_ptr, &uinfo->ssi_ptr);
+		err |= __put_user(kinfo->si_int, &uinfo->ssi_int);
+		return err ? -EFAULT : sizeof(*uinfo);
+	}
 	switch (kinfo->si_code & __SI_MASK) {
 	case __SI_KILL:
 		err |= __put_user(kinfo->si_pid, &uinfo->ssi_pid);
 		err |= __put_user(kinfo->si_uid, &uinfo->ssi_uid);
 		break;
 	case __SI_TIMER:
-		 err |= __put_user(kinfo->si_tid, &uinfo->ssi_tid);
-		 err |= __put_user(kinfo->si_overrun, &uinfo->ssi_overrun);
-		 err |= __put_user((long) kinfo->si_ptr, &uinfo->ssi_ptr);
-		 err |= __put_user(kinfo->si_int, &uinfo->ssi_int);
+		err |= __put_user(kinfo->si_tid, &uinfo->ssi_tid);
+		err |= __put_user(kinfo->si_overrun, &uinfo->ssi_overrun);
+		err |= __put_user(ssi_ptr, &uinfo->ssi_ptr);
+		err |= __put_user(kinfo->si_int, &uinfo->ssi_int);
 		break;
 	case __SI_POLL:
 		err |= __put_user(kinfo->si_band, &uinfo->ssi_band);
 		err |= __put_user(kinfo->si_fd, &uinfo->ssi_fd);
 		break;
 	case __SI_FAULT:
-		err |= __put_user((long) kinfo->si_addr, &uinfo->ssi_addr);
+		/* Ensure that ssi_addr is sign-extended to 64 bits */
+		ssi_addr = is_compat_task() ? (int)(long) kinfo->si_addr :
+			(long) kinfo->si_addr;
+		err |= __put_user(ssi_addr, &uinfo->ssi_addr);
 #ifdef __ARCH_SI_TRAPNO
 		err |= __put_user(kinfo->si_trapno, &uinfo->ssi_trapno);
 #endif
@@ -139,21 +166,20 @@ static int signalfd_copyinfo(struct signalfd_siginfo __user *uinfo,
 	case __SI_MESGQ: /* But this is */
 		err |= __put_user(kinfo->si_pid, &uinfo->ssi_pid);
 		err |= __put_user(kinfo->si_uid, &uinfo->ssi_uid);
-		err |= __put_user((long) kinfo->si_ptr, &uinfo->ssi_ptr);
+		err |= __put_user(ssi_ptr, &uinfo->ssi_ptr);
 		err |= __put_user(kinfo->si_int, &uinfo->ssi_int);
 		break;
-	default:
-		/*
-		 * This case catches also the signals queued by sigqueue().
-		 */
+#ifdef __ARCH_SIGSYS
+	case __SI_SYS: /* SIGSYS fields are not in signalfd_siginfo */
+		break;
+#endif
+	default: /* this is just in case for now ... */
 		err |= __put_user(kinfo->si_pid, &uinfo->ssi_pid);
 		err |= __put_user(kinfo->si_uid, &uinfo->ssi_uid);
-		err |= __put_user((long) kinfo->si_ptr, &uinfo->ssi_ptr);
-		err |= __put_user(kinfo->si_int, &uinfo->ssi_int);
 		break;
 	}
 
-	return err ? -EFAULT: sizeof(*uinfo);
+	return err ? -EFAULT : sizeof(*uinfo);
 }
 
 static ssize_t signalfd_dequeue(struct signalfd_ctx *ctx, siginfo_t *info,
-- 
2.6.2

Re: [PATCH v2 00/20] Fix handling of compat_siginfo_t

[Andy Lutomirski]

On Wed, Nov 4, 2015 at 4:50 PM, Amanieu d'Antras <amanieu@gmail.com> wrote:
> One issue that isn't resolved in this series is sending signals between a 32-bit
> process and 64-bit process. Sending a si_int will work correctly, but a si_ptr
> value will likely get corrupted due to the different layouts of the 32-bit and
> 64-bit siginfo_t structures.

This is so screwed up it's not even funny.

A 64-bit big-endian compat calls rt_sigqueueinfo.  It passes in (among
other things) a sigval_t.  The kernel can choose to interpret it as a
pointer (call it p) or an integer (call it i).  Then (unsigned long)p
= (i<<32) | [something].  If the number was an integer to begin with
*and* user code zeroed out the mess first, then [something] will be 0.
Regardless, p != i unless they're both zero.

If the result gets delivered to a signalfd, then it's plausible that
everything could work.  If it gets delivered to a 64-bit siginfo, then
all is well because it's in exactly the same screwed up state it was
in when the signal gets sent.

If, however, it's delivered to a compat task, wtf is the kernel
supposed to do?  We're effectively supposed to convert a 64-bit
sigval_t to a 32-bit sigval_t.  On a little-endian architecture, we
can fudge it because it doesn't really matter whether we consider the
pointer or the int to be authoritative.  I think that, on big-endian,
we're screwed.

BTW, x86 has its own set of screwups here.  Somehow cr2 and error_code
ended up as part of ucontext instead of siginfo, which makes
absolutely no sense to me and bloats task_struct.

--Andy

Re: [PATCH v2 00/20] Fix handling of compat_siginfo_t

[Oleg Nesterov]

On 11/07, Andy Lutomirski wrote:
>
> On Wed, Nov 4, 2015 at 4:50 PM, Amanieu d'Antras <amanieu@gmail.com> wrote:
> > One issue that isn't resolved in this series is sending signals between a 32-bit
> > process and 64-bit process. Sending a si_int will work correctly, but a si_ptr
> > value will likely get corrupted due to the different layouts of the 32-bit and
> > 64-bit siginfo_t structures.
>
> This is so screwed up it's not even funny.

Agreed,

> A 64-bit big-endian compat calls rt_sigqueueinfo.  It passes in (among
> other things) a sigval_t.  The kernel can choose to interpret it

I always thought that the kernel should not interpret it at all. And indeed,
copy_siginfo_to_user() does

	if (from->si_code < 0)
		return __copy_to_user(to, from, sizeof(siginfo_t))

probably copy_siginfo_to_user32() should do something similar, at least
it should not truncate ->si_code it it is less than zero.

Not sure what signalfd_copyinfo() should do.

But perhaps I was wrong, I failed to find man sigqueueinfo, and man
sigqueue() documents that it passes sigval_t.


> BTW, x86 has its own set of screwups here.  Somehow cr2 and error_code
> ended up as part of ucontext instead of siginfo, which makes
> absolutely no sense to me and bloats task_struct.

Yes, and probably ->ip should have been the part of siginfo too. Say,
if you get SIGBUS you can't trust sc->ip if another signal was dequeued
before SIGBUS, in this case sc->ip will point to the handler of that
another signal. That is why we have SYNCHRONOUS_MASK and it helps, but
still this doesn't look nice.

Oleg.