Re: [PATCH 01/35] prctl: arch-agnostic prctl for shadow stack

From: "Edgecombe, Rick P" <rick.p.edgecombe@intel.com>
To: "corbet@lwn.net" <corbet@lwn.net>,
	"ardb@kernel.org" <ardb@kernel.org>,
	"maz@kernel.org" <maz@kernel.org>,
	"shuah@kernel.org" <shuah@kernel.org>,
	"Szabolcs.Nagy@arm.com" <Szabolcs.Nagy@arm.com>,
	"keescook@chromium.org" <keescook@chromium.org>,
	"james.morse@arm.com" <james.morse@arm.com>,
	"debug@rivosinc.com" <debug@rivosinc.com>,
	"akpm@linux-foundation.org" <akpm@linux-foundation.org>,
	"catalin.marinas@arm.com" <catalin.marinas@arm.com>,
	"oleg@redhat.com" <oleg@redhat.com>,
	"arnd@arndb.de" <arnd@arndb.de>,
	"ebiederm@xmission.com" <ebiederm@xmission.com>,
	"will@kernel.org" <will@kernel.org>,
	"suzuki.poulose@arm.com" <suzuki.poulose@arm.com>,
	"oliver.upton@linux.dev" <oliver.upton@linux.dev>,
	"broonie@kernel.org" <broonie@kernel.org>
Cc: "hjl.tools@gmail.com" <hjl.tools@gmail.com>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
	"linux-riscv@lists.infradead.org"
	<linux-riscv@lists.infradead.org>,
	"linux-kselftest@vger.kernel.org"
	<linux-kselftest@vger.kernel.org>,
	"linux-fsdevel@vger.kernel.org" <linux-fsdevel@vger.kernel.org>,
	"linux-mm@kvack.org" <linux-mm@kvack.org>,
	"paul.walmsley@sifive.com" <paul.walmsley@sifive.com>,
	"aou@eecs.berkeley.edu" <aou@eecs.berkeley.edu>,
	"palmer@dabbelt.com" <palmer@dabbelt.com>,
	"linux-doc@vger.kernel.org" <linux-doc@vger.kernel.org>,
	"kvmarm@lists.linux.dev" <kvmarm@lists.linux.dev>,
	"linux-arm-kernel@lists.infradead.org" 
	<linux-arm-kernel@lists.infradead.org>,
	"linux-arch@vger.kernel.org" <linux-arch@vger.kernel.org>
Subject: Re: [PATCH 01/35] prctl: arch-agnostic prctl for shadow stack
Date: Tue, 18 Jul 2023 17:45:01 +0000	[thread overview]
Message-ID: <3185fa6c37e54d26d66cd0e87e74fb5492399d78.camel@intel.com> (raw)
In-Reply-To: <20230716-arm64-gcs-v1-1-bf567f93bba6@kernel.org>

On Sun, 2023-07-16 at 22:50 +0100, Mark Brown wrote:
> From: Deepak Gupta <debug@rivosinc.com>
> 
> Three architectures (x86, aarch64, riscv) have announced support for
> shadow stack.  This patch adds arch-agnostic prtcl support to enable
> /disable/get/set status of shadow stack and forward control (landing
> pad)
> flow cfi statuses.

What is this about forward control flow? Seems to be just about shadow
stack.

> 
> New prctls are
>       - PR_GET_SHADOW_STACK_STATUS, PR_SET_SHADOW_STACK_STATUS
> 
> Signed-off-by: Deepak Gupta <debug@rivosinc.com>
> [Rebased onto current kernels, renumbering to track other allocations
>  already upstream, dropping indirect LP, updating to pass arg to set
>   by value, fix missing prototypes for weak functions and update
> title.
>   -- broonie]
> Signed-off-by: Mark Brown <broonie@kernel.org>

This is similar to the arch_prctl() thing x86 does, but I actually see
a fair amount of differences:

1. PR_SET_SHADOW_STACK_STATUS seems like a strange name for the thing
actually doing the whole enabling of the feature which involves
allocating memory, etc. And in the future a growing array of different
things (enabling push, write, etc).

2. x86 only allows one enabling/disabling operation at a time. So you
can't enable shadow stack AND WRSS with one syscall, for example. This
is to make it so it's clear which operation failed. Also, since some
features depend on others (WRSS), there would need to be some ordering
and rollback logic. There was some discussion about a batch enabling
arch_prctl() that could report failures independently, but it was
deemed premature optimization.

3. It only allows you to lock the whole feature, and not individual
subfeatures. For things like WRSS, it came up that there might be an
elf bit, like the shadow stack one, but that works a bit different.
Instead of only enabling shadow stack when ALL DSOs support the
feature, it would want to be enabled if ANY DSOs require it. So
userspace might want to do something like lock shadow stack, but leave
WRSS unlocked in case a dlopen() call came across a WRSS-requiring DSO.

4. To support CRIU, there needed to be a ptrace-only unlock feature.
The arch_prctl() has a special ptrace route to enforce that this unlock
is only coming from ptrace. Is there some way to do this with a regular
prctl()?

5. I see in the next patch there is hinted support for write and push
as well (although I can't find the implementation in the patches, am I
missing it?). X86 has something close enough to write, but not push.
What is the idea for when the features don't exactly match?

I think when Deepak originally brought up this unified prctl-based
interface, it seemed far away before we could tell if it *could* be
unified. Do either of you have any thoughts on whether the above points
could be incorporated?