From: Kees Cook <keescook@chromium.org> To: Will Deacon <will.deacon@arm.com> Cc: "Dave Martin" <Dave.Martin@arm.com>, linux-arm-kernel@lists.infradead.org, linux-arch <linux-arch@vger.kernel.org>, "Okamoto Takayuki" <tokamoto@jp.fujitsu.com>, libc-alpha <libc-alpha@sourceware.org>, "Ard Biesheuvel" <ard.biesheuvel@linaro.org>, "Szabolcs Nagy" <szabolcs.nagy@arm.com>, "Catalin Marinas" <catalin.marinas@arm.com>, "Alex Bennée" <alex.bennee@linaro.org>, kvmarm@lists.cs.columbia.edu Subject: Re: [PATCH v5 15/30] arm64/sve: Signal handling support Date: Thu, 7 Dec 2017 10:50:38 -0800 [thread overview] Message-ID: <CAGXu5jLO6tHm-mCPBo-csCw--+_jhLfGD_sHXCkFjmyvdame=g@mail.gmail.com> (raw) In-Reply-To: <20171207104948.GE31900@arm.com> On Thu, Dec 7, 2017 at 2:49 AM, Will Deacon <will.deacon@arm.com> wrote: > 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 <Dave.Martin@arm.com> 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. Sure, but someone working backward from finding the __copy_*, it's less obvious. :) >> 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? My question is mainly: why not just use copy_*() everywhere instead? Having these things so spread out makes it fragile, and there's very little performance benefit from using __copy_*() over copy_*(). As far as I can see, everything looks correctly checked here, but it took a while to convince myself of it. Having Dave's description in the other reply certainly helped, though, thanks for that! :) -Kees -- Kees Cook Pixel Security
WARNING: multiple messages have this Message-ID (diff)
From: keescook@chromium.org (Kees Cook) To: linux-arm-kernel@lists.infradead.org Subject: [PATCH v5 15/30] arm64/sve: Signal handling support Date: Thu, 7 Dec 2017 10:50:38 -0800 [thread overview] Message-ID: <CAGXu5jLO6tHm-mCPBo-csCw--+_jhLfGD_sHXCkFjmyvdame=g@mail.gmail.com> (raw) In-Reply-To: <20171207104948.GE31900@arm.com> On Thu, Dec 7, 2017 at 2:49 AM, Will Deacon <will.deacon@arm.com> wrote: > 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 <Dave.Martin@arm.com> 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. Sure, but someone working backward from finding the __copy_*, it's less obvious. :) >> 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? My question is mainly: why not just use copy_*() everywhere instead? Having these things so spread out makes it fragile, and there's very little performance benefit from using __copy_*() over copy_*(). As far as I can see, everything looks correctly checked here, but it took a while to convince myself of it. Having Dave's description in the other reply certainly helped, though, thanks for that! :) -Kees -- Kees Cook Pixel Security
next prev parent reply other threads:[~2017-12-07 18:50 UTC|newest] Thread overview: 174+ messages / expand[flat|nested] mbox.gz Atom feed top 2017-10-31 15:50 [PATCH v5 00/30] ARM Scalable Vector Extension (SVE) Dave Martin 2017-10-31 15:50 ` Dave Martin 2017-10-31 15:50 ` Dave Martin 2017-10-31 15:50 ` [PATCH v5 01/30] regset: Add support for dynamically sized regsets Dave Martin 2017-10-31 15:50 ` Dave Martin 2017-11-01 11:42 ` Catalin Marinas 2017-11-01 11:42 ` Catalin Marinas 2017-11-01 13:16 ` Dave Martin 2017-11-01 13:16 ` Dave Martin 2017-11-08 11:50 ` Alex Bennée 2017-11-08 11:50 ` Alex Bennée 2017-11-08 11:50 ` Alex Bennée 2017-10-31 15:50 ` [PATCH v5 02/30] arm64: fpsimd: Correctly annotate exception helpers called from asm Dave Martin 2017-10-31 15:50 ` Dave Martin 2017-10-31 15:50 ` Dave Martin 2017-11-01 11:42 ` Catalin Marinas 2017-11-01 11:42 ` Catalin Marinas 2017-10-31 15:50 ` [PATCH v5 03/30] arm64: signal: Verify extra data is user-readable in sys_rt_sigreturn Dave Martin 2017-10-31 15:50 ` Dave Martin 2017-10-31 15:50 ` Dave Martin 2017-11-01 11:43 ` Catalin Marinas 2017-11-01 11:43 ` Catalin Marinas 2017-10-31 15:50 ` [PATCH v5 04/30] arm64: KVM: Hide unsupported AArch64 CPU features from guests Dave Martin 2017-10-31 15:50 ` Dave Martin 2017-11-01 4:47 ` Christoffer Dall 2017-11-01 4:47 ` Christoffer Dall 2017-11-01 10:26 ` Dave Martin 2017-11-01 10:26 ` Dave Martin 2017-11-02 8:15 ` Christoffer Dall 2017-11-02 8:15 ` Christoffer Dall 2017-11-02 9:20 ` Dave Martin 2017-11-02 9:20 ` Dave Martin 2017-11-02 11:01 ` Dave Martin 2017-11-02 11:01 ` Dave Martin 2017-11-02 19:18 ` Christoffer Dall 2017-11-02 19:18 ` Christoffer Dall 2017-10-31 15:50 ` [PATCH v5 05/30] arm64: efi: Add missing Kconfig dependency on KERNEL_MODE_NEON Dave Martin 2017-10-31 15:50 ` Dave Martin 2017-10-31 15:50 ` Dave Martin 2017-10-31 15:50 ` [PATCH v5 06/30] arm64: Port deprecated instruction emulation to new sysctl interface Dave Martin 2017-10-31 15:50 ` Dave Martin 2017-10-31 15:50 ` [PATCH v5 07/30] arm64: fpsimd: Simplify uses of {set,clear}_ti_thread_flag() Dave Martin 2017-10-31 15:50 ` [PATCH v5 07/30] arm64: fpsimd: Simplify uses of {set, clear}_ti_thread_flag() Dave Martin 2017-10-31 15:51 ` [PATCH v5 08/30] arm64/sve: System register and exception syndrome definitions Dave Martin 2017-10-31 15:51 ` Dave Martin 2017-10-31 15:51 ` [PATCH v5 09/30] arm64/sve: Low-level SVE architectural state manipulation functions Dave Martin 2017-10-31 15:51 ` Dave Martin 2017-10-31 15:51 ` [PATCH v5 10/30] arm64/sve: Kconfig update and conditional compilation support Dave Martin 2017-10-31 15:51 ` Dave Martin 2017-10-31 15:51 ` [PATCH v5 11/30] arm64/sve: Signal frame and context structure definition Dave Martin 2017-10-31 15:51 ` Dave Martin 2017-11-08 16:34 ` Alex Bennée 2017-11-08 16:34 ` Alex Bennée 2017-11-08 16:34 ` Alex Bennée 2017-10-31 15:51 ` [PATCH v5 12/30] arm64/sve: Low-level CPU setup Dave Martin 2017-10-31 15:51 ` Dave Martin 2017-11-08 16:37 ` Alex Bennée 2017-11-08 16:37 ` Alex Bennée 2017-11-08 16:37 ` Alex Bennée 2017-10-31 15:51 ` [PATCH v5 13/30] arm64/sve: Core task context handling Dave Martin 2017-10-31 15:51 ` Dave Martin 2017-10-31 15:51 ` Dave Martin 2017-11-09 17:16 ` Alex Bennée 2017-11-09 17:16 ` Alex Bennée 2017-11-09 17:16 ` Alex Bennée 2017-11-09 17:56 ` Dave Martin 2017-11-09 17:56 ` Dave Martin 2017-11-09 18:06 ` Alex Bennée 2017-11-09 18:06 ` Alex Bennée 2017-11-09 18:06 ` Alex Bennée 2017-10-31 15:51 ` [PATCH v5 14/30] arm64/sve: Support vector length resetting for new processes Dave Martin 2017-10-31 15:51 ` Dave Martin 2017-10-31 15:51 ` [PATCH v5 15/30] arm64/sve: Signal handling support Dave Martin 2017-10-31 15:51 ` Dave Martin 2017-10-31 15:51 ` Dave Martin 2017-11-01 14:33 ` Catalin Marinas 2017-11-01 14:33 ` Catalin Marinas 2017-11-07 13:22 ` Alex Bennée 2017-11-07 13:22 ` Alex Bennée 2017-11-07 13:22 ` Alex Bennée 2017-11-08 16:11 ` Dave Martin 2017-11-08 16:11 ` Dave Martin 2017-12-06 19:56 ` Kees Cook 2017-12-06 19:56 ` Kees Cook 2017-12-07 10:49 ` Will Deacon 2017-12-07 10:49 ` Will Deacon 2017-12-07 12:03 ` Dave Martin 2017-12-07 12:03 ` Dave Martin 2017-12-07 18:50 ` Kees Cook [this message] 2017-12-07 18:50 ` Kees Cook 2017-12-11 14:07 ` Will Deacon 2017-12-11 14:07 ` Will Deacon 2017-12-11 19:23 ` Kees Cook 2017-12-11 19:23 ` Kees Cook 2017-12-12 10:40 ` Will Deacon 2017-12-12 10:40 ` Will Deacon 2017-12-12 11:11 ` Dave Martin 2017-12-12 11:11 ` Dave Martin 2017-12-12 19:36 ` Kees Cook 2017-12-12 19:36 ` Kees Cook 2017-12-12 19:36 ` Kees Cook 2017-10-31 15:51 ` [PATCH v5 16/30] arm64/sve: Backend logic for setting the vector length Dave Martin 2017-10-31 15:51 ` Dave Martin 2017-10-31 15:51 ` Dave Martin 2017-11-10 10:27 ` Alex Bennée 2017-11-10 10:27 ` Alex Bennée 2017-11-10 10:27 ` Alex Bennée 2017-10-31 15:51 ` [PATCH v5 17/30] arm64: cpufeature: Move sys_caps_initialised declarations Dave Martin 2017-10-31 15:51 ` Dave Martin 2017-10-31 15:51 ` [PATCH v5 18/30] arm64/sve: Probe SVE capabilities and usable vector lengths Dave Martin 2017-10-31 15:51 ` Dave Martin 2017-10-31 15:51 ` Dave Martin 2017-10-31 15:51 ` [PATCH v5 19/30] arm64/sve: Preserve SVE registers around kernel-mode NEON use Dave Martin 2017-10-31 15:51 ` Dave Martin 2017-10-31 15:51 ` [PATCH v5 20/30] arm64/sve: Preserve SVE registers around EFI runtime service calls Dave Martin 2017-10-31 15:51 ` Dave Martin 2017-10-31 15:51 ` [PATCH v5 21/30] arm64/sve: ptrace and ELF coredump support Dave Martin 2017-10-31 15:51 ` Dave Martin 2017-10-31 15:51 ` [PATCH v5 22/30] arm64/sve: Add prctl controls for userspace vector length management Dave Martin 2017-10-31 15:51 ` Dave Martin 2017-10-31 15:51 ` Dave Martin 2017-10-31 15:51 ` [PATCH v5 23/30] arm64/sve: Add sysctl to set the default vector length for new processes Dave Martin 2017-10-31 15:51 ` Dave Martin 2017-10-31 15:51 ` Dave Martin 2017-10-31 15:51 ` [PATCH v5 24/30] arm64/sve: KVM: Prevent guests from using SVE Dave Martin 2017-10-31 15:51 ` Dave Martin 2017-10-31 15:51 ` Dave Martin 2017-10-31 15:51 ` [PATCH v5 25/30] arm64/sve: KVM: Treat guest SVE use as undefined instruction execution Dave Martin 2017-10-31 15:51 ` Dave Martin 2017-10-31 15:51 ` [PATCH v5 26/30] arm64/sve: KVM: Hide SVE from CPU features exposed to guests Dave Martin 2017-10-31 15:51 ` Dave Martin 2017-10-31 15:51 ` [PATCH v5 27/30] arm64/sve: Detect SVE and activate runtime support Dave Martin 2017-10-31 15:51 ` Dave Martin 2017-10-31 15:51 ` Dave Martin 2017-10-31 15:51 ` [RFC PATCH v5 29/30] arm64: signal: Report signal frame size to userspace via auxv Dave Martin 2017-10-31 15:51 ` Dave Martin 2017-10-31 15:51 ` Dave Martin 2017-10-31 15:51 ` [RFC PATCH v5 30/30] arm64/sve: signal: Include SVE when computing AT_MINSIGSTKSZ Dave Martin 2017-10-31 15:51 ` Dave Martin 2017-10-31 15:51 ` Dave Martin [not found] ` <1509465082-30427-1-git-send-email-Dave.Martin-5wv7dgnIgG8@public.gmane.org> 2017-10-31 15:51 ` [PATCH v5 28/30] arm64/sve: Add documentation Dave Martin 2017-10-31 15:51 ` Dave Martin 2017-10-31 15:51 ` Dave Martin 2017-11-02 16:32 ` [PATCH v5 00/30] ARM Scalable Vector Extension (SVE) Will Deacon 2017-11-02 16:32 ` Will Deacon 2017-11-02 16:32 ` Will Deacon [not found] ` <20171102163248.GB595-5wv7dgnIgG8@public.gmane.org> 2017-11-02 17:04 ` Dave P Martin 2017-11-02 17:04 ` Dave P Martin 2017-11-02 17:04 ` Dave P Martin 2017-11-29 15:04 ` Alex Bennée 2017-11-29 15:04 ` Alex Bennée 2017-11-29 15:04 ` Alex Bennée [not found] ` <877eu9dt3n.fsf-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> 2017-11-29 15:21 ` Will Deacon 2017-11-29 15:21 ` Will Deacon 2017-11-29 15:21 ` Will Deacon [not found] ` <20171129152140.GD10650-5wv7dgnIgG8@public.gmane.org> 2017-11-29 15:37 ` Dave Martin 2017-11-29 15:37 ` Dave Martin 2017-11-29 15:37 ` Dave Martin 2018-01-08 14:49 ` Yury Norov 2018-01-08 14:49 ` Yury Norov 2018-01-08 14:49 ` Yury Norov 2018-01-09 16:51 ` Yury Norov 2018-01-09 16:51 ` Yury Norov 2018-01-09 16:51 ` Yury Norov 2018-01-15 17:22 ` Dave Martin 2018-01-15 17:22 ` Dave Martin 2018-01-15 17:22 ` Dave Martin [not found] ` <20180115172201.GW22781-M5GwZQ6tE7x5pKCnmE3YQBJ8xKzm50AiAL8bYrjMMd8@public.gmane.org> 2018-01-16 10:11 ` Yury Norov 2018-01-16 10:11 ` Yury Norov 2018-01-16 16:05 ` Dave Martin 2018-01-16 16:05 ` Dave Martin 2018-01-15 16:55 ` Dave Martin 2018-01-15 16:55 ` Dave Martin 2018-01-15 16:55 ` Dave Martin
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='CAGXu5jLO6tHm-mCPBo-csCw--+_jhLfGD_sHXCkFjmyvdame=g@mail.gmail.com' \ --to=keescook@chromium.org \ --cc=Dave.Martin@arm.com \ --cc=alex.bennee@linaro.org \ --cc=ard.biesheuvel@linaro.org \ --cc=catalin.marinas@arm.com \ --cc=kvmarm@lists.cs.columbia.edu \ --cc=libc-alpha@sourceware.org \ --cc=linux-arch@vger.kernel.org \ --cc=linux-arm-kernel@lists.infradead.org \ --cc=szabolcs.nagy@arm.com \ --cc=tokamoto@jp.fujitsu.com \ --cc=will.deacon@arm.com \ /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: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.