From: Mark Rutland <mark.rutland@arm.com> To: linux-arm-kernel@lists.infradead.org Cc: akpm@linux-foundation.org, alex.popov@linux.com, catalin.marinas@arm.com, keescook@chromium.org, linux-kernel@vger.kernel.org, luto@kernel.org, mark.rutland@arm.com, will@kernel.org Subject: [PATCH v2 07/13] stackleak: rework poison scanning Date: Wed, 27 Apr 2022 18:31:22 +0100 [thread overview] Message-ID: <20220427173128.2603085-8-mark.rutland@arm.com> (raw) In-Reply-To: <20220427173128.2603085-1-mark.rutland@arm.com> Currently we over-estimate the region of stack which must be erased. To determine the region to be erased, we scan downards for a contiguous block of poison values (or the low bound of the stack). There are a few minor problems with this today: * When we find a block of poison values, we include this block within the region to erase. As this is included within the region to erase, this causes us to redundantly overwrite 'STACKLEAK_SEARCH_DEPTH' (128) bytes with poison. * As the loop condition checks 'poison_count <= depth', it will run an additional iteration after finding the contiguous block of poison, decrementing 'erase_low' once more than necessary. As this is included within the region to erase, this causes us to redundantly overwrite an additional unsigned long with poison. * As we always decrement 'erase_low' after checking an element on the stack, we always include the element below this within the region to erase. As this is included within the region to erase, this causes us to redundantly overwrite an additional unsigned long with poison. Note that this is not a functional problem. As the loop condition checks 'erase_low > task_stack_low', we'll never clobber the STACK_END_MAGIC. As we always decrement 'erase_low' after this, we'll never fail to erase the element immediately above the STACK_END_MAGIC. In total, this can cause us to erase `128 + 2 * sizeof(unsigned long)` bytes more than necessary, which is unfortunate. This patch reworks the logic to find the address immediately above the poisoned region, by finding the lowest non-poisoned address. This is factored into a stackleak_find_top_of_poison() helper both for clarity and so that this can be shared with the LKDTM test in subsequent patches. Signed-off-by: Mark Rutland <mark.rutland@arm.com> Cc: Alexander Popov <alex.popov@linux.com> Cc: Andrew Morton <akpm@linux-foundation.org> Cc: Andy Lutomirski <luto@kernel.org> Cc: Kees Cook <keescook@chromium.org> --- include/linux/stackleak.h | 26 ++++++++++++++++++++++++++ kernel/stackleak.c | 18 ++++-------------- 2 files changed, 30 insertions(+), 14 deletions(-) diff --git a/include/linux/stackleak.h b/include/linux/stackleak.h index 467661aeb4136..c36e7a3b45e7e 100644 --- a/include/linux/stackleak.h +++ b/include/linux/stackleak.h @@ -42,6 +42,32 @@ stackleak_task_high_bound(const struct task_struct *tsk) return (unsigned long)task_pt_regs(tsk); } +/* + * Find the address immediately above the poisoned region of the stack, where + * that region falls between 'low' (inclusive) and 'high' (exclusive). + */ +static __always_inline unsigned long +stackleak_find_top_of_poison(const unsigned long low, const unsigned long high) +{ + const unsigned int depth = STACKLEAK_SEARCH_DEPTH / sizeof(unsigned long); + unsigned int poison_count = 0; + unsigned long poison_high = high; + unsigned long sp = high; + + while (sp > low && poison_count < depth) { + sp -= sizeof(unsigned long); + + if (*(unsigned long *)sp == STACKLEAK_POISON) { + poison_count++; + } else { + poison_count = 0; + poison_high = sp; + } + } + + return poison_high; +} + static inline void stackleak_task_init(struct task_struct *t) { t->lowest_stack = stackleak_task_low_bound(t); diff --git a/kernel/stackleak.c b/kernel/stackleak.c index ba346d46218f5..afd54b8e10b83 100644 --- a/kernel/stackleak.c +++ b/kernel/stackleak.c @@ -74,20 +74,10 @@ static __always_inline void __stackleak_erase(void) { const unsigned long task_stack_low = stackleak_task_low_bound(current); const unsigned long task_stack_high = stackleak_task_high_bound(current); - unsigned long erase_low = current->lowest_stack; - unsigned long erase_high; - unsigned int poison_count = 0; - const unsigned int depth = STACKLEAK_SEARCH_DEPTH / sizeof(unsigned long); - - /* Search for the poison value in the kernel stack */ - while (erase_low > task_stack_low && poison_count <= depth) { - if (*(unsigned long *)erase_low == STACKLEAK_POISON) - poison_count++; - else - poison_count = 0; - - erase_low -= sizeof(unsigned long); - } + unsigned long erase_low, erase_high; + + erase_low = stackleak_find_top_of_poison(task_stack_low, + current->lowest_stack); #ifdef CONFIG_STACKLEAK_METRICS current->prev_lowest_stack = erase_low; -- 2.30.2
WARNING: multiple messages have this Message-ID (diff)
From: Mark Rutland <mark.rutland@arm.com> To: linux-arm-kernel@lists.infradead.org Cc: akpm@linux-foundation.org, alex.popov@linux.com, catalin.marinas@arm.com, keescook@chromium.org, linux-kernel@vger.kernel.org, luto@kernel.org, mark.rutland@arm.com, will@kernel.org Subject: [PATCH v2 07/13] stackleak: rework poison scanning Date: Wed, 27 Apr 2022 18:31:22 +0100 [thread overview] Message-ID: <20220427173128.2603085-8-mark.rutland@arm.com> (raw) In-Reply-To: <20220427173128.2603085-1-mark.rutland@arm.com> Currently we over-estimate the region of stack which must be erased. To determine the region to be erased, we scan downards for a contiguous block of poison values (or the low bound of the stack). There are a few minor problems with this today: * When we find a block of poison values, we include this block within the region to erase. As this is included within the region to erase, this causes us to redundantly overwrite 'STACKLEAK_SEARCH_DEPTH' (128) bytes with poison. * As the loop condition checks 'poison_count <= depth', it will run an additional iteration after finding the contiguous block of poison, decrementing 'erase_low' once more than necessary. As this is included within the region to erase, this causes us to redundantly overwrite an additional unsigned long with poison. * As we always decrement 'erase_low' after checking an element on the stack, we always include the element below this within the region to erase. As this is included within the region to erase, this causes us to redundantly overwrite an additional unsigned long with poison. Note that this is not a functional problem. As the loop condition checks 'erase_low > task_stack_low', we'll never clobber the STACK_END_MAGIC. As we always decrement 'erase_low' after this, we'll never fail to erase the element immediately above the STACK_END_MAGIC. In total, this can cause us to erase `128 + 2 * sizeof(unsigned long)` bytes more than necessary, which is unfortunate. This patch reworks the logic to find the address immediately above the poisoned region, by finding the lowest non-poisoned address. This is factored into a stackleak_find_top_of_poison() helper both for clarity and so that this can be shared with the LKDTM test in subsequent patches. Signed-off-by: Mark Rutland <mark.rutland@arm.com> Cc: Alexander Popov <alex.popov@linux.com> Cc: Andrew Morton <akpm@linux-foundation.org> Cc: Andy Lutomirski <luto@kernel.org> Cc: Kees Cook <keescook@chromium.org> --- include/linux/stackleak.h | 26 ++++++++++++++++++++++++++ kernel/stackleak.c | 18 ++++-------------- 2 files changed, 30 insertions(+), 14 deletions(-) diff --git a/include/linux/stackleak.h b/include/linux/stackleak.h index 467661aeb4136..c36e7a3b45e7e 100644 --- a/include/linux/stackleak.h +++ b/include/linux/stackleak.h @@ -42,6 +42,32 @@ stackleak_task_high_bound(const struct task_struct *tsk) return (unsigned long)task_pt_regs(tsk); } +/* + * Find the address immediately above the poisoned region of the stack, where + * that region falls between 'low' (inclusive) and 'high' (exclusive). + */ +static __always_inline unsigned long +stackleak_find_top_of_poison(const unsigned long low, const unsigned long high) +{ + const unsigned int depth = STACKLEAK_SEARCH_DEPTH / sizeof(unsigned long); + unsigned int poison_count = 0; + unsigned long poison_high = high; + unsigned long sp = high; + + while (sp > low && poison_count < depth) { + sp -= sizeof(unsigned long); + + if (*(unsigned long *)sp == STACKLEAK_POISON) { + poison_count++; + } else { + poison_count = 0; + poison_high = sp; + } + } + + return poison_high; +} + static inline void stackleak_task_init(struct task_struct *t) { t->lowest_stack = stackleak_task_low_bound(t); diff --git a/kernel/stackleak.c b/kernel/stackleak.c index ba346d46218f5..afd54b8e10b83 100644 --- a/kernel/stackleak.c +++ b/kernel/stackleak.c @@ -74,20 +74,10 @@ static __always_inline void __stackleak_erase(void) { const unsigned long task_stack_low = stackleak_task_low_bound(current); const unsigned long task_stack_high = stackleak_task_high_bound(current); - unsigned long erase_low = current->lowest_stack; - unsigned long erase_high; - unsigned int poison_count = 0; - const unsigned int depth = STACKLEAK_SEARCH_DEPTH / sizeof(unsigned long); - - /* Search for the poison value in the kernel stack */ - while (erase_low > task_stack_low && poison_count <= depth) { - if (*(unsigned long *)erase_low == STACKLEAK_POISON) - poison_count++; - else - poison_count = 0; - - erase_low -= sizeof(unsigned long); - } + unsigned long erase_low, erase_high; + + erase_low = stackleak_find_top_of_poison(task_stack_low, + current->lowest_stack); #ifdef CONFIG_STACKLEAK_METRICS current->prev_lowest_stack = erase_low; -- 2.30.2 _______________________________________________ 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-04-27 17:32 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 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 ` Mark Rutland [this message] 2022-04-27 17:31 ` [PATCH v2 07/13] stackleak: rework poison scanning 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=20220427173128.2603085-8-mark.rutland@arm.com \ --to=mark.rutland@arm.com \ --cc=akpm@linux-foundation.org \ --cc=alex.popov@linux.com \ --cc=catalin.marinas@arm.com \ --cc=keescook@chromium.org \ --cc=linux-arm-kernel@lists.infradead.org \ --cc=linux-kernel@vger.kernel.org \ --cc=luto@kernel.org \ --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.