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 D569DC433F5 for ; Sun, 8 May 2022 21:28:37 +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=w1MveHTzmEDpVuv4O77LwRIchuGd+Ha6j3VAFDrqMO8=; b=Qpxa6lJFjdWCzf gNNt+Kyzk4MeWYN8pdYFuSARRYsrS35ynMGKQcT2cF3Kh/jnWIE6AuazTj7WiBk6qC7vX+cgID3rj HA5/6QXhSxC3ZiYfcj697BCQBCe2uMe5WwOBq0pACGY1swlSjOTcpgQWlWd1dVg8qxXrHVlLUkA1B 0BsCWMTeviXeCf4Ocz1jf+wJhyClNGcFY9eyjajo4mlk3F7iKn1tmAEF/4Hjnh8EMvdw9sVuCPZwB 6ndwolkz8QcE8OoL8rYsg092urkLrQuAPtH9ee+9TcZTf0oq4TULXdwc7gdhTvvNqEqnDdHdn8bkR 2aKbTXp3bux4IyY0whCw==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.94.2 #2 (Red Hat Linux)) id 1nnoR9-00BatJ-26; Sun, 08 May 2022 21:27:31 +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 1nnoR5-00Basw-O4 for linux-arm-kernel@lists.infradead.org; Sun, 08 May 2022 21:27:29 +0000 Received: by mail-ej1-f52.google.com with SMTP id j6so23409551ejc.13 for ; Sun, 08 May 2022 14:27:27 -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=hCHtx9BuNNSfq1aRuUx+wGgoggwlZE/FbOgWdTxGgow=; b=tyOVqT0QrMnmK3rQxTlXXCeUZyGf9zAEQ9nVM5q6CE1qwu+RjsJ4vjkAapRXIVRlBs tPGXtzo2g1xjrMAHM4Y+NJ4hs45wgXMxsiwm4HtDXm3zQOzmnIvom8aSlRrB9Jrir9W1 rKLaDOXwTBdXli0Yc6AOftunMgCpZ4Z4NW8OKYKdcUajfat/EDcyknQqAOdSokPA+pHl k4hdEsmnnoSNWmq/C4mTJi9cO9K7Wrl16Nx+/9IzHwew0iJ2wWbVb707/9yQIzSE70+L AWXq+QmbG9zbqIvlyWnzfQTehUBdRxD+56OzWziTdXwhcaIaf/BaCRhTDyn8MU1M4Gpy Wn6w== X-Gm-Message-State: AOAM533NtCXfoojamXpbX8DdmAAsKIZePln5/Zqoas0UuP11MTkCFb0W SZ3TgfEtNF93R63uyVLcijc= X-Google-Smtp-Source: ABdhPJxvvF+a9KSbPswCVEEc/fm/hcT6xNkpN9vsYCD2dpuz4CfF3YqiVEi6n30L14fIUvrwBIE3oA== X-Received: by 2002:a17:907:980f:b0:6f8:616f:eccc with SMTP id ji15-20020a170907980f00b006f8616fecccmr6615087ejc.381.1652045246340; Sun, 08 May 2022 14:27:26 -0700 (PDT) Received: from [10.9.0.34] ([46.166.128.205]) by smtp.gmail.com with ESMTPSA id bo9-20020a0564020b2900b0042617ba63d5sm5243357edb.95.2022.05.08.14.27.24 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Sun, 08 May 2022 14:27:25 -0700 (PDT) Message-ID: Date: Mon, 9 May 2022 00:27:22 +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 06/13] stackleak: rework stack high bound handling 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-7-mark.rutland@arm.com> From: Alexander Popov In-Reply-To: <20220427173128.2603085-7-mark.rutland@arm.com> X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20220508_142727_841985_AD1C813E X-CRM114-Status: GOOD ( 43.90 ) 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: > Prior to returning to userpace, we reset current->lowest_stack to a > reasonable high bound. Currently we do this by subtracting the arbitrary > value `THREAD_SIZE/64` from the top of the stack, for reasons lost to > history. > > Looking at configurations today: > > * On i386 where THREAD_SIZE is 8K, the bound will be 128 bytes. The > pt_regs at the top of the stack is 68 bytes (with 0 to 16 bytes of > padding above), and so this covers an additional portion of 44 to 60 > bytes. > > * On x86_64 where THREAD_SIZE is at least 16K (up to 32K with KASAN) the > bound will be at least 256 bytes (up to 512 with KASAN). The pt_regs > at the top of the stack is 168 bytes, and so this cover an additional > 88 bytes of stack (up to 344 with KASAN). > > * On arm64 where THREAD_SIZE is at least 16K (up to 64K with 64K pages > and VMAP_STACK), the bound will be at least 256 bytes (up to 1024 with > KASAN). The pt_regs at the top of the stack is 336 bytes, so this can > fall within the pt_regs, or can cover an additional 688 bytes of > stack. > > Clearly the `THREAD_SIZE/64` value doesn't make much sense -- in the > worst case, this will cause more than 600 bytes of stack to be erased > for every syscall, even if actual stack usage were substantially > smaller. > > This patches makes this slightly less nonsensical by consistently > resetting current->lowest_stack to the base of the task pt_regs. For > clarity and for consistency with the handling of the low bound, the > generation of the high bound is split into a helper with commentary > explaining why. > > Since the pt_regs at the top of the stack will be clobbered upon the > next exception entry, we don't need to poison these at exception exit. > By using task_pt_regs() as the high stack boundary instead of > current_top_of_stack() we avoid some redundant poisoning, and the > compiler can share the address generation between the poisoning and > restting of `current->lowest_stack`, making the generated code more > optimal. > > It's not clear to me whether the existing `THREAD_SIZE/64` offset was a > dodgy heuristic to skip the pt_regs, or whether it was attempting to > minimize the number of times stackleak_check_stack() would have to > update `current->lowest_stack` when stack usage was shallow at the cost > of unconditionally poisoning a small portion of the stack for every exit > to userspace. I inherited this 'THREAD_SIZE/64' logic is from the original grsecurity patch. As I mentioned, originally this was written in asm. For x86_64: mov TASK_thread_sp0(%r11), %rdi sub $256, %rdi mov %rdi, TASK_lowest_stack(%r11) For x86_32: mov TASK_thread_sp0(%ebp), %edi sub $128, %edi mov %edi, TASK_lowest_stack(%ebp) 256 bytes for x86_64 and 128 bytes for x86_32 are exactly THREAD_SIZE/64. I think this value was chosen as optimal for minimizing poison scanning. It's possible that stackleak_track_stack() is not called during the syscall because all the called functions have small stack frames. > For now I've simply removed the offset, and if we need/want to minimize > updates for shallow stack usage it should be easy to add a better > heuristic atop, with appropriate commentary so we know what's going on. I like your idea to erase the thread stack up to pt_regs if we call the stackleak erasing from the trampoline stack. But here I don't understand where task_pt_regs() points to... > Signed-off-by: Mark Rutland > Cc: Alexander Popov > Cc: Andrew Morton > Cc: Andy Lutomirski > Cc: Kees Cook > --- > include/linux/stackleak.h | 14 ++++++++++++++ > kernel/stackleak.c | 19 ++++++++++++++----- > 2 files changed, 28 insertions(+), 5 deletions(-) > > diff --git a/include/linux/stackleak.h b/include/linux/stackleak.h > index 67430faa5c518..467661aeb4136 100644 > --- a/include/linux/stackleak.h > +++ b/include/linux/stackleak.h > @@ -28,6 +28,20 @@ stackleak_task_low_bound(const struct task_struct *tsk) > return (unsigned long)end_of_stack(tsk) + sizeof(unsigned long); > } > > +/* > + * The address immediately after the highest address on tsk's stack which we > + * can plausibly erase. > + */ > +static __always_inline unsigned long > +stackleak_task_high_bound(const struct task_struct *tsk) > +{ > + /* > + * The task's pt_regs lives at the top of the task stack and will be > + * overwritten by exception entry, so there's no need to erase them. > + */ > + return (unsigned long)task_pt_regs(tsk); > +} > + > 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 d5f684dc0a2d9..ba346d46218f5 100644 > --- a/kernel/stackleak.c > +++ b/kernel/stackleak.c > @@ -73,6 +73,7 @@ late_initcall(stackleak_sysctls_init); > 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; > @@ -93,14 +94,22 @@ static __always_inline void __stackleak_erase(void) > #endif > > /* > - * 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. > + * Write poison to the task's stack between 'erase_low' and > + * 'erase_high'. > + * > + * If we're running on a different stack (e.g. an entry trampoline > + * stack) we can erase everything below the pt_regs at the top of the > + * task stack. > + * > + * If we're running on the task stack itself, we must not clobber any > + * stack used by this function and its caller. We assume that this > + * function has a fixed-size stack frame, and the current stack pointer > + * doesn't change while we write poison. > */ > if (on_thread_stack()) > erase_high = current_stack_pointer; > else > - erase_high = current_top_of_stack(); > + erase_high = task_stack_high; > > while (erase_low < erase_high) { > *(unsigned long *)erase_low = STACKLEAK_POISON; > @@ -108,7 +117,7 @@ static __always_inline void __stackleak_erase(void) > } > > /* Reset the 'lowest_stack' value for the next syscall */ > - current->lowest_stack = current_top_of_stack() - THREAD_SIZE/64; > + current->lowest_stack = task_stack_high; > } > > asmlinkage void noinstr stackleak_erase(void) _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel