From mboxrd@z Thu Jan  1 00:00:00 1970
MIME-Version: 1.0
Sender: keescook@google.com
In-Reply-To: <1519059306-30135-1-git-send-email-matt.redfearn@mips.com>
References: <1515636190-24061-1-git-send-email-keescook@chromium.org> <1519059306-30135-1-git-send-email-matt.redfearn@mips.com>
From: Kees Cook <keescook@chromium.org>
Date: Mon, 19 Feb 2018 15:55:27 -0800
Message-ID: <CAGXu5jKtrJ6Nmses_pM-qkXAkOPXAxwT+V3B+omqu0tx4xEh8w@mail.gmail.com>
Subject: Re: [PATCH] signals: Move put_compat_sigset to compat.h to silence
 hardened usercopy
Content-Type: text/plain; charset="UTF-8"
To: Matt Redfearn <matt.redfearn@mips.com>
Cc: "Dmitry V . Levin" <ldv@altlinux.org>, Al Viro <viro@zeniv.linux.org.uk>, Kernel Hardening <kernel-hardening@lists.openwall.com>, Linux MIPS Mailing List <linux-mips@linux-mips.org>
List-ID: <kernel-hardening.lists.openwall.com>

On Mon, Feb 19, 2018 at 8:55 AM, Matt Redfearn <matt.redfearn@mips.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@mips.com>

Thanks for the catch and fix!

Acked-by: Kees Cook <keescook@chromium.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