[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <CAGXu5jKtrJ6Nmses_pM-qkXAkOPXAxwT+V3B+omqu0tx4xEh8w@mail.gmail.com>
Date: Mon, 19 Feb 2018 15:55:27 -0800
From: Kees Cook <keescook@...omium.org>
To: Matt Redfearn <matt.redfearn@...s.com>
Cc: "Dmitry V . Levin" <ldv@...linux.org>, Al Viro <viro@...iv.linux.org.uk>,
Kernel Hardening <kernel-hardening@...ts.openwall.com>,
Linux MIPS Mailing List <linux-mips@...ux-mips.org>
Subject: Re: [PATCH] signals: Move put_compat_sigset to compat.h to silence
hardened usercopy
On Mon, Feb 19, 2018 at 8:55 AM, Matt Redfearn <matt.redfearn@...s.com> wrote:
> Since commit afcc90f8621e ("usercopy: WARN() on slab cache usercopy
> region violations"), MIPS systems booting with a compat root filesystem
> emit a warning when copying compat siginfo to userspace:
>
> WARNING: CPU: 0 PID: 953 at mm/usercopy.c:81 usercopy_warn+0x98/0xe8
> Bad or missing usercopy whitelist? Kernel memory exposure attempt
> detected from SLAB object 'task_struct' (offset 1432, size 16)!
> Modules linked in:
> CPU: 0 PID: 953 Comm: S01logging Not tainted 4.16.0-rc2 #10
> Stack : ffffffff808c0000 0000000000000000 0000000000000001 65ac85163f3bdc4a
> 65ac85163f3bdc4a 0000000000000000 90000000ff667ab8 ffffffff808c0000
> 00000000000003f8 ffffffff808d0000 00000000000000d1 0000000000000000
> 000000000000003c 0000000000000000 ffffffff808c8ca8 ffffffff808d0000
> ffffffff808d0000 ffffffff80810000 fffffc0000000000 ffffffff80785c30
> 0000000000000009 0000000000000051 90000000ff667eb0 90000000ff667db0
> 000000007fe0d938 0000000000000018 ffffffff80449958 0000000020052798
> ffffffff808c0000 90000000ff664000 90000000ff667ab0 00000000100c0000
> ffffffff80698810 0000000000000000 0000000000000000 0000000000000000
> 0000000000000000 0000000000000000 ffffffff8010d02c 65ac85163f3bdc4a
> ...
> Call Trace:
> [<ffffffff8010d02c>] show_stack+0x9c/0x130
> [<ffffffff80698810>] dump_stack+0x90/0xd0
> [<ffffffff80137b78>] __warn+0x100/0x118
> [<ffffffff80137bdc>] warn_slowpath_fmt+0x4c/0x70
> [<ffffffff8021e4a8>] usercopy_warn+0x98/0xe8
> [<ffffffff8021e68c>] __check_object_size+0xfc/0x250
> [<ffffffff801bbfb8>] put_compat_sigset+0x30/0x88
> [<ffffffff8011af24>] setup_rt_frame_n32+0xc4/0x160
> [<ffffffff8010b8b4>] do_signal+0x19c/0x230
> [<ffffffff8010c408>] do_notify_resume+0x60/0x78
> [<ffffffff80106f50>] work_notifysig+0x10/0x18
> ---[ end trace 88fffbf69147f48a ]---
>
> Commit 5905429ad856 ("fork: Provide usercopy whitelisting for
> task_struct") noted that:
>
> "While the blocked and saved_sigmask fields of task_struct are copied to
> userspace (via sigmask_to_save() and setup_rt_frame()), it is always
> copied with a static length (i.e. sizeof(sigset_t))."
>
> However, this is not true in the case of compat signals, whose sigset
> is copied by put_compat_sigset and receives size as an argument.
>
> At most call sites, put_compat_sigset is copying a sigset from the
> current task_struct. This triggers a warning when
> CONFIG_HARDENED_USERCOPY is active. However, by marking this function as
> static inline, the warning can be avoided because in all of these cases
> the size is constant at compile time, which is allowed. The only site
> where this is not the case is handling the rt_sigpending syscall, but
> there the copy is being made from a stack local variable so does not
> trigger the warning.
>
> Move put_compat_sigset to compat.h, and mark it static inline. This
> fixes the WARN on MIPS.
>
> Fixes: afcc90f8621e ("usercopy: WARN() on slab cache usercopy region violations")
> Signed-off-by: Matt Redfearn <matt.redfearn@...s.com>
Thanks for the catch and fix!
Acked-by: Kees Cook <keescook@...omium.org>
-Kees
> ---
>
> include/linux/compat.h | 26 ++++++++++++++++++++++++--
> kernel/compat.c | 19 -------------------
> 2 files changed, 24 insertions(+), 21 deletions(-)
>
> diff --git a/include/linux/compat.h b/include/linux/compat.h
> index 8a9643857c4a..c4139c7a0de0 100644
> --- a/include/linux/compat.h
> +++ b/include/linux/compat.h
> @@ -17,6 +17,7 @@
> #include <linux/if.h>
> #include <linux/fs.h>
> #include <linux/aio_abi.h> /* for aio_context_t */
> +#include <linux/uaccess.h>
> #include <linux/unistd.h>
>
> #include <asm/compat.h>
> @@ -550,8 +551,29 @@ asmlinkage long compat_sys_settimeofday(struct compat_timeval __user *tv,
> asmlinkage long compat_sys_adjtimex(struct compat_timex __user *utp);
>
> extern int get_compat_sigset(sigset_t *set, const compat_sigset_t __user *compat);
> -extern int put_compat_sigset(compat_sigset_t __user *compat,
> - const sigset_t *set, unsigned int size);
> +
> +/*
> + * Defined inline such that size can be compile time constant, which avoids
> + * CONFIG_HARDENED_USERCOPY complaining about copies from task_struct
> + */
> +static inline int
> +put_compat_sigset(compat_sigset_t __user *compat, const sigset_t *set,
> + unsigned int size)
> +{
> + /* size <= sizeof(compat_sigset_t) <= sizeof(sigset_t) */
> +#ifdef __BIG_ENDIAN
> + compat_sigset_t v;
> + switch (_NSIG_WORDS) {
> + case 4: v.sig[7] = (set->sig[3] >> 32); v.sig[6] = set->sig[3];
> + case 3: v.sig[5] = (set->sig[2] >> 32); v.sig[4] = set->sig[2];
> + case 2: v.sig[3] = (set->sig[1] >> 32); v.sig[2] = set->sig[1];
> + case 1: v.sig[1] = (set->sig[0] >> 32); v.sig[0] = set->sig[0];
> + }
> + return copy_to_user(compat, &v, size) ? -EFAULT : 0;
> +#else
> + return copy_to_user(compat, set, size) ? -EFAULT : 0;
> +#endif
> +}
>
> asmlinkage long compat_sys_migrate_pages(compat_pid_t pid,
> compat_ulong_t maxnode, const compat_ulong_t __user *old_nodes,
> diff --git a/kernel/compat.c b/kernel/compat.c
> index 3247fe761f60..3f5fa8902e7d 100644
> --- a/kernel/compat.c
> +++ b/kernel/compat.c
> @@ -488,25 +488,6 @@ get_compat_sigset(sigset_t *set, const compat_sigset_t __user *compat)
> }
> EXPORT_SYMBOL_GPL(get_compat_sigset);
>
> -int
> -put_compat_sigset(compat_sigset_t __user *compat, const sigset_t *set,
> - unsigned int size)
> -{
> - /* size <= sizeof(compat_sigset_t) <= sizeof(sigset_t) */
> -#ifdef __BIG_ENDIAN
> - compat_sigset_t v;
> - switch (_NSIG_WORDS) {
> - case 4: v.sig[7] = (set->sig[3] >> 32); v.sig[6] = set->sig[3];
> - case 3: v.sig[5] = (set->sig[2] >> 32); v.sig[4] = set->sig[2];
> - case 2: v.sig[3] = (set->sig[1] >> 32); v.sig[2] = set->sig[1];
> - case 1: v.sig[1] = (set->sig[0] >> 32); v.sig[0] = set->sig[0];
> - }
> - return copy_to_user(compat, &v, size) ? -EFAULT : 0;
> -#else
> - return copy_to_user(compat, set, size) ? -EFAULT : 0;
> -#endif
> -}
> -
> #ifdef CONFIG_NUMA
> COMPAT_SYSCALL_DEFINE6(move_pages, pid_t, pid, compat_ulong_t, nr_pages,
> compat_uptr_t __user *, pages32,
> --
> 2.7.4
>
--
Kees Cook
Pixel Security
Powered by blists - more mailing lists
Confused about mailing lists and their use?
Read about mailing lists on Wikipedia
and check out these
guidelines on proper formatting of your messages.
