From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from bombadil.infradead.org (bombadil.infradead.org [198.137.202.133]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id 18CB2C433F5 for ; Tue, 10 May 2022 13:15:59 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20210309; h=Sender: Content-Transfer-Encoding:Content-Type:List-Subscribe:List-Help:List-Post: List-Archive:List-Unsubscribe:List-Id:In-Reply-To:MIME-Version:References: Message-ID:Subject:Cc:To:From:Date:Reply-To:Content-ID:Content-Description: Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID: List-Owner; bh=hl5oE9qXiDOGOj0CRlQk8anZTiI8LuhFQLIs4I0gT54=; b=uvPOcWEL2HrW8V oEKpswmSPYEDy0anu7aU0FOSulEca4dXOdVhArkuz/pIWXl0B/nMmOi/nTMaSE8D6gtIrKFiGnsAj XYMkUp9KWPRHfI27QAAUte5IoqbcakkW+3NmORXXJnrGyTMFzoYs24bTcT72f2IPWbjVQQ1HpeLLf yRwcyoWGC1BifYWMmRBHKQU3KKFi+DILlUFrzNEW/rpSdUKewE/czZOoFLP+2LmlNsg2EzQ4ZMk0u CzAUc0DKvWkqixE6uvTUNSnWR7vMiXzahd/6pnJKkJRd8gjdqZ5hg01sX+jMjrOtRBXnHaqbw0XGS 2YV0wTt0a6W/tSzuaGaw==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.94.2 #2 (Red Hat Linux)) id 1noPgp-002AAu-Eu; Tue, 10 May 2022 13:14:11 +0000 Received: from foss.arm.com ([217.140.110.172]) by bombadil.infradead.org with esmtp (Exim 4.94.2 #2 (Red Hat Linux)) id 1noPgk-002AA0-Ku for linux-arm-kernel@lists.infradead.org; Tue, 10 May 2022 13:14:08 +0000 Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.121.207.14]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id 74D191042; Tue, 10 May 2022 06:14:05 -0700 (PDT) Received: from FVFF77S0Q05N (unknown [10.57.1.67]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id 0BA333F73D; Tue, 10 May 2022 06:14:03 -0700 (PDT) Date: Tue, 10 May 2022 14:13:58 +0100 From: Mark Rutland To: Alexander Popov Cc: linux-arm-kernel@lists.infradead.org, akpm@linux-foundation.org, catalin.marinas@arm.com, keescook@chromium.org, linux-kernel@vger.kernel.org, luto@kernel.org, will@kernel.org Subject: Re: [PATCH v2 07/13] stackleak: rework poison scanning Message-ID: References: <20220427173128.2603085-1-mark.rutland@arm.com> <20220427173128.2603085-8-mark.rutland@arm.com> <268ea8f7-472b-f1d4-6b8b-0c8fefccc0fa@linux.com> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <268ea8f7-472b-f1d4-6b8b-0c8fefccc0fa@linux.com> X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20220510_061406_866089_A38CE1C1 X-CRM114-Status: GOOD ( 47.60 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org On Mon, May 09, 2022 at 04:51:35PM +0300, Alexander Popov wrote: > Hello Mark! > > On 27.04.2022 20:31, Mark Rutland wrote: > > 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. > > Right, this can be improved. > > > * 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. > > Actually, I think the current code is correct. > > I'm intentionally searching one poison value more than > STACKLEAK_SEARCH_DEPTH to avoid the corner case. See the BUILD_BUG_ON > assertion in stackleak_track_stack() that describes it: > > /* > * Having CONFIG_STACKLEAK_TRACK_MIN_SIZE larger than > * STACKLEAK_SEARCH_DEPTH makes the poison search in > * stackleak_erase() unreliable. Let's prevent that. > */ > BUILD_BUG_ON(CONFIG_STACKLEAK_TRACK_MIN_SIZE > STACKLEAK_SEARCH_DEPTH); I had read that, but as written that doesn't imply that it's necessary to scan one more element than STACKLEAK_SEARCH_DEPTH, nor why. I'm more than happy to change the logic, but I think we need a very clear explanation as to why we need to scan the specific number of bytes we scan, and we should account for that *within* STACKLEAK_SEARCH_DEPTH for clarity. > > 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. > > Right, I don't see any bug in the current erasing code. > > When I wrote the current code, I carefully checked all the corner cases. For > example, on the first stack erasing, the STACK_END_MAGIC was not > overwritten, but the memory next to it was erased. Same for the beginning of > the stack: I carefully checked that no unpoisoned bytes were left on the > thread stack. > > > 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. > > You know, I wrote stackleak_erase() in very plain C. I wanted a compiler to > generate assembly that is very close to the original asm version. I worried > that compilers might do weird stuff with the local variables and the stack > pointer. > > So I checked stackleak for gcc versions 4.8, 5, 6, 7, 8, 9, and 10 on > x86_64, i386 and arm64. This is my project that helped with this work: > https://github.com/a13xp0p0v/kernel-build-containers I've used the kernel.org cross toolchains, as published at: https://mirrors.edge.kernel.org/pub/tools/crosstool/ > Mark, in this patch series you use many local variables and helper functions. > Honestly, this worries me. For example, compilers can (and usually do) > ignore the presence of the 'inline' specifier for the purpose of > optimization. I've deliberately used `__always_inline` rather than regular `inline` to prevent code being placed out-of-line. As mentioned in oether replies it has a stronger semantic. Thanks, Mark. > > Thanks! > > > Signed-off-by: Mark Rutland > > Cc: Alexander Popov > > Cc: Andrew Morton > > Cc: Andy Lutomirski > > Cc: Kees Cook > > --- > > 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; > _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by smtp.lore.kernel.org (Postfix) with ESMTP id 2D401C43217 for ; Tue, 10 May 2022 13:19:15 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S242734AbiEJNXJ (ORCPT ); Tue, 10 May 2022 09:23:09 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:40082 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S242889AbiEJNVI (ORCPT ); Tue, 10 May 2022 09:21:08 -0400 Received: from foss.arm.com (foss.arm.com [217.140.110.172]) by lindbergh.monkeyblade.net (Postfix) with ESMTP id A47292BF33D for ; Tue, 10 May 2022 06:14:05 -0700 (PDT) Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.121.207.14]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id 74D191042; Tue, 10 May 2022 06:14:05 -0700 (PDT) Received: from FVFF77S0Q05N (unknown [10.57.1.67]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id 0BA333F73D; Tue, 10 May 2022 06:14:03 -0700 (PDT) Date: Tue, 10 May 2022 14:13:58 +0100 From: Mark Rutland To: Alexander Popov Cc: linux-arm-kernel@lists.infradead.org, akpm@linux-foundation.org, catalin.marinas@arm.com, keescook@chromium.org, linux-kernel@vger.kernel.org, luto@kernel.org, will@kernel.org Subject: Re: [PATCH v2 07/13] stackleak: rework poison scanning Message-ID: References: <20220427173128.2603085-1-mark.rutland@arm.com> <20220427173128.2603085-8-mark.rutland@arm.com> <268ea8f7-472b-f1d4-6b8b-0c8fefccc0fa@linux.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <268ea8f7-472b-f1d4-6b8b-0c8fefccc0fa@linux.com> Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Mon, May 09, 2022 at 04:51:35PM +0300, Alexander Popov wrote: > Hello Mark! > > On 27.04.2022 20:31, Mark Rutland wrote: > > 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. > > Right, this can be improved. > > > * 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. > > Actually, I think the current code is correct. > > I'm intentionally searching one poison value more than > STACKLEAK_SEARCH_DEPTH to avoid the corner case. See the BUILD_BUG_ON > assertion in stackleak_track_stack() that describes it: > > /* > * Having CONFIG_STACKLEAK_TRACK_MIN_SIZE larger than > * STACKLEAK_SEARCH_DEPTH makes the poison search in > * stackleak_erase() unreliable. Let's prevent that. > */ > BUILD_BUG_ON(CONFIG_STACKLEAK_TRACK_MIN_SIZE > STACKLEAK_SEARCH_DEPTH); I had read that, but as written that doesn't imply that it's necessary to scan one more element than STACKLEAK_SEARCH_DEPTH, nor why. I'm more than happy to change the logic, but I think we need a very clear explanation as to why we need to scan the specific number of bytes we scan, and we should account for that *within* STACKLEAK_SEARCH_DEPTH for clarity. > > 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. > > Right, I don't see any bug in the current erasing code. > > When I wrote the current code, I carefully checked all the corner cases. For > example, on the first stack erasing, the STACK_END_MAGIC was not > overwritten, but the memory next to it was erased. Same for the beginning of > the stack: I carefully checked that no unpoisoned bytes were left on the > thread stack. > > > 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. > > You know, I wrote stackleak_erase() in very plain C. I wanted a compiler to > generate assembly that is very close to the original asm version. I worried > that compilers might do weird stuff with the local variables and the stack > pointer. > > So I checked stackleak for gcc versions 4.8, 5, 6, 7, 8, 9, and 10 on > x86_64, i386 and arm64. This is my project that helped with this work: > https://github.com/a13xp0p0v/kernel-build-containers I've used the kernel.org cross toolchains, as published at: https://mirrors.edge.kernel.org/pub/tools/crosstool/ > Mark, in this patch series you use many local variables and helper functions. > Honestly, this worries me. For example, compilers can (and usually do) > ignore the presence of the 'inline' specifier for the purpose of > optimization. I've deliberately used `__always_inline` rather than regular `inline` to prevent code being placed out-of-line. As mentioned in oether replies it has a stronger semantic. Thanks, Mark. > > Thanks! > > > Signed-off-by: Mark Rutland > > Cc: Alexander Popov > > Cc: Andrew Morton > > Cc: Andy Lutomirski > > Cc: Kees Cook > > --- > > 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; >