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 8DB24C43334 for ; Fri, 3 Jun 2022 16:57:08 +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=yY2Xpb/xaj1DXV7sQ86Xb4zd7w6OBSVmh36WqRrVFYM=; b=VI8En57R16AXEr 1fGYj7CqnxdNowqBO1t4QuxR5catZZSAt1J/vbYxBaO0msDyQQRlF/TUZMlwmbfzUjJ+//CsMH+84 iOwty/9U6eQMdVbCefZOl7vZcf47abtG6RpCh5JS7ZuHdy6Dj/DzNgATZUMrfm2VTu4zqYf1SNm/I nqg4QTz7QA+Gm3U8TF5y9tw1voTWwu7vcnbIvPCeOPxzRcl7gfKQGH+1GmXqWL1vIFnhR+yPf0zcU k5fX7q7h2z/3Gmca7TCEU11BJcXJDIM7PdBdZEsC4cDfCUbMy/1GfhrVXMR+vgzps475M8Hpyvskr ZuuqLUYC3VAXBa+EGVgw==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.94.2 #2 (Red Hat Linux)) id 1nxAag-008E4y-3y; Fri, 03 Jun 2022 16:56:02 +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 1nxAac-008E3w-Rg for linux-arm-kernel@lists.infradead.org; Fri, 03 Jun 2022 16:56:00 +0000 Received: by mail-ej1-f52.google.com with SMTP id h23so5992790ejj.12 for ; Fri, 03 Jun 2022 09:55:57 -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=MFtoHW1bgltDv8sgWfWuiBl/gfDMFhsXm0Yso4Y2E0E=; b=DyDyrJIPfh/P5nSSDlYnYAWRZjoM125BS68AfDBeX4kBflRAHUV1csMXXsn0UwFJdG wVUUBAeOVWouKrFwVqGt8uk9GoqSZT6hrz7xTNAeK/2Wby9a+uomMM/zrX5lXBJ6xhAX e015ASSXS27CE7o+aMOSjiwFqqTSF6RVCtfdQOJ0gQzpKgcs9X3+b3yoENTWBzbdmGS+ 1slKAOk5VomIiVmwyVuNT2OmJAwa9xcmttYATHdTdS68bg2Aene9FVj6O9SMMjxx/8qv Vl8YoMuuXcmOvvFk/RuVR4a22XhbtJQhDf7FzqflQgOOKPtDSE7sF37xtXabhjHJJgwu RdUw== X-Gm-Message-State: AOAM5313T1dG9ehUUtO21rNGEqs7U1Og3sJG4YhhupCq+ZSaK/2ttirh qeaVvTYz8aot7fcWWmLnFiY= X-Google-Smtp-Source: ABdhPJyU0Bn1azYIg4Wh8a0Ih9Tg14FzbYzn/WtcAWiOnfAYhx7sfTIOSRq4gMbRe33lX07ahgPewA== X-Received: by 2002:a17:907:1689:b0:6fe:fcf4:128e with SMTP id hc9-20020a170907168900b006fefcf4128emr9746715ejc.446.1654275356418; Fri, 03 Jun 2022 09:55:56 -0700 (PDT) Received: from [10.9.0.34] ([46.166.133.199]) by smtp.gmail.com with ESMTPSA id lt9-20020a170906fa8900b006fec56a80a8sm2983714ejb.115.2022.06.03.09.55.53 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Fri, 03 Jun 2022 09:55:55 -0700 (PDT) Message-ID: <58e7e80d-204e-be39-2033-f02c59e122d1@linux.com> Date: Fri, 3 Jun 2022 19:55:51 +0300 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101 Thunderbird/91.9.0 Subject: Re: [PATCH v2 07/13] stackleak: rework poison scanning Content-Language: en-US To: Kees Cook Cc: Mark Rutland , linux-arm-kernel@lists.infradead.org, akpm@linux-foundation.org, catalin.marinas@arm.com, linux-kernel@vger.kernel.org, luto@kernel.org, will@kernel.org References: <20220427173128.2603085-1-mark.rutland@arm.com> <20220427173128.2603085-8-mark.rutland@arm.com> <268ea8f7-472b-f1d4-6b8b-0c8fefccc0fa@linux.com> <02e40030-52a5-f23c-85be-be59a7d3211c@linux.com> <73e3a82b-fbf6-5448-56ba-adda130230d3@linux.com> <202205311108.40D216E6@keescook> From: Alexander Popov In-Reply-To: <202205311108.40D216E6@keescook> X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20220603_095558_945439_ACA9B133 X-CRM114-Status: GOOD ( 28.06 ) 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 31.05.2022 21:13, Kees Cook wrote: > On Fri, May 27, 2022 at 02:25:12AM +0300, Alexander Popov wrote: >> On 24.05.2022 16:31, Mark Rutland wrote: >>> [...] >>> It's also worth noting that `noinstr` code will also not be instrumented >>> regardless of frame size -- we might want some build-time assertion for those. >> >> I developed a trick that shows noinstr functions that stackleak would like to instrument: >> >> diff --git a/scripts/gcc-plugins/stackleak_plugin.c b/scripts/gcc-plugins/stackleak_plugin.c >> index 42f0252ee2a4..6db748d44957 100644 >> --- a/scripts/gcc-plugins/stackleak_plugin.c >> +++ b/scripts/gcc-plugins/stackleak_plugin.c >> @@ -397,6 +397,9 @@ static unsigned int stackleak_cleanup_execute(void) >> const char *fn = DECL_NAME_POINTER(current_function_decl); >> bool removed = false; >> >> + if (verbose) >> + fprintf(stderr, "stackleak: I see noinstr function %s()\n", fn); >> + >> /* >> * Leave stack tracking in functions that call alloca(). >> * Additional case: >> @@ -464,12 +467,12 @@ static bool stackleak_gate(void) >> if (STRING_EQUAL(section, ".meminit.text")) >> return false; >> if (STRING_EQUAL(section, ".noinstr.text")) >> - return false; >> + return true; >> if (STRING_EQUAL(section, ".entry.text")) >> return false; >> } >> >> - return track_frame_size >= 0; >> + return false; >> } >> >> /* Build the function declaration for stackleak_track_stack() */ >> @@ -589,8 +592,6 @@ __visible int plugin_init(struct plugin_name_args *plugin_info, >> build_for_x86 = true; >> } else if (!strcmp(argv[i].key, "disable")) { >> disable = true; >> - } else if (!strcmp(argv[i].key, "verbose")) { >> - verbose = true; >> } else { >> error(G_("unknown option '-fplugin-arg-%s-%s'"), >> plugin_name, argv[i].key); >> @@ -598,6 +599,8 @@ __visible int plugin_init(struct plugin_name_args *plugin_info, >> } >> } >> >> + verbose = true; >> + >> if (disable) { >> if (verbose) >> fprintf(stderr, "stackleak: disabled for this translation unit\n"); >> >> >> Building defconfig for x86_64 gives this: >> >> stackleak: I see noinstr function __do_fast_syscall_32() >> stackleak: instrument __do_fast_syscall_32(): calls_alloca >> -- >> stackleak: I see noinstr function do_syscall_64() >> stackleak: instrument do_syscall_64(): calls_alloca >> -- >> stackleak: I see noinstr function do_int80_syscall_32() >> stackleak: instrument do_int80_syscall_32(): calls_alloca > > As you say, these are from RANDOMIZE_KSTACK_OFFSET, and are around > bounds-checked, and should already be getting wiped since these will > call into deeper (non-noinst) functions. Kees, it crossed my mind that for correct stack erasing the kernel with RANDOMIZE_KSTACK_OFFSET needs at least one stackleak_track_stack() call during the syscall handling. Otherwise current->lowest_stack would point to the stack address where no stack frame was placed because of alloca with random size. Am I right? How about calling stackleak_track_stack() explicitly after the kernel stack randomization? >> stackleak: I see noinstr function do_machine_check() >> stackleak: instrument do_machine_check() >> -- >> stackleak: I see noinstr function exc_general_protection() >> stackleak: instrument exc_general_protection() >> -- >> stackleak: I see noinstr function fixup_bad_iret() >> stackleak: instrument fixup_bad_iret() >> >> >> The cases with calls_alloca are caused by CONFIG_RANDOMIZE_KSTACK_OFFSET=y. >> Kees knows about that peculiarity. >> >> Other cases are noinstr functions with large stack frame: >> do_machine_check(), exc_general_protection(), and fixup_bad_iret(). >> >> I think adding a build-time assertion is not possible, since it would break >> building the kernel. > > Do these functions share the syscall behavior of always calling into > non-noinst functions that _do_ have stack depth instrumentation? This is a right question. I can't say for sure, but it looks like do_machine_check(), exc_general_protection() and fixup_bad_iret() do some low-level exception/trap handling and don't affect syscall handling. Do you agree? >> [...] >>> In security/Kconfig.hardening we have: >>> >>> | config STACKLEAK_TRACK_MIN_SIZE >>> | int "Minimum stack frame size of functions tracked by STACKLEAK" >>> | default 100 >>> | range 0 4096 >>> | depends on GCC_PLUGIN_STACKLEAK >>> | help >>> | The STACKLEAK gcc plugin instruments the kernel code for tracking >>> | the lowest border of the kernel stack (and for some other purposes). >>> | It inserts the stackleak_track_stack() call for the functions with >>> | a stack frame size greater than or equal to this parameter. >>> | If unsure, leave the default value 100. >>> >>> ... where the vast majority of that range is going to lead to a BUILD_BUG(). >> >> Honestly, I don't like the idea of having the STACKLEAK_TRACK_MIN_SIZE option in the Kconfig. >> >> I was forced by the maintainers to introduce it when I was working on the stackleak patchset. >> >> How about dropping CONFIG_STACKLEAK_TRACK_MIN_SIZE from Kconfig? >> >> That would also allow to drop this build-time assertion. > > Should this be arch-specific? (i.e. just make it a per-arch Kconfig > default, instead of user-selectable into weird values?) I don't think CONFIG_STACKLEAK_TRACK_MIN_SIZE is arch-specific, since STACKLEAK_SEARCH_DEPTH is the same for all architectures that support stackleak. Best regards, Alexander _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel