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 9A8E5C433F5 for ; Sun, 8 May 2022 20:50:05 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S232744AbiEHUxx (ORCPT ); Sun, 8 May 2022 16:53:53 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:35534 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S232731AbiEHUxn (ORCPT ); Sun, 8 May 2022 16:53:43 -0400 Received: from mail-ej1-f50.google.com (mail-ej1-f50.google.com [209.85.218.50]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 7ADAD38BB for ; Sun, 8 May 2022 13:49:51 -0700 (PDT) Received: by mail-ej1-f50.google.com with SMTP id y3so23298351ejo.12 for ; Sun, 08 May 2022 13:49:51 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:message-id:date:mime-version:user-agent:reply-to :subject:content-language:to:cc:references:from:in-reply-to :content-transfer-encoding; bh=N3u3DsBrtjhbjGZ+KXEeJGBa+J91FAgZg94Zd6yuT7w=; b=OPoNglGfV+5DXE9pvz+l899QCwZCacAMYMjszaSnokeNn9cpoHhArp1/EgpxTZSiWy jaRcPWN0DMQisyXj+106l4gkQ1ZrcFmVOxw0mtVimIi5xpXuHFB7LGEVlmMx994fDvI5 /r0IrtbFT2kVsB8VGK88bsn8pVTjqig8kgFGBdGtVVOPs4nWVOU7tdpxTPICY7e/ccDh dDfIUs5j22ZPPYJr5GA2uPb2JzQCDh1AVrAgN3H00m/FqXZLkFvIOmUQz2yeM9rRnIYO ZhrrFdU+puj7qhNM4Y7S2+ENxKTa0YfQwol0jhq83PaGNrlGGherXGj0yuyaTnGIYzny 8W2Q== X-Gm-Message-State: AOAM533sVDelkbjN/7arnfL/rWiqsyYABM+wlcYnpcEsFKbahybKijRs mu8S6tkg2jPJV1eBIqTf5/E= X-Google-Smtp-Source: ABdhPJw5eG+Ips+GenyVP3VdatI+SK51I5pSEukIkiR9R8KOj/eQOSo8c103NESr6ebBqGkOdOxH3w== X-Received: by 2002:a17:907:a088:b0:6f4:f661:f77a with SMTP id hu8-20020a170907a08800b006f4f661f77amr12070854ejc.77.1652042989945; Sun, 08 May 2022 13:49:49 -0700 (PDT) Received: from [10.9.0.34] ([46.166.128.205]) by smtp.gmail.com with ESMTPSA id b18-20020aa7dc12000000b0042617ba63acsm5276498edu.54.2022.05.08.13.49.47 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Sun, 08 May 2022 13:49:49 -0700 (PDT) Message-ID: Date: Sun, 8 May 2022 23:49:46 +0300 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101 Thunderbird/91.7.0 Reply-To: alex.popov@linux.com Subject: Re: [PATCH v2 05/13] stackleak: clarify variable names Content-Language: en-US To: Mark Rutland , linux-arm-kernel@lists.infradead.org Cc: akpm@linux-foundation.org, catalin.marinas@arm.com, keescook@chromium.org, linux-kernel@vger.kernel.org, luto@kernel.org, will@kernel.org References: <20220427173128.2603085-1-mark.rutland@arm.com> <20220427173128.2603085-6-mark.rutland@arm.com> From: Alexander Popov In-Reply-To: <20220427173128.2603085-6-mark.rutland@arm.com> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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`. > 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. 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? > 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 */ 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 C62A1C433F5 for ; Sun, 8 May 2022 20:51:09 +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-Type: Content-Transfer-Encoding:Reply-To:List-Subscribe:List-Help:List-Post: List-Archive:List-Unsubscribe:List-Id:In-Reply-To:From:References:Cc:To: Subject:MIME-Version:Date:Message-ID:Content-ID:Content-Description: Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID: List-Owner; bh=k+kCmkijb5rY8rrmZOYOkUebhx+TbM7gJ+bx3/MjtVU=; b=hPHjS8RqP6wFqT cCYJ7Q9r4YT8QKQSkPXxH4apq+ttbUnxEWAQKlEeUD/3t2Mmc4bST72blaVIp9ERF2/pOHmlsCqBC ML3Zm0bThL2s3+l4NnbdR2sHSA7ZTBh925rMl8+LEkOS0WJV5ry+MS/DAr8vrM6cFz+d1KeEc3Kde Y2WRHDmB9WhZvbUqE0A7+HHxk+qb4pTtzbyFJ7dlihnfKtDQt1TixqMm9r8b5rx9AUmt2M86qttIq 0lSSbwRfRTVdnsog2UwBh6wbFLVKlKpq8SG81KqVcqvV2c6oLfnxdeqbFabGOGao9E5fXfGY68PLZ HwRCC+m34dbZ5YP7QZNA==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.94.2 #2 (Red Hat Linux)) id 1nnnqr-00BWXU-En; Sun, 08 May 2022 20:50:01 +0000 Received: from mail-ej1-f52.google.com ([209.85.218.52]) by bombadil.infradead.org with esmtps (Exim 4.94.2 #2 (Red Hat Linux)) id 1nnnqh-00BWUJ-UG for linux-arm-kernel@lists.infradead.org; Sun, 08 May 2022 20:49:59 +0000 Received: by mail-ej1-f52.google.com with SMTP id ks9so16932682ejb.2 for ; Sun, 08 May 2022 13:49:51 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:message-id:date:mime-version:user-agent:reply-to :subject:content-language:to:cc:references:from:in-reply-to :content-transfer-encoding; bh=N3u3DsBrtjhbjGZ+KXEeJGBa+J91FAgZg94Zd6yuT7w=; b=tB8XOdHNmUE5Xv7kNfowRRb5l2zFKNoHdGC6i2MXo69uWGPI9whdu9LEc03ny1+UXt C1qTPUkNmf4dQPlkYvrViaL+NovoogHFNvo0hH5kVMlbDjYoBv72QRNHl2Dz5F1a8AZF ++hGXWQvbHbaum2boQmsMZXzGyAiBv1KM0pf4SFfUbu5ebk5QdSYXp57rby8rIBqxxS2 w63d7Y/svy8D3MUNRs4RisagkdSR3kxzhOoZejQQ6AdvnP1MxzqlOhJKoxIAjqppucul iMt3JdcVtWooXwrtYBpEeeeVKNPuRvfyqzMqkSchq/HTW8G2L/vG/Gv9/GArOkMo3IC2 zCeQ== X-Gm-Message-State: AOAM531HjaLQ0X33oTS15znb23Hkdk2WskHtyePjgkD6t9odvfQPcUDR WBzLfjJlGEB8L14zEJX9P1TSG6WPXb0= X-Google-Smtp-Source: ABdhPJw5eG+Ips+GenyVP3VdatI+SK51I5pSEukIkiR9R8KOj/eQOSo8c103NESr6ebBqGkOdOxH3w== X-Received: by 2002:a17:907:a088:b0:6f4:f661:f77a with SMTP id hu8-20020a170907a08800b006f4f661f77amr12070854ejc.77.1652042989945; Sun, 08 May 2022 13:49:49 -0700 (PDT) Received: from [10.9.0.34] ([46.166.128.205]) by smtp.gmail.com with ESMTPSA id b18-20020aa7dc12000000b0042617ba63acsm5276498edu.54.2022.05.08.13.49.47 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Sun, 08 May 2022 13:49:49 -0700 (PDT) Message-ID: Date: Sun, 8 May 2022 23:49:46 +0300 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101 Thunderbird/91.7.0 Subject: Re: [PATCH v2 05/13] stackleak: clarify variable names Content-Language: en-US To: Mark Rutland , linux-arm-kernel@lists.infradead.org Cc: akpm@linux-foundation.org, catalin.marinas@arm.com, keescook@chromium.org, linux-kernel@vger.kernel.org, luto@kernel.org, will@kernel.org References: <20220427173128.2603085-1-mark.rutland@arm.com> <20220427173128.2603085-6-mark.rutland@arm.com> From: Alexander Popov In-Reply-To: <20220427173128.2603085-6-mark.rutland@arm.com> X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20220508_134952_020303_D909A470 X-CRM114-Status: GOOD ( 31.52 ) 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: , Reply-To: alex.popov@linux.com Content-Transfer-Encoding: 7bit Content-Type: text/plain; charset="us-ascii"; Format="flowed" Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org 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`. > 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. 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? > 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