* [PATCH v2 19/20] signalfd: Fix some issues in signalfd_copyinfo
2015-11-05 0:50 [PATCH v2 00/20] Fix handling of compat_siginfo_t Amanieu d'Antras
@ 2015-11-05 0:50 ` Amanieu d'Antras
2015-11-08 5:09 ` [PATCH v2 00/20] Fix handling of compat_siginfo_t Andy Lutomirski
1 sibling, 0 replies; 4+ messages in thread
From: Amanieu d'Antras @ 2015-11-05 0:50 UTC (permalink / raw)
To: linux-kernel
Cc: Oleg Nesterov, Amanieu d'Antras, Alexander Viro,
linux-fsdevel
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
^ permalink raw reply related [flat|nested] 4+ messages in thread* Re: [PATCH v2 00/20] Fix handling of compat_siginfo_t
2015-11-05 0:50 [PATCH v2 00/20] Fix handling of compat_siginfo_t Amanieu d'Antras
2015-11-05 0:50 ` [PATCH v2 19/20] signalfd: Fix some issues in signalfd_copyinfo Amanieu d'Antras
@ 2015-11-08 5:09 ` Andy Lutomirski
2015-11-09 15:12 ` Oleg Nesterov
1 sibling, 1 reply; 4+ messages in thread
From: Andy Lutomirski @ 2015-11-08 5:09 UTC (permalink / raw)
To: Amanieu d'Antras
Cc: linux-kernel@vger.kernel.org, Oleg Nesterov,
linux-arm-kernel@lists.infradead.org, Linux MIPS Mailing List,
linux-parisc, linuxppc-dev, linux-s390@vger.kernel.org,
sparclinux, Linux FS Devel, linux-arch, Linux API, Kenton Varda
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
^ permalink raw reply [flat|nested] 4+ messages in thread