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 3D532C433F5 for ; Tue, 31 May 2022 18:14:07 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1346917AbiEaSOD (ORCPT ); Tue, 31 May 2022 14:14:03 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:33120 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1346878AbiEaSNx (ORCPT ); Tue, 31 May 2022 14:13:53 -0400 Received: from mail-pj1-x1034.google.com (mail-pj1-x1034.google.com [IPv6:2607:f8b0:4864:20::1034]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 39C188CCCB for ; Tue, 31 May 2022 11:13:46 -0700 (PDT) Received: by mail-pj1-x1034.google.com with SMTP id d12-20020a17090abf8c00b001e2eb431ce4so3048420pjs.1 for ; Tue, 31 May 2022 11:13:46 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=chromium.org; s=google; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to; bh=fC3UkvWOG+QmFlfZQIlGHU889ZsG1Db1WvDJ8i9YBsM=; b=ld8O+fyZnmR7brIc1eUdB/k5VS0H7xLdq6W000QIFS+eJmuMrcwOa06jYK3TBK8OjJ VcsylE0GjOnWA3R6yX+3k0c71K5ZhkjueM36n38a74gRBL0bAr52+IDakG8cTvjQwYjm /550UJPyKL6O1KC6Q75oVDO5MzvYunATn1c4I= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to; bh=fC3UkvWOG+QmFlfZQIlGHU889ZsG1Db1WvDJ8i9YBsM=; b=qsxL2I3bvGoATplea4q48AceH8kr29tOjR81/tQaCjQgjiBh5fvBVO2FATjo6n3Zax nOb4jg9hhUHIZ8NESod8qq7uFvDouzG35eoKNrg5mCNrTgHM9nfsoFGZPVYvoR59HkBQ 7JgycewILHdwb84UwYSmrQDpI3IH4VreGz2EKQOJECHL6EwL9B+Uqw7FYdPvzHNx4ZWr zkFMPXTKsKIecG2W7ZC6rN4L5MGNIIeOCLS6S9QzUvr/HyNDZnVBZHIIyuGi84mb3dRN OdkhjwDqSNseipBt/2tq+Q20rJ+ylHA4rpFGknmdttuWWwhk1I/kby3GHmptRmAkpljI 493w== X-Gm-Message-State: AOAM530/LM5PO9zHFwf+Z1DX361BZHfvZRz0bC1D/l+0Azy3utHtKCDx 8vihLw+j+H3RlLLe70GxFAMpFQ== X-Google-Smtp-Source: ABdhPJxy0oanyehmUSaDd8NgjuZe98D1SD/vq/qC2ZS9dS5eOJTBxze4un0W7pOhM4liLACJKTna7A== X-Received: by 2002:a17:90a:181:b0:1e0:6535:b616 with SMTP id 1-20020a17090a018100b001e06535b616mr29350041pjc.154.1654020825425; Tue, 31 May 2022 11:13:45 -0700 (PDT) Received: from www.outflux.net (smtp.outflux.net. [198.145.64.163]) by smtp.gmail.com with ESMTPSA id q11-20020a170903204b00b00163be997587sm6420695pla.100.2022.05.31.11.13.45 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Tue, 31 May 2022 11:13:45 -0700 (PDT) Date: Tue, 31 May 2022 11:13:44 -0700 From: Kees Cook To: Alexander Popov 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 Subject: Re: [PATCH v2 07/13] stackleak: rework poison scanning Message-ID: <202205311108.40D216E6@keescook> 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> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <73e3a82b-fbf6-5448-56ba-adda130230d3@linux.com> Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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. > 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? > [...] > > 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?) -- Kees Cook 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 60EADC433FE for ; Tue, 31 May 2022 18:14: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=8DV7nCTSp2RrjO0sYV4MrKJp2bfoG3UJeHL9GyVxHW0=; b=BfR6aTNBLD++Ud rSlqqXU2DRzFRBvtaTmlrKMz0tdmiSFTZbxpNGZXOayw0x55rM96/JvqGlPygsEEJ9SsiVij1ERwy JPlsYVonrzK5kWz9oqKmpG1SiY2/Y5jF8oH2BTJNsQaAmbFuiMLEMRLTW1JQaL1U+e9sE/UaOIgUv JNDwEuF7AOeYBxEnk0YKdIBsLGJAocdXJgB3UZmZ59xNrUH+TMAry5/jxbUA//Ap4Ao0PyE4dVBBA zBCdREOkMrWjxWIWSySSC+CQiEnU5dHQ/TI2nqYDH8QndBG5gxFNnReHYoXHdvTzqtCuF2cwBG3u+ NNinDdcSPGOWHaMIEIUQ==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.94.2 #2 (Red Hat Linux)) id 1nw6NK-00C8kS-Ip; Tue, 31 May 2022 18:13:50 +0000 Received: from mail-pj1-x1036.google.com ([2607:f8b0:4864:20::1036]) by bombadil.infradead.org with esmtps (Exim 4.94.2 #2 (Red Hat Linux)) id 1nw6NH-00C8jG-8B for linux-arm-kernel@lists.infradead.org; Tue, 31 May 2022 18:13:49 +0000 Received: by mail-pj1-x1036.google.com with SMTP id l20-20020a17090a409400b001dd2a9d555bso3069302pjg.0 for ; Tue, 31 May 2022 11:13:45 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=chromium.org; s=google; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to; bh=fC3UkvWOG+QmFlfZQIlGHU889ZsG1Db1WvDJ8i9YBsM=; b=ld8O+fyZnmR7brIc1eUdB/k5VS0H7xLdq6W000QIFS+eJmuMrcwOa06jYK3TBK8OjJ VcsylE0GjOnWA3R6yX+3k0c71K5ZhkjueM36n38a74gRBL0bAr52+IDakG8cTvjQwYjm /550UJPyKL6O1KC6Q75oVDO5MzvYunATn1c4I= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to; bh=fC3UkvWOG+QmFlfZQIlGHU889ZsG1Db1WvDJ8i9YBsM=; b=0UpsCNCist+EO5rsIq4ctu2tIGR4ES3tt2dnfiwR/QCN9vJaieTlgHtTiaI12P6UMD mERHN4FP5EnGvcnyP4C+ix7kfXOJaKjVU4tAS1RjJgGkXtQIvI5whEIyNrKvw4hGQPBC DlaohhQHz7qY4CbYBi75XrM3rgAKS7fsRxLL9aEUZadE+++4q8mP2kcv4mgjXidPYpqu YyRUkbKsACg+Eef4/1BXAXGR5EqiA6vRsYcodUZx0bR/WftHZ0W3JUWXMTtt+kH2e2q7 WYkBIAPH52lzY5/nLmSNsCXysi6jUn9jW57SgOrS+Ub9qFnhBHbvaBGA7Prfks1AWoRb 2T/w== X-Gm-Message-State: AOAM532v5znuRGRobDWNEUxlrtWVb2nBCwo1HY35t3CXP1IheIutv1ij ifAilo7A4XyZhwv8pFcaOuMk2Q== X-Google-Smtp-Source: ABdhPJxy0oanyehmUSaDd8NgjuZe98D1SD/vq/qC2ZS9dS5eOJTBxze4un0W7pOhM4liLACJKTna7A== X-Received: by 2002:a17:90a:181:b0:1e0:6535:b616 with SMTP id 1-20020a17090a018100b001e06535b616mr29350041pjc.154.1654020825425; Tue, 31 May 2022 11:13:45 -0700 (PDT) Received: from www.outflux.net (smtp.outflux.net. [198.145.64.163]) by smtp.gmail.com with ESMTPSA id q11-20020a170903204b00b00163be997587sm6420695pla.100.2022.05.31.11.13.45 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Tue, 31 May 2022 11:13:45 -0700 (PDT) Date: Tue, 31 May 2022 11:13:44 -0700 From: Kees Cook To: Alexander Popov 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 Subject: Re: [PATCH v2 07/13] stackleak: rework poison scanning Message-ID: <202205311108.40D216E6@keescook> 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> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <73e3a82b-fbf6-5448-56ba-adda130230d3@linux.com> X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20220531_111347_356719_4895FC2F X-CRM114-Status: GOOD ( 35.49 ) 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 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. > 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? > [...] > > 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?) -- Kees Cook _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel