| Message ID | 1515636190-24061-35-git-send-email-keescook@chromium.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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@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 >
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
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) \
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@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(+)