From mboxrd@z Thu Jan 1 00:00:00 1970 From: Will Deacon Subject: Re: [PATCH v5 15/30] arm64/sve: Signal handling support Date: Thu, 7 Dec 2017 10:49:48 +0000 Message-ID: <20171207104948.GE31900@arm.com> References: <1509465082-30427-1-git-send-email-Dave.Martin@arm.com> <1509465082-30427-16-git-send-email-Dave.Martin@arm.com> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Return-path: Received: from usa-sjc-mx-foss1.foss.arm.com ([217.140.101.70]:48906 "EHLO foss.arm.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752291AbdLGKtn (ORCPT ); Thu, 7 Dec 2017 05:49:43 -0500 Content-Disposition: inline In-Reply-To: Sender: linux-arch-owner@vger.kernel.org List-ID: To: Kees Cook Cc: Dave Martin , linux-arm-kernel@lists.infradead.org, linux-arch , Okamoto Takayuki , libc-alpha , Ard Biesheuvel , Szabolcs Nagy , Catalin Marinas , Alex =?iso-8859-1?Q?Benn=E9e?= , kvmarm@lists.cs.columbia.edu Hi Kees, On Wed, Dec 06, 2017 at 11:56:50AM -0800, Kees Cook wrote: > On Tue, Oct 31, 2017 at 8:51 AM, Dave Martin wrote: > > Miscellaneous: > > > > * Change inconsistent copy_to_user() calls to __copy_to_user() in > > preserve_sve_context(). > > > > There are already __put_user_error() calls here. > > > > The whole extended signal frame is already checked for > > access_ok(VERIFY_WRITE) in get_sigframe(). > > Verifying all these __copy_to/from_user() calls is rather non-trivial. > For example, I had to understand that the access_ok() check actually > spans memory that both user->sigframe and user->next_frame point into. I don't think that's particularly difficult -- you just have to read the four lines preceding the access_ok. > And it isn't clear to me that all users of apply_user_offset() are > within this range too, along with other manually calculated offsets in > setup_sigframe(). The offsets passed into apply_user_offset are calculated by setup_sigframe_layout as the stack is allocated, so they're correct by construction. We could add a size check in apply_user_offset if you like? > And it's not clear if parse_user_sigframe() is safe either. Are > user->fpsimd and user->sve checked somewhere? It seems like it's > safely contained by in sf->uc.uc_mcontext.__reserved, but it's hard to > read, though I do see access_ok() checks against __reserved at the end > of the while loop. This one is certainly more difficult to follow, mainly because it's spread about a bit and we have to check the extra context separately. However, the main part of the frame is checked in sys_rt_sigreturn before calling restore_sigframe, and the extra context is checked in parse_user_sigframe if we find it. Dave, any thoughts on making this easier to understand? Will