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 6B316C433FE for ; Tue, 10 May 2022 13:05:54 +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=+2WuPrq/J3j2zk8D2Wtf8hOEVAhuJeYPB7UquxUVAm0=; b=4zD2FgODcSQrnG ZvPQ8bA62yL30t613X/+JFP9QedK7oq+iSaH2YcKwDoTikC4YU1Rq27kWZJ+SFdbdcSEJQZRgdD82 KjSejGnFXU6I/FHftdjgnmEcYnt1Lnn3rdFSEXf/tNPDNxC5Nfv7LZolVP/WOaIgtIGSF1DyartjY Qfq2SpdofXPPVAdY5OnLd+cIVLiPGrjbG31tAssc57Mpubci9abVDW6F/LmHV2f1ZVVTfVECNUyll EO8DELoZIMC4T+0KVf/NBtb/DLoXwUzxMk1P1nSisMyDfiLtiTiAisLIj124c8RhQ69TYEqVi68an aZcL+iW5xm8L1JbjZoRg==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.94.2 #2 (Red Hat Linux)) id 1noPXM-0026ZC-NB; Tue, 10 May 2022 13:04:25 +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 1noPV3-0025WH-3X for linux-arm-kernel@lists.infradead.org; Tue, 10 May 2022 13:02:05 +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 47B3023A; Tue, 10 May 2022 06:01:59 -0700 (PDT) Received: from FVFF77S0Q05N (unknown [10.57.1.67]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id D27493F73D; Tue, 10 May 2022 06:01:57 -0700 (PDT) Date: Tue, 10 May 2022 14:01:49 +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 05/13] stackleak: clarify variable names Message-ID: References: <20220427173128.2603085-1-mark.rutland@arm.com> <20220427173128.2603085-6-mark.rutland@arm.com> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20220510_060201_386102_25B612CD X-CRM114-Status: GOOD ( 41.02 ) 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 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. > > As the same time, remove the comment above the variables, since it is > > unclear whether it's intended as rationale, a complaint, or a TODO, and > > is more confusing than helpful. > > Yes, this comment is a bit confusing :) I can elaborate. > > In the original grsecurity patch, the stackleak erasing was written in asm. > When I adopted it and proposed for the upstream, Linus strongly opposed this. > So I developed stackleak erasing in C. > > And I wrote this comment to remember that having 'kstack_ptr' and 'boundary' > variables on the stack (which we are clearing) would not be good. Ok, so I think that falls into the "complaint" bucket I mentioned. I understand that we don't have any guarantee from the compiler as to how it will use the stack, and that's obviously a potential problem. > 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. Thanks, Mark. > > There should be no functional change as a result of this patch. > > > > Signed-off-by: Mark Rutland > > Cc: Alexander Popov > > Cc: Andrew Morton > > Cc: Andy Lutomirski > > Cc: Kees Cook > > --- > > kernel/stackleak.c | 30 ++++++++++++++---------------- > > 1 file changed, 14 insertions(+), 16 deletions(-) > > > > diff --git a/kernel/stackleak.c b/kernel/stackleak.c > > index 24b7cf01b2972..d5f684dc0a2d9 100644 > > --- a/kernel/stackleak.c > > +++ b/kernel/stackleak.c > > @@ -73,40 +73,38 @@ late_initcall(stackleak_sysctls_init); > > static __always_inline void __stackleak_erase(void) > > { > > const unsigned long task_stack_low = stackleak_task_low_bound(current); > > - > > - /* It would be nice not to have 'kstack_ptr' and 'boundary' on stack */ > > - unsigned long kstack_ptr = current->lowest_stack; > > - unsigned long boundary = task_stack_low; > > + 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 (kstack_ptr > boundary && poison_count <= depth) { > > - if (*(unsigned long *)kstack_ptr == STACKLEAK_POISON) > > + while (erase_low > task_stack_low && poison_count <= depth) { > > + if (*(unsigned long *)erase_low == STACKLEAK_POISON) > > poison_count++; > > else > > poison_count = 0; > > - kstack_ptr -= sizeof(unsigned long); > > + erase_low -= sizeof(unsigned long); > > } > > #ifdef CONFIG_STACKLEAK_METRICS > > - current->prev_lowest_stack = kstack_ptr; > > + current->prev_lowest_stack = erase_low; > > #endif > > /* > > - * Now write the poison value to the kernel stack. Start from > > - * 'kstack_ptr' and move up till the new 'boundary'. We assume that > > - * the stack pointer doesn't change when we write poison. > > + * Now write the poison value to the kernel stack between 'erase_low' > > + * and 'erase_high'. We assume that the stack pointer doesn't change > > + * when we write poison. > > */ > > if (on_thread_stack()) > > - boundary = current_stack_pointer; > > + erase_high = current_stack_pointer; > > else > > - boundary = current_top_of_stack(); > > + erase_high = current_top_of_stack(); > > - while (kstack_ptr < boundary) { > > - *(unsigned long *)kstack_ptr = STACKLEAK_POISON; > > - kstack_ptr += sizeof(unsigned long); > > + while (erase_low < erase_high) { > > + *(unsigned long *)erase_low = STACKLEAK_POISON; > > + erase_low += sizeof(unsigned long); > > } > > /* Reset the 'lowest_stack' value for the next syscall */ > _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel