[PATCH 33/38] arm64: Implement thread_struct whitelist for hardened usercopy

[PATCH 33/38] arm64: Implement thread_struct whitelist for hardened usercopy

[Kees Cook]

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 at 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);
+}
+
 #ifdef CONFIG_COMPAT
 #define task_user_tls(t)						\
 ({									\
-- 
2.7.4

[PATCH 33/38] arm64: Implement thread_struct whitelist for hardened usercopy

[Dave P Martin]

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 at 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).

Cheers
---Dave
IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.

[PATCH 33/38] arm64: Implement thread_struct whitelist for hardened usercopy

[Kees Cook]

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 at 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...

-Kees

-- 
Kees Cook
Pixel Security

[PATCH 33/38] arm64: Implement thread_struct whitelist for hardened usercopy

[Dave Martin]

[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 at 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,
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.

This suggests we should have:

	*offset = offsetof(struct thread_struct, fpsimd_state);
	*size = sizeof(struct user_fpsimd_state);

Thoughts?

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

Cheers
---Dave

[PATCH 33/38] arm64: Implement thread_struct whitelist for hardened usercopy

[Kees Cook]

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 at 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

[PATCH 33/38] arm64: Implement thread_struct whitelist for hardened usercopy

[Dave Martin]

On Mon, Mar 26, 2018 at 10:41:09AM -0700, Kees Cook wrote:
> 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 at 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?

Mark Rutland got it by running Syzkaller, but it's easily reproducible
with CONFIG_HARDENED_USERCOPY enabled.

> > 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(), ?

All are probably affected except for __fpr_set() which uses a bounce
buffer.  I'm not sure why though: compat_vfp_set() doesn't use a bounce
buffer, though it's not trying to do anything different.

> > I'm making some assumptions about how the usercopy hardening works.
> 
> I think you're right -- I just tricked myself after looking at arm32.

OK, I'll send a patch once we've retested.

Cheers
---Dave

[PATCH 34/38] arm: Implement thread_struct whitelist for hardened usercopy

[Kees Cook]

ARM does not carry FPU state in the thread structure, so it can declare
no usercopy whitelist at all.

Cc: Russell King <linux@armlinux.org.uk>
Cc: Ingo Molnar <mingo@kernel.org>
Cc: Christian Borntraeger <borntraeger@de.ibm.com>
Cc: "Peter Zijlstra (Intel)" <peterz@infradead.org>
Cc: linux-arm-kernel at lists.infradead.org
Signed-off-by: Kees Cook <keescook@chromium.org>
---
 arch/arm/Kconfig                 | 1 +
 arch/arm/include/asm/processor.h | 7 +++++++
 2 files changed, 8 insertions(+)

diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
index 51c8df561077..3ea00d65f35d 100644
--- a/arch/arm/Kconfig
+++ b/arch/arm/Kconfig
@@ -50,6 +50,7 @@ config ARM
 	select HAVE_ARCH_KGDB if !CPU_ENDIAN_BE32 && MMU
 	select HAVE_ARCH_MMAP_RND_BITS if MMU
 	select HAVE_ARCH_SECCOMP_FILTER if (AEABI && !OABI_COMPAT)
+	select HAVE_ARCH_THREAD_STRUCT_WHITELIST
 	select HAVE_ARCH_TRACEHOOK
 	select HAVE_ARM_SMCCC if CPU_V7
 	select HAVE_EBPF_JIT if !CPU_ENDIAN_BE32
diff --git a/arch/arm/include/asm/processor.h b/arch/arm/include/asm/processor.h
index 338cbe0a18ef..01a41be58d43 100644
--- a/arch/arm/include/asm/processor.h
+++ b/arch/arm/include/asm/processor.h
@@ -45,6 +45,13 @@ struct thread_struct {
 	struct debug_info	debug;
 };
 
+/* Nothing needs to be usercopy-whitelisted from thread_struct. */
+static inline void arch_thread_struct_whitelist(unsigned long *offset,
+						unsigned long *size)
+{
+	*offset = *size = 0;
+}
+
 #define INIT_THREAD  {	}
 
 #define start_thread(regs,pc,sp)					\
-- 
2.7.4

[PATCH 34/38] arm: Implement thread_struct whitelist for hardened usercopy

[Russell King - ARM Linux]

On Wed, Jan 10, 2018 at 06:03:06PM -0800, Kees Cook wrote:
> ARM does not carry FPU state in the thread structure, so it can declare
> no usercopy whitelist at all.

This comment seems to be misleading.  We have stored FP state in the
thread structure for a long time - for example, VFP state is stored
in thread->vfpstate.hard, so we _do_ have floating point state in
the thread structure.

What I think this commit message needs to describe is why we don't
need a whitelist _despite_ having FP state in the thread structure.

At the moment, the commit message is making me think that this patch
is wrong and will introduce a regression.

Thanks.

> 
> Cc: Russell King <linux@armlinux.org.uk>
> Cc: Ingo Molnar <mingo@kernel.org>
> Cc: Christian Borntraeger <borntraeger@de.ibm.com>
> Cc: "Peter Zijlstra (Intel)" <peterz@infradead.org>
> Cc: linux-arm-kernel at lists.infradead.org
> Signed-off-by: Kees Cook <keescook@chromium.org>
> ---
>  arch/arm/Kconfig                 | 1 +
>  arch/arm/include/asm/processor.h | 7 +++++++
>  2 files changed, 8 insertions(+)
> 
> diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
> index 51c8df561077..3ea00d65f35d 100644
> --- a/arch/arm/Kconfig
> +++ b/arch/arm/Kconfig
> @@ -50,6 +50,7 @@ config ARM
>  	select HAVE_ARCH_KGDB if !CPU_ENDIAN_BE32 && MMU
>  	select HAVE_ARCH_MMAP_RND_BITS if MMU
>  	select HAVE_ARCH_SECCOMP_FILTER if (AEABI && !OABI_COMPAT)
> +	select HAVE_ARCH_THREAD_STRUCT_WHITELIST
>  	select HAVE_ARCH_TRACEHOOK
>  	select HAVE_ARM_SMCCC if CPU_V7
>  	select HAVE_EBPF_JIT if !CPU_ENDIAN_BE32
> diff --git a/arch/arm/include/asm/processor.h b/arch/arm/include/asm/processor.h
> index 338cbe0a18ef..01a41be58d43 100644
> --- a/arch/arm/include/asm/processor.h
> +++ b/arch/arm/include/asm/processor.h
> @@ -45,6 +45,13 @@ struct thread_struct {
>  	struct debug_info	debug;
>  };
>  
> +/* Nothing needs to be usercopy-whitelisted from thread_struct. */
> +static inline void arch_thread_struct_whitelist(unsigned long *offset,
> +						unsigned long *size)
> +{
> +	*offset = *size = 0;
> +}
> +
>  #define INIT_THREAD  {	}
>  
>  #define start_thread(regs,pc,sp)					\
> -- 
> 2.7.4
> 

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 8.8Mbps down 630kbps up
According to speedtest.net: 8.21Mbps down 510kbps up

[PATCH 34/38] arm: Implement thread_struct whitelist for hardened usercopy

[Kees Cook]

On Thu, Jan 11, 2018 at 2:24 AM, Russell King - ARM Linux
<linux@armlinux.org.uk> wrote:
> On Wed, Jan 10, 2018 at 06:03:06PM -0800, Kees Cook wrote:
>> ARM does not carry FPU state in the thread structure, so it can declare
>> no usercopy whitelist at all.
>
> This comment seems to be misleading.  We have stored FP state in the
> thread structure for a long time - for example, VFP state is stored
> in thread->vfpstate.hard, so we _do_ have floating point state in
> the thread structure.
>
> What I think this commit message needs to describe is why we don't
> need a whitelist _despite_ having FP state in the thread structure.
>
> At the moment, the commit message is making me think that this patch
> is wrong and will introduce a regression.

Yeah, I will improve this comment; it's not clear enough. The places
where I see state copied to/from userspace are all either static sizes
or already use bounce buffers (or both). e.g.:

        err |= __copy_from_user(&hwstate->fpregs, &ufp->fpregs,
                                sizeof(hwstate->fpregs));

I will adjust the commit log and comment to more clearly describe the
lack of whitelisting due to all-static sized copies.

Thanks!

-Kees

-- 
Kees Cook
Pixel Security