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 BEA36C433EF for ; Wed, 11 May 2022 08:04:20 +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=yN0jbXeN5dnVnnEXDDoqSDgYs3PTe8tgu4rc1Pq3Fwg=; b=AsvYwIUdwSXKEM Bs0F/ZPugG/JZ3ZrOEQqfc/Zk0Nbwyvftx6qGcuTTNYJmLrQZgLgqE4D8yxydQEbi5jhVr85OcO5l Ukha10mWYriD77NCjsTQaYSO7iGSSWueTA/kAmFWuolbOJnnqbsWSXeQsj2vquRaDmn8MFzWuYw8U q05IB0SUKfNC0Vps1RkgqLVdZ1X0UwZl6Cmf/QCZfXreMi6jw+jUoqLMGmSGT6vEF3HAU0BRurGwo YSEABK6f++4PnDAxcUKrUjtxcO8MhDr0adCoTP2d7LzDPk6vG0P597BRJQBT+opaL+gCHlZj0xTW6 yicJCzSvDbh+7DDiUALA==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.94.2 #2 (Red Hat Linux)) id 1nohJM-005nYZ-45; Wed, 11 May 2022 08:03:08 +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 1nohJH-005nWn-Ih for linux-arm-kernel@lists.infradead.org; Wed, 11 May 2022 08:03:06 +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 3DEC8106F; Wed, 11 May 2022 01:03:00 -0700 (PDT) Received: from FVFF77S0Q05N (unknown [10.57.3.187]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id C8E5C3F73D; Wed, 11 May 2022 01:02:58 -0700 (PDT) Date: Wed, 11 May 2022 09:02:45 +0100 From: Mark Rutland To: Kees Cook Cc: Alexander Popov , 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 03/13] stackleak: remove redundant check Message-ID: References: <20220427173128.2603085-1-mark.rutland@arm.com> <20220427173128.2603085-4-mark.rutland@arm.com> <202205101958.2A33DE20@keescook> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <202205101958.2A33DE20@keescook> X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20220511_010304_819312_81BFDD21 X-CRM114-Status: GOOD ( 25.15 ) 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 Tue, May 10, 2022 at 08:00:38PM -0700, Kees Cook wrote: > On Tue, May 10, 2022 at 12:46:48PM +0100, Mark Rutland wrote: > > On Sun, May 08, 2022 at 09:17:01PM +0300, Alexander Popov wrote: > > > On 27.04.2022 20:31, Mark Rutland wrote: > > > > In __stackleak_erase() we check that the `erase_low` value derived from > > > > `current->lowest_stack` is above the lowest legitimate stack pointer > > > > value, but this is already enforced by stackleak_track_stack() when > > > > recording the lowest stack value. > > > > > > > > Remove the redundant check. > > > > > > > > There should be no functional change as a result of this patch. > > > > > > Mark, I can't agree here. I think this check is important. > > > The performance profit from dropping it is less than the confidence decrease :) > > > > > > With this check, if the 'lowest_stack' value is corrupted, stackleak doesn't > > > overwrite some wrong kernel memory, but simply clears the whole thread > > > stack, which is safe behavior. > > > > If you feel strongly about it, I can restore the check, but I struggle to > > believe that it's worthwhile. The `lowest_stack` value lives in the > > task_struct, and if you have the power to corrupt that you have the power to do > > much more interesting things. > > > > If we do restore it, I'd like to add a big fat comment explaining the > > rationale (i.e. that it only matter if someone could corrupt > > `current->lowest_stack`, as otherwise that's guarnateed to be within bounds). > > Yeah, let's restore it and add the comment. While I do agree it's likely > that such an corruption would likely mean an attacker had significant > control over kernel memory already, it is not uncommon that an attack > only has a limited index from a given address, etc. Or some manipulation > is possible via weird gadgets, etc. It's unlikely, but not impossible, > and a bounds-check for that value is cheap compared to the rest of the > work happening. :) Fair enough; I can go spin a patch restoring this. I'm somewhat unhappy with silently fixing that up, though -- IMO it'd be better to BUG() or similar in that case. Thanks, Mark. _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel