[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20180116123332.GX22781@e103592.cambridge.arm.com>
Date: Tue, 16 Jan 2018 12:33:34 +0000
From: Dave Martin <Dave.Martin@....com>
To: Kees Cook <keescook@...omium.org>
Cc: Mark Rutland <Mark.Rutland@....com>, Jan Kara <jack@...e.cz>,
"kernel-hardening@...ts.openwall.com" <kernel-hardening@...ts.openwall.com>,
"Peter Zijlstra (Intel)" <peterz@...radead.org>,
Catalin Marinas <Catalin.Marinas@....com>,
Will Deacon <Will.Deacon@....com>, Rik van Riel <riel@...hat.com>,
"linux-mm@...ck.org" <linux-mm@...ck.org>,
Christoph Lameter <cl@...ux.com>, Ingo Molnar <mingo@...nel.org>,
zijun_hu <zijun_hu@....com>,
"linux-arch@...r.kernel.org" <linux-arch@...r.kernel.org>,
Christoph Hellwig <hch@...radead.org>,
Christian Borntraeger <borntraeger@...ibm.com>,
Laura Abbott <labbott@...hat.com>,
David Windsor <dave@...lcore.net>,
Marc Zyngier <Marc.Zyngier@....com>,
Dave Kleikamp <dave.kleikamp@...cle.com>,
Alexander Viro <viro@...iv.linux.org.uk>,
Andy Lutomirski <luto@...nel.org>,
"linux-arm-kernel@...ts.infradead.org" <linux-arm-kernel@...ts.infradead.org>,
Luis de Bethencourt <luisbg@...nel.org>,
"Martin K. Petersen" <martin.petersen@...cle.com>,
"netdev@...r.kernel.org" <netdev@...r.kernel.org>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
Matthew Garrett <mjg59@...gle.com>,
James Morse <James.Morse@....com>,
"linux-fsdevel@...r.kernel.org" <linux-fsdevel@...r.kernel.org>,
Paolo Bonzini <pbonzini@...hat.com>,
Andrew Morton <akpm@...ux-foundation.org>,
Linus Torvalds <torvalds@...ux-foundation.org>,
"David S. Miller" <davem@...emloft.net>,
Christoffer Dall <christoffer.dall@...aro.org>
Subject: Re: [PATCH 33/38] arm64: Implement thread_struct whitelist for
hardened usercopy
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@....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@....com>
> >> Cc: Will Deacon <will.deacon@....com>
> >> Cc: Christian Borntraeger <borntraeger@...ibm.com>
> >> Cc: Ingo Molnar <mingo@...nel.org>
> >> Cc: James Morse <james.morse@....com>
> >> Cc: "Peter Zijlstra (Intel)" <peterz@...radead.org>
> >> Cc: Dave Martin <Dave.Martin@....com>
> >> Cc: zijun_hu <zijun_hu@....com>
> >> Cc: linux-arm-kernel@...ts.infradead.org
> >> Signed-off-by: Kees Cook <keescook@...omium.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.
Why do static sizes bypass these checks? Just for efficiency?
> I've been running some arm64 builds now with this change, and I
> haven't tripped over any problems yet...
Sounds fair enough for now.
I haven't ruled out getting rid of the bounce buffers for FPSIMD
copy-in/out. They add stack and runtime overhead, and don't seem
to bring benefits.
Bounce buffers enable copies to succeed or fail atomically, rather
than being half-done and then faulting. This feels cleaner, but
In practice this doesn't seem to matter in real situations.
For SVE there are no bounce buffers, because the register data
can be unreasonably large (at least in theory).
Cheers
---Dave
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.
