kernel-hardening.lists.openwall.com archive mirror
 help / color / mirror / Atom feed
From: Kees Cook <keescook@chromium.org>
To: Dave Martin <Dave.Martin@arm.com>
Cc: Mark Rutland <Mark.Rutland@arm.com>,
	"kernel-hardening@lists.openwall.com"
	<kernel-hardening@lists.openwall.com>,
	Catalin Marinas <Catalin.Marinas@arm.com>,
	Will Deacon <Will.Deacon@arm.com>,
	"linux-arm-kernel@lists.infradead.org"
	<linux-arm-kernel@lists.infradead.org>
Subject: Re: [PATCH 33/38] arm64: Implement thread_struct whitelist for hardened usercopy
Date: Mon, 26 Mar 2018 10:41:09 -0700	[thread overview]
Message-ID: <CAGXu5jL1S+CFEruBOaN_yJa7cY+a9gr8cBQgFcYowW=gT5VfSA@mail.gmail.com> (raw)
In-Reply-To: <20180326162158.GH16308@e103592.cambridge.arm.com>

On Mon, Mar 26, 2018 at 9:22 AM, Dave Martin <Dave.Martin@arm.com> wrote:
> [Dropped most of the original Cc list, since most people are unlikely to
> care about this thread archaeology.]
>
> On Mon, Jan 15, 2018 at 12:06:17PM -0800, Kees Cook wrote:
>> On Mon, Jan 15, 2018 at 4:24 AM, Dave P Martin <Dave.Martin@arm.com> wrote:
>> > On Thu, Jan 11, 2018 at 02:03:05AM +0000, Kees Cook wrote:
>> >> This whitelists the FPU register state portion of the thread_struct for
>> >> copying to userspace, instead of the default entire structure.
>> >>
>> >> Cc: Catalin Marinas <catalin.marinas@arm.com>
>> >> Cc: Will Deacon <will.deacon@arm.com>
>> >> Cc: Christian Borntraeger <borntraeger@de.ibm.com>
>> >> Cc: Ingo Molnar <mingo@kernel.org>
>> >> Cc: James Morse <james.morse@arm.com>
>> >> Cc: "Peter Zijlstra (Intel)" <peterz@infradead.org>
>> >> Cc: Dave Martin <Dave.Martin@arm.com>
>> >> Cc: zijun_hu <zijun_hu@htc.com>
>> >> Cc: linux-arm-kernel@lists.infradead.org
>> >> Signed-off-by: Kees Cook <keescook@chromium.org>
>> >> ---
>> >>  arch/arm64/Kconfig                 | 1 +
>> >>  arch/arm64/include/asm/processor.h | 8 ++++++++
>> >>  2 files changed, 9 insertions(+)
>> >>
>> >> diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
>> >> index a93339f5178f..c84477e6a884 100644
>> >> --- a/arch/arm64/Kconfig
>> >> +++ b/arch/arm64/Kconfig
>> >> @@ -90,6 +90,7 @@ config ARM64
>> >>       select HAVE_ARCH_MMAP_RND_BITS
>> >>       select HAVE_ARCH_MMAP_RND_COMPAT_BITS if COMPAT
>> >>       select HAVE_ARCH_SECCOMP_FILTER
>> >> +     select HAVE_ARCH_THREAD_STRUCT_WHITELIST
>> >>       select HAVE_ARCH_TRACEHOOK
>> >>       select HAVE_ARCH_TRANSPARENT_HUGEPAGE
>> >>       select HAVE_ARCH_VMAP_STACK
>> >> diff --git a/arch/arm64/include/asm/processor.h b/arch/arm64/include/asm/processor.h
>> >> index 023cacb946c3..e58a5864ec89 100644
>> >> --- a/arch/arm64/include/asm/processor.h
>> >> +++ b/arch/arm64/include/asm/processor.h
>> >> @@ -113,6 +113,14 @@ struct thread_struct {
>> >>       struct debug_info       debug;          /* debugging */
>> >>  };
>> >>
>> >> +/* Whitelist the fpsimd_state for copying to userspace. */
>> >> +static inline void arch_thread_struct_whitelist(unsigned long *offset,
>> >> +                                             unsigned long *size)
>> >> +{
>> >> +     *offset = offsetof(struct thread_struct, fpsimd_state);
>> >> +     *size = sizeof(struct fpsimd_state);
>> >
>> > This should be fpsimd_state.user_fpsimd (fpsimd_state.cpu is important
>> > for correctly context switching and not supposed to be user-accessible.
>> > A user copy that encompasses that is definitely a bug).
>>
>> So, I actually spent some more time looking at this due to the
>> comments from rmk on arm32, and I don't think any whitelist is needed
>> here at all. (i.e. it can be *offset = *size = 0) This is because all
>> the usercopying I could find uses static sizes or bounce buffers, both
>> of which bypass the dynamic-size hardened usercopy checks.
>>
>> I've been running some arm64 builds now with this change, and I
>> haven't tripped over any problems yet...
>
> Hmmm, it looks like we may be hitting this with user_regset_copyout()
> when reading the fp regs via ptrace.  This is maybe not surprising,

Did you get one of the WARNs for it?

> since the size comes from userspace for PTRACE_{GET,SET}REGSET.
> Also, while we copy into a bounce buffer for SETREGSET here, we do copy
> straight out of task_struct for GETREGSET here.

Hm, yeah,

> This suggests we should have:
>
>         *offset = offsetof(struct thread_struct, fpsimd_state);
>         *size = sizeof(struct user_fpsimd_state);

This is what I had originally for arm64, but when I tried exercising
this code more recently, it didn't need the whitelist. It really looks
like I forgot what I had tested the first time, though. :P

> Thoughts?

Seems like it would be tripped by:

static int __fpr_get(struct task_struct *target,
                     const struct user_regset *regset,
                     unsigned int pos, unsigned int count,
                     void *kbuf, void __user *ubuf, unsigned int start_pos)
{
        struct user_fpsimd_state *uregs;

        sve_sync_to_fpsimd(target);

        uregs = &target->thread.fpsimd_state.user_fpsimd;

        return user_regset_copyout(&pos, &count, &kbuf, &ubuf, uregs,
                                   start_pos, start_pos + sizeof(*uregs));
}

And similarly __fpr_set(), compat_vfp_get(), compat_vfp_set(),
sve_get(), and sve_set(), ?

> I'm making some assumptions about how the usercopy hardening works.

I think you're right -- I just tricked myself after looking at arm32.

-Kees

-- 
Kees Cook
Pixel Security

  reply	other threads:[~2018-03-26 17:41 UTC|newest]

Thread overview: 82+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-01-11  2:02 [kernel-hardening] [PATCH v5 00/38] Hardened usercopy whitelisting Kees Cook
2018-01-11  2:02 ` [kernel-hardening] [PATCH 01/38] usercopy: Remove pointer from overflow report Kees Cook
2018-01-11  2:02 ` [kernel-hardening] [PATCH 02/38] usercopy: Enhance and rename report_usercopy() Kees Cook
2018-01-11 17:06   ` [kernel-hardening] " Christopher Lameter
2018-01-14 20:57     ` Kees Cook
2018-01-11  2:02 ` [kernel-hardening] [PATCH 03/38] usercopy: Include offset in hardened usercopy report Kees Cook
2018-01-11  2:02 ` [kernel-hardening] [PATCH 04/38] lkdtm/usercopy: Adjust test to include an offset to check reporting Kees Cook
2018-01-11  2:02 ` [kernel-hardening] [PATCH 05/38] stddef.h: Introduce sizeof_field() Kees Cook
2018-01-11  2:02 ` [kernel-hardening] [PATCH 06/38] usercopy: Prepare for usercopy whitelisting Kees Cook
2018-01-11  2:02 ` [kernel-hardening] [PATCH 07/38] usercopy: WARN() on slab cache usercopy region violations Kees Cook
2018-01-11  2:02 ` [kernel-hardening] [PATCH 08/38] usercopy: Allow strict enforcement of whitelists Kees Cook
2018-01-11  2:02 ` [kernel-hardening] [PATCH 09/38] usercopy: Mark kmalloc caches as usercopy caches Kees Cook
2019-11-12  7:17   ` Jiri Slaby
2019-11-12 21:21     ` Kees Cook
2019-11-14 21:27       ` Kees Cook
2020-01-23  8:14         ` Jiri Slaby
2020-01-27 23:19           ` Kees Cook
2020-01-28  7:58             ` Christian Borntraeger
2020-01-28 23:01               ` Kees Cook
2020-01-29  9:26                 ` Ursula Braun
2020-01-29 16:43                 ` Christopher Lameter
2020-01-29 17:07                   ` Christian Borntraeger
2020-01-29 17:09                     ` Christoph Hellwig
2020-01-29 17:19                       ` Christian Borntraeger
2020-01-30 19:23                         ` Kees Cook
2020-01-31 12:03                           ` Jann Horn
2020-02-01 17:56                             ` Kees Cook
2020-02-01 19:27                               ` Jann Horn
2020-02-03  7:46                                 ` Matthew Wilcox
2020-02-03 17:41                                   ` Christoph Hellwig
2020-02-03 17:20                               ` Christopher Lameter
2020-04-07  8:00                             ` Vlastimil Babka
2020-04-07 11:05                               ` Christian Borntraeger
2020-04-20  7:53                               ` Jiri Slaby
2020-04-20 17:43                                 ` Kees Cook
2020-02-03 17:38                           ` Christoph Hellwig
2020-02-03 17:36                         ` Christoph Hellwig
2018-01-11  2:02 ` [kernel-hardening] [PATCH 10/38] dcache: Define usercopy region in dentry_cache slab cache Kees Cook
2018-01-11  2:02 ` [kernel-hardening] [PATCH 11/38] vfs: Define usercopy region in names_cache slab caches Kees Cook
2018-01-11  2:02 ` [kernel-hardening] [PATCH 12/38] vfs: Copy struct mount.mnt_id to userspace using put_user() Kees Cook
2018-01-11  2:02 ` [kernel-hardening] [PATCH 13/38] ext4: Define usercopy region in ext4_inode_cache slab cache Kees Cook
2018-01-11 17:01   ` [kernel-hardening] " Theodore Ts'o
2018-01-11 23:05     ` Kees Cook
2018-01-14 22:34       ` Matthew Wilcox
2018-01-11  2:02 ` [kernel-hardening] [PATCH 14/38] ext2: Define usercopy region in ext2_inode_cache " Kees Cook
2018-01-11  2:02 ` [kernel-hardening] [PATCH 15/38] jfs: Define usercopy region in jfs_ip " Kees Cook
2018-01-11  2:02 ` [kernel-hardening] [PATCH 16/38] befs: Define usercopy region in befs_inode_cache " Kees Cook
2018-01-11  2:02 ` [kernel-hardening] [PATCH 17/38] exofs: Define usercopy region in exofs_inode_cache " Kees Cook
2018-01-11  2:02 ` [kernel-hardening] [PATCH 18/38] orangefs: Define usercopy region in orangefs_inode_cache " Kees Cook
2018-01-11  2:02 ` [kernel-hardening] [PATCH 19/38] ufs: Define usercopy region in ufs_inode_cache " Kees Cook
2018-01-11  2:02 ` [kernel-hardening] [PATCH 20/38] vxfs: Define usercopy region in vxfs_inode " Kees Cook
2018-01-11  2:02 ` [kernel-hardening] [PATCH 21/38] cifs: Define usercopy region in cifs_request " Kees Cook
2018-01-11  2:02 ` [kernel-hardening] [PATCH 22/38] scsi: Define usercopy region in scsi_sense_cache " Kees Cook
2018-01-11  2:02 ` [kernel-hardening] [PATCH 23/38] net: Define usercopy region in struct proto " Kees Cook
2018-01-11  2:02 ` [kernel-hardening] [PATCH 24/38] ip: Define usercopy region in IP " Kees Cook
2018-01-11  2:02 ` [kernel-hardening] [PATCH 25/38] caif: Define usercopy region in caif " Kees Cook
2018-01-11  2:02 ` [kernel-hardening] [PATCH 26/38] sctp: Define usercopy region in SCTP " Kees Cook
2018-01-11  2:02 ` [kernel-hardening] [PATCH 27/38] sctp: Copy struct sctp_sock.autoclose to userspace using put_user() Kees Cook
2018-01-18 21:31   ` [kernel-hardening] " Laura Abbott
2018-01-18 21:36     ` Kees Cook
2018-01-11  2:03 ` [kernel-hardening] [PATCH 28/38] net: Restrict unwhitelisted proto caches to size 0 Kees Cook
2018-01-11  2:03 ` [kernel-hardening] [PATCH 29/38] fork: Define usercopy region in mm_struct slab caches Kees Cook
2018-01-11  2:03 ` [kernel-hardening] [PATCH 30/38] fork: Define usercopy region in thread_stack " Kees Cook
2018-01-11  2:03 ` [kernel-hardening] [PATCH 31/38] fork: Provide usercopy whitelisting for task_struct Kees Cook
2018-01-11  2:03 ` [kernel-hardening] [PATCH 32/38] x86: Implement thread_struct whitelist for hardened usercopy Kees Cook
2018-01-11  2:03 ` [kernel-hardening] [PATCH 33/38] arm64: " Kees Cook
2018-01-15 12:24   ` [kernel-hardening] " Dave P Martin
2018-01-15 20:06     ` Kees Cook
2018-01-16 12:33       ` Dave Martin
2018-03-26 16:22       ` Dave Martin
2018-03-26 17:41         ` Kees Cook [this message]
2018-03-27 12:32           ` Dave Martin
2018-01-11  2:03 ` [kernel-hardening] [PATCH 34/38] arm: " Kees Cook
2018-01-11 10:24   ` [kernel-hardening] " Russell King - ARM Linux
2018-01-11 23:21     ` Kees Cook
2018-01-11  2:03 ` [kernel-hardening] [PATCH 35/38] kvm: whitelist struct kvm_vcpu_arch Kees Cook
2018-01-11  2:03 ` [kernel-hardening] [PATCH 36/38] kvm: x86: fix KVM_XEN_HVM_CONFIG ioctl Kees Cook
2018-01-11  2:03 ` [kernel-hardening] [PATCH 37/38] usercopy: Restrict non-usercopy caches to size 0 Kees Cook
2018-01-11  2:03 ` [kernel-hardening] [PATCH 38/38] lkdtm: Update usercopy tests for whitelisting Kees Cook
2018-02-19 16:55 ` [PATCH] signals: Move put_compat_sigset to compat.h to silence hardened usercopy Matt Redfearn
2018-02-19 23:55   ` Kees Cook
2018-03-02 21:40     ` James Hogan

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to='CAGXu5jL1S+CFEruBOaN_yJa7cY+a9gr8cBQgFcYowW=gT5VfSA@mail.gmail.com' \
    --to=keescook@chromium.org \
    --cc=Catalin.Marinas@arm.com \
    --cc=Dave.Martin@arm.com \
    --cc=Mark.Rutland@arm.com \
    --cc=Will.Deacon@arm.com \
    --cc=kernel-hardening@lists.openwall.com \
    --cc=linux-arm-kernel@lists.infradead.org \
    --subject='Re: [PATCH 33/38] arm64: Implement thread_struct whitelist for hardened usercopy' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).