From: Kees Cook <keescook@chromium.org> To: Mark Rutland <mark.rutland@arm.com> Cc: Alexander Popov <alex.popov@linux.com>, linux-arm-kernel@lists.infradead.org, akpm@linux-foundation.org, catalin.marinas@arm.com, linux-kernel@vger.kernel.org, luto@kernel.org, will@kernel.org Subject: Re: [PATCH v2 05/13] stackleak: clarify variable names Date: Tue, 10 May 2022 20:05:58 -0700 [thread overview] Message-ID: <202205102001.7EB50CBE18@keescook> (raw) In-Reply-To: <YnpiPWYqkA7RW3lm@FVFF77S0Q05N> On Tue, May 10, 2022 at 02:01:49PM +0100, Mark Rutland wrote: > On Sun, May 08, 2022 at 11:49:46PM +0300, Alexander Popov wrote: > > On 27.04.2022 20:31, Mark Rutland wrote: > > > The logic within __stackleak_erase() can be a little hard to follow, as > > > `boundary` switches from being the low bound to the high bound mid way > > > through the function, and `kstack_ptr` is used to represent the start of > > > the region to erase while `boundary` represents the end of the region to > > > erase. > > > > > > Make this a little clearer by consistently using clearer variable names. > > > The `boundary` variable is removed, the bounds of the region to erase > > > are described by `erase_low` and `erase_high`, and bounds of the task > > > stack are described by `task_stack_low` and `task_stck_high`. > > > > A typo here in `task_stck_high`. > > Ah; whoops. No worries; I fixed this when I took the patch. > > That was also the main reason why I reused the 'boundary' variable: I wanted > > the compiler to allocate it in the register and I avoided creating many > > local variables. > > > > Mark, did your refactoring make the compiler allocate local variables on the > > stack instead of the registers? > > Considering the whole series, testing with GCC 11.1.0: > > * On arm64: > before: stackleak_erase() uses 48 bytes of stack > after: stackleak_erase() uses 0 bytes of stack > > Note: this is entirely due to patch 1; arm64 has enough GPRs that it > doesn't need to use the stack. > > * On x86_64: > before: stackleak_erase() uses 0 bytes of stack > after: stackleak_erase() uses 0 bytes of stack > > * On i386 > before: stackleak_erase() uses 8 bytes of stach > after: stackleak_erase() uses 16 bytes of stack > > The i386 case isn't ideal, but given that those bytes will easily be used by > the entry triage code before getting to any syscall handling, I don't believe > that's an issue in practice. I am biased and totally fine with choosing a solution where 64-bit improvement comes at a 32-bit cost. -- Kees Cook
WARNING: multiple messages have this Message-ID (diff)
From: Kees Cook <keescook@chromium.org> To: Mark Rutland <mark.rutland@arm.com> Cc: Alexander Popov <alex.popov@linux.com>, linux-arm-kernel@lists.infradead.org, akpm@linux-foundation.org, catalin.marinas@arm.com, linux-kernel@vger.kernel.org, luto@kernel.org, will@kernel.org Subject: Re: [PATCH v2 05/13] stackleak: clarify variable names Date: Tue, 10 May 2022 20:05:58 -0700 [thread overview] Message-ID: <202205102001.7EB50CBE18@keescook> (raw) In-Reply-To: <YnpiPWYqkA7RW3lm@FVFF77S0Q05N> On Tue, May 10, 2022 at 02:01:49PM +0100, Mark Rutland wrote: > On Sun, May 08, 2022 at 11:49:46PM +0300, Alexander Popov wrote: > > On 27.04.2022 20:31, Mark Rutland wrote: > > > The logic within __stackleak_erase() can be a little hard to follow, as > > > `boundary` switches from being the low bound to the high bound mid way > > > through the function, and `kstack_ptr` is used to represent the start of > > > the region to erase while `boundary` represents the end of the region to > > > erase. > > > > > > Make this a little clearer by consistently using clearer variable names. > > > The `boundary` variable is removed, the bounds of the region to erase > > > are described by `erase_low` and `erase_high`, and bounds of the task > > > stack are described by `task_stack_low` and `task_stck_high`. > > > > A typo here in `task_stck_high`. > > Ah; whoops. No worries; I fixed this when I took the patch. > > That was also the main reason why I reused the 'boundary' variable: I wanted > > the compiler to allocate it in the register and I avoided creating many > > local variables. > > > > Mark, did your refactoring make the compiler allocate local variables on the > > stack instead of the registers? > > Considering the whole series, testing with GCC 11.1.0: > > * On arm64: > before: stackleak_erase() uses 48 bytes of stack > after: stackleak_erase() uses 0 bytes of stack > > Note: this is entirely due to patch 1; arm64 has enough GPRs that it > doesn't need to use the stack. > > * On x86_64: > before: stackleak_erase() uses 0 bytes of stack > after: stackleak_erase() uses 0 bytes of stack > > * On i386 > before: stackleak_erase() uses 8 bytes of stach > after: stackleak_erase() uses 16 bytes of stack > > The i386 case isn't ideal, but given that those bytes will easily be used by > the entry triage code before getting to any syscall handling, I don't believe > that's an issue in practice. I am biased and totally fine with choosing a solution where 64-bit improvement comes at a 32-bit cost. -- Kees Cook _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
next prev parent reply other threads:[~2022-05-11 3:06 UTC|newest] Thread overview: 94+ messages / expand[flat|nested] mbox.gz Atom feed top 2022-04-27 17:31 [PATCH v2 00/13] stackleak: fixes and rework Mark Rutland 2022-04-27 17:31 ` Mark Rutland 2022-04-27 17:31 ` [PATCH v2 01/13] arm64: stackleak: fix current_top_of_stack() Mark Rutland 2022-04-27 17:31 ` Mark Rutland 2022-05-04 16:41 ` Catalin Marinas 2022-05-04 16:41 ` Catalin Marinas 2022-05-04 19:01 ` Kees Cook 2022-05-04 19:01 ` Kees Cook 2022-05-04 19:55 ` Catalin Marinas 2022-05-04 19:55 ` Catalin Marinas 2022-05-05 8:25 ` Will Deacon 2022-05-05 8:25 ` Will Deacon 2022-05-08 17:24 ` Alexander Popov 2022-05-08 17:24 ` Alexander Popov 2022-05-10 11:36 ` Mark Rutland 2022-05-10 11:36 ` Mark Rutland 2022-04-27 17:31 ` [PATCH v2 02/13] stackleak: move skip_erasing() check earlier Mark Rutland 2022-04-27 17:31 ` Mark Rutland 2022-05-08 17:44 ` Alexander Popov 2022-05-08 17:44 ` Alexander Popov 2022-05-10 11:40 ` Mark Rutland 2022-05-10 11:40 ` Mark Rutland 2022-04-27 17:31 ` [PATCH v2 03/13] stackleak: remove redundant check Mark Rutland 2022-04-27 17:31 ` Mark Rutland 2022-05-08 18:17 ` Alexander Popov 2022-05-08 18:17 ` Alexander Popov 2022-05-10 11:46 ` Mark Rutland 2022-05-10 11:46 ` Mark Rutland 2022-05-11 3:00 ` Kees Cook 2022-05-11 3:00 ` Kees Cook 2022-05-11 8:02 ` Mark Rutland 2022-05-11 8:02 ` Mark Rutland 2022-05-11 14:44 ` Kees Cook 2022-05-11 14:44 ` Kees Cook 2022-05-12 9:14 ` Mark Rutland 2022-05-12 9:14 ` Mark Rutland 2022-05-15 16:17 ` Alexander Popov 2022-05-15 16:17 ` Alexander Popov 2022-05-24 10:03 ` Mark Rutland 2022-05-24 10:03 ` Mark Rutland 2022-05-26 22:09 ` Alexander Popov 2022-05-26 22:09 ` Alexander Popov 2022-04-27 17:31 ` [PATCH v2 04/13] stackleak: rework stack low bound handling Mark Rutland 2022-04-27 17:31 ` Mark Rutland 2022-04-27 17:31 ` [PATCH v2 05/13] stackleak: clarify variable names Mark Rutland 2022-04-27 17:31 ` Mark Rutland 2022-05-08 20:49 ` Alexander Popov 2022-05-08 20:49 ` Alexander Popov 2022-05-10 13:01 ` Mark Rutland 2022-05-10 13:01 ` Mark Rutland 2022-05-11 3:05 ` Kees Cook [this message] 2022-05-11 3:05 ` Kees Cook 2022-04-27 17:31 ` [PATCH v2 06/13] stackleak: rework stack high bound handling Mark Rutland 2022-04-27 17:31 ` Mark Rutland 2022-05-08 21:27 ` Alexander Popov 2022-05-08 21:27 ` Alexander Popov 2022-05-10 11:22 ` Mark Rutland 2022-05-10 11:22 ` Mark Rutland 2022-05-15 16:32 ` Alexander Popov 2022-05-15 16:32 ` Alexander Popov 2022-04-27 17:31 ` [PATCH v2 07/13] stackleak: rework poison scanning Mark Rutland 2022-04-27 17:31 ` Mark Rutland 2022-05-09 13:51 ` Alexander Popov 2022-05-09 13:51 ` Alexander Popov 2022-05-10 13:13 ` Mark Rutland 2022-05-10 13:13 ` Mark Rutland 2022-05-15 17:33 ` Alexander Popov 2022-05-15 17:33 ` Alexander Popov 2022-05-24 13:31 ` Mark Rutland 2022-05-24 13:31 ` Mark Rutland 2022-05-26 23:25 ` Alexander Popov 2022-05-26 23:25 ` Alexander Popov 2022-05-31 18:13 ` Kees Cook 2022-05-31 18:13 ` Kees Cook 2022-06-03 16:55 ` Alexander Popov 2022-06-03 16:55 ` Alexander Popov 2022-04-27 17:31 ` [PATCH v2 08/13] lkdtm/stackleak: avoid spurious failure Mark Rutland 2022-04-27 17:31 ` Mark Rutland 2022-04-27 17:31 ` [PATCH v2 09/13] lkdtm/stackleak: rework boundary management Mark Rutland 2022-04-27 17:31 ` Mark Rutland 2022-05-04 19:07 ` Kees Cook 2022-05-04 19:07 ` Kees Cook 2022-04-27 17:31 ` [PATCH v2 10/13] lkdtm/stackleak: prevent unexpected stack usage Mark Rutland 2022-04-27 17:31 ` Mark Rutland 2022-04-27 17:31 ` [PATCH v2 11/13] lkdtm/stackleak: check stack boundaries Mark Rutland 2022-04-27 17:31 ` Mark Rutland 2022-04-27 17:31 ` [PATCH v2 12/13] stackleak: add on/off stack variants Mark Rutland 2022-04-27 17:31 ` Mark Rutland 2022-04-27 17:31 ` [PATCH v2 13/13] arm64: entry: use stackleak_erase_on_task_stack() Mark Rutland 2022-04-27 17:31 ` Mark Rutland 2022-05-04 16:42 ` Catalin Marinas 2022-05-04 16:42 ` Catalin Marinas 2022-05-04 19:16 ` [PATCH v2 00/13] stackleak: fixes and rework Kees Cook 2022-05-04 19:16 ` Kees Cook
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=202205102001.7EB50CBE18@keescook \ --to=keescook@chromium.org \ --cc=akpm@linux-foundation.org \ --cc=alex.popov@linux.com \ --cc=catalin.marinas@arm.com \ --cc=linux-arm-kernel@lists.infradead.org \ --cc=linux-kernel@vger.kernel.org \ --cc=luto@kernel.org \ --cc=mark.rutland@arm.com \ --cc=will@kernel.org \ /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.