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 ADC9AC27C79 for ; Thu, 20 Jun 2024 04:04:50 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20210309; h=Sender:List-Subscribe:List-Help :List-Post:List-Archive:List-Unsubscribe:List-Id:Content-Transfer-Encoding: Content-Type:In-Reply-To:From:References:CC:To:Subject:MIME-Version:Date: Message-ID:Reply-To:Content-ID:Content-Description:Resent-Date:Resent-From: Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID:List-Owner; bh=NTIfvDpUVoCVnlWxIArLWi4PboYHG8L8vZrlGXJQ4g8=; b=kpZqtUNippt9e+xEpn/X4lI3GV 7gNKZtQjo4RqipTPmZeSAoWo8IeTg4M1yiXHRO6kpMBb9SD2npFbq59stYcFntn4OjXxnm63WgbT7 h+2U0olhL6lX6BuGq4wfhi5W/FKu+6yPGupXMTvhcBZWV/0hTSK/4PIa2ct94I7IK5LF/H5tNV2mQ NaahLrwO96hWERX+7EQr/OuyYkkdI26wzl4IhwZEhdLHluOaUmtzaYw/O+emN4pXVPMPNaT4rDNSV vMFGrFu5BkWy4aC/RQa/FnkpXRfJf6d4ifTGKNP77jFMFuOZF0I4cI4g8eZSqgjPaV6m28bdx/OVu IrdjIJTw==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.97.1 #2 (Red Hat Linux)) id 1sK92J-00000003X7D-3qhZ; Thu, 20 Jun 2024 04:04:35 +0000 Received: from szxga02-in.huawei.com ([45.249.212.188]) by bombadil.infradead.org with esmtps (Exim 4.97.1 #2 (Red Hat Linux)) id 1sK92G-00000003X65-0rbo for linux-arm-kernel@lists.infradead.org; Thu, 20 Jun 2024 04:04:33 +0000 Received: from mail.maildlp.com (unknown [172.19.163.252]) by szxga02-in.huawei.com (SkyGuard) with ESMTP id 4W4RZS1JHmznWlM; Thu, 20 Jun 2024 11:59:28 +0800 (CST) Received: from dggpemd100004.china.huawei.com (unknown [7.185.36.20]) by mail.maildlp.com (Postfix) with ESMTPS id B6DF9180087; Thu, 20 Jun 2024 12:04:23 +0800 (CST) Received: from [10.67.109.211] (10.67.109.211) by dggpemd100004.china.huawei.com (7.185.36.20) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.2.1258.34; Thu, 20 Jun 2024 12:04:23 +0800 Message-ID: <4e137bd6-845c-41d0-9c96-85b4170ebf6a@huawei.com> Date: Thu, 20 Jun 2024 12:04:23 +0800 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH] remove AND operation in choose_random_kstack_offset() Content-Language: en-US To: Mark Rutland , Arnd Bergmann CC: Kees Cook , , , , , , Catalin Marinas , Will Deacon , Heiko Carstens , , Alexander Gordeev , Christian Borntraeger , Sven Schnelle , Thomas Gleixner , Ingo Molnar , Borislav Petkov , Dave Hansen , "H. Peter Anvin" , "Gustavo A. R. Silva" , Leonardo Bras , Mark Brown , , References: <20240617133721.377540-1-liuyuntao12@huawei.com> <202406171122.B5FDA6A@keescook> From: "liuyuntao (F)" In-Reply-To: Content-Type: text/plain; charset="UTF-8"; format=flowed Content-Transfer-Encoding: 7bit X-Originating-IP: [10.67.109.211] X-ClientProxiedBy: dggems706-chm.china.huawei.com (10.3.19.183) To dggpemd100004.china.huawei.com (7.185.36.20) X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20240619_210432_606212_78BA5954 X-CRM114-Status: GOOD ( 26.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: , Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org On 2024/6/18 18:45, Mark Rutland wrote: > Hi Arnd, > > On Mon, Jun 17, 2024 at 10:33:08PM +0200, Arnd Bergmann wrote: >> On Mon, Jun 17, 2024, at 20:22, Kees Cook wrote: >>> On Mon, Jun 17, 2024 at 04:52:15PM +0100, Mark Rutland wrote: >>>> On Mon, Jun 17, 2024 at 01:37:21PM +0000, Yuntao Liu wrote: >>>>> Since the offset would be bitwise ANDed with 0x3FF in >>>>> add_random_kstack_offset(), so just remove AND operation here. >>>>> >>>>> Signed-off-by: Yuntao Liu >>>> >>>> The comments in arm64 and x86 say that they're deliberately capping the >>>> offset at fewer bits than the result of KSTACK_OFFSET_MAX() masking the >>>> value with 0x3FF. >>>> >>>> Maybe it's ok to expand that, but if that's the case the commit message >>>> needs to explain why it's safe add extra bits (2 on arm64, 3 on s39 and >>>> x86), and those comments need to be updated accordingly. >>>> >>>> As-is, I do not think this patch is ok. >>> >>> Yeah, I agree: the truncation is intentional and tuned to the >>> architecture. >> >> It may be intentional, but it's clearly nonsense: there is nothing >> inherent to the architecture that means we have can go only 256 >> bytes instead of 512 bytes into the 16KB available stack space. >> >> As far as I can tell, any code just gets bloated to the point >> where it fills up the available memory, regardless of how >> much you give it. I'm sure one can find code paths today that >> exceed the 16KB, so there is no point pretending that 15.75KB >> is somehow safe to use while 15.00KB is not. >> >> I'm definitely in favor of making this less architecture >> specific, we just need to pick a good value, and we may well >> end up deciding to use less than the default 1KB. We can also >> go the opposite way and make the limit 4KB but then increase >> the default stack size to 20KB for kernels that enable >> randomization. > > Sorry, to be clear, I'm happy for this to change, so long as: > > * The commit message explains why that's safe. > > IIUC this goes from 511 to 1023 bytes on arm64, which is ~3% of the > stack, so maybe that is ok. It'd be nice to see any rationale/analysis > beyond "the offset would be bitwise ANDed with 0x3FF". > > * The comments in architecture code referring to the masking get > removed/updated along with the masking. > > My complaint was that the patch didn't do those things. > Sorry for that I don't adjust the comments in architecture code referring to the masking. I've tested the stack entropy by applying this patch on arm64. before: Bits of stack entropy: 6 after: Bits of stack entropy: 7 It seems the difference was minimal, so I didn't reflect it in the commit message. Now it appears that I missed some of the Kees's intentions. Kees has resent the patch, and everything should be fine now. Thanks! Yuntao > Mark.