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 1FF5DC5321E for ; Mon, 26 Aug 2024 10:01:49 +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:In-Reply-To:Content-Type: MIME-Version:References:Message-ID:Subject:Cc:To:From:Date:Reply-To: Content-Transfer-Encoding:Content-ID:Content-Description:Resent-Date: Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID:List-Owner; bh=4rsSXyaJ3QVy5Ua/sViPfTCQinmf5h3VNlhfNbenFMM=; b=pptFDojZ/RZ9KZo1kYlrvp1DUL FmKoHZMO/HKucoY20NC0dwpyr9Qc52yY4co6GDcW1hfhh+LqI2gh8Vz6bEsucVtJXfwOqb/1nCRWo tCj9BZSbO3wShHy0G0pdhuK9gzOsKq8WcozxyRI9kJRYxmrxbljrRVSbYjXF2/R+50GQ1PywV/AQP YjTOdisX2NoACPie9d501eROCeuPLhemjJ9sDXJde4HFg0ssXDOvaHhXMP5h5SzjJdjCqPtydDbSl 5k9U4hAOWVgPJD0Zn4VCe2w7KeYAHvVKzLKf+B0W7JmDsRnhVLD34+K4hTn8iDjyb1IuJRmDMFyTC oPDMxaVw==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.97.1 #2 (Red Hat Linux)) id 1siWXY-00000006pCE-2lYV; Mon, 26 Aug 2024 10:01:36 +0000 Received: from nyc.source.kernel.org ([147.75.193.91]) by bombadil.infradead.org with esmtps (Exim 4.97.1 #2 (Red Hat Linux)) id 1siWWA-00000006oYn-2Pzv; Mon, 26 Aug 2024 10:00:14 +0000 Received: from smtp.kernel.org (transwarp.subspace.kernel.org [100.75.92.58]) by nyc.source.kernel.org (Postfix) with ESMTP id 3546AA41C6E; Mon, 26 Aug 2024 10:00:02 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id ABF28C51400; Mon, 26 Aug 2024 10:00:01 +0000 (UTC) Date: Mon, 26 Aug 2024 13:00:09 +0300 From: Catalin Marinas To: Mark Brown Cc: Will Deacon , Jonathan Corbet , Andrew Morton , Marc Zyngier , Oliver Upton , James Morse , Suzuki K Poulose , Arnd Bergmann , Oleg Nesterov , Eric Biederman , Shuah Khan , "Rick P. Edgecombe" , Deepak Gupta , Ard Biesheuvel , Szabolcs Nagy , Kees Cook , "H.J. Lu" , Paul Walmsley , Palmer Dabbelt , Albert Ou , Florian Weimer , Christian Brauner , Thiago Jung Bauermann , Ross Burton , Yury Khrustalev , Wilco Dijkstra , linux-arm-kernel@lists.infradead.org, linux-doc@vger.kernel.org, kvmarm@lists.linux.dev, linux-fsdevel@vger.kernel.org, linux-arch@vger.kernel.org, linux-mm@kvack.org, linux-kselftest@vger.kernel.org, linux-kernel@vger.kernel.org, linux-riscv@lists.infradead.org Subject: Re: [PATCH v11 25/39] arm64/signal: Expose GCS state in signal frames Message-ID: References: <20240822-arm64-gcs-v11-0-41b81947ecb5@kernel.org> <20240822-arm64-gcs-v11-25-41b81947ecb5@kernel.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: X-TUID: eW4byJ2kTpsL X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20240826_030010_776547_6231ECE7 X-CRM114-Status: GOOD ( 36.81 ) 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 Fri, Aug 23, 2024 at 11:01:13PM +0100, Mark Brown wrote: > On Fri, Aug 23, 2024 at 04:59:11PM +0100, Catalin Marinas wrote: > > On Fri, Aug 23, 2024 at 11:25:30AM +0100, Mark Brown wrote: > > > > We could store either the cap token or the interrupted GCSPR_EL0 (the > > > address below the cap token). It felt more joined up to go with the cap > > > token since notionally signal return is consuming the cap token but > > > either way would work, we could just add an offset when looking at the > > > pointer. > > > In a hypothetical sigaltshadowstack() scenario, would the cap go on the > > new signal shadow stack or on the old one? I assume on the new one but > > in sigcontext we'd save the original GCSPR_EL0. In such hypothetical > > case, the original GCSPR_EL0 would not need 8 subtracted. > > I would have put the token on the old stack since that's what we'd be > returning to. After some more spec reading, your approach makes sense as it matches the GCSSS[12] instructions where the outgoing, rather than incoming, shadow stack is capped. So all good I think. However, a bit more below on the restore order (it's ok but a bit confusing). > This raises interesting questions about what happens if > the reason for the signal is that we just overflowed the normal stack > (which are among the issues that have got in the way of working out if > or how we do something with sigaltshadowstack). That's not that different from the classic case where we get an error trying to setup the frame. signal_setup_done() handles it by forcing a SIGSEGV. I'd say we do the same here. > I'm not clear what the > purpose of the token would be on the new stack, the token basically says > "this is somewhere we can sigreturn to", that's not the case for the > alternative stack. Yeah, I thought we have to somehow mark the top of the stack with this token. But looking at the architecture stack switching, it caps the outgoing stack (in our case this would be the interrupted one). So that's settled. On the patch itself, I think there are some small inconsistencies on how it reads the GCSPR_EL0: preserve_gcs_context() does a gcs_preserve_current_state() and subsequently reads the value from the thread structure. A bit later, gcs_signal_entry() goes for the sysreg directly. I don't think that's a problem even if the thread gets preempted but it would be nice to be consistent. Maybe leave the gcs_preserve_current_state() only a context switch thing. Would it work if we don't touch the thread structure at all in the signal code? We wouldn't deliver a signal in the middle of the switch_to() code. So any value we write in thread struct would be overridden at the next switch. If GCS is disabled for a guest, we save the GCSPR_EL0 with the cap size subtracted but there's no cap written. In restore_gcs_context() it doesn't look like we add the cap size back when writing GCSPR_EL0. If GCS is enabled, we do consume the cap and add 8 but otherwise it looks that we keep decreasing GCSPR_EL0. I think we should always subtract the cap size if GCS is enabled. This could could do with some refactoring as I find it hard to follow (not sure exactly how, maybe just comments will do). I'd also keep a single write to GCSPR_EL0 on the return path but I'm ok with two if we need to cope with GCS being disabled but the GCSPR_EL0 still being saved/restored. Another aspect for gcs_restore_signal(), I think it makes more sense for the cap to be consumed _after_ restoring the sigcontext since this has the actual gcspr_el0 where we stored the cap and represents the original stack. If we'll get an alternative shadow stack, current GCSPR_EL0 on sigreturn points to that alternative shadow stack rather than the original one. That's what confused me when reviewing the patch and I thought the cap goes to the top of the signal stack. -- Catalin