Received-SPF: pass (google.com: domain of linux-kernel+bounces-75768-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Date: Wed, 21 Feb 2024 17:32:56 -0800
From: Deepak Gupta <debug@rivosinc.com>
To: David Hildenbrand <david@redhat.com>
Cc: rick.p.edgecombe@intel.com, broonie@kernel.org, Szabolcs.Nagy@arm.com,
	kito.cheng@sifive.com, keescook@chromium.org,
	ajones@ventanamicro.com, paul.walmsley@sifive.com,
	palmer@dabbelt.com, conor.dooley@microchip.com, cleger@rivosinc.com,
	atishp@atishpatra.org, alex@ghiti.fr, bjorn@rivosinc.com,
	alexghiti@rivosinc.com, corbet@lwn.net, aou@eecs.berkeley.edu,
	oleg@redhat.com, akpm@linux-foundation.org, arnd@arndb.de,
	ebiederm@xmission.com, shuah@kernel.org, brauner@kernel.org,
	guoren@kernel.org, samitolvanen@google.com, evan@rivosinc.com,
	xiao.w.wang@intel.com, apatel@ventanamicro.com,
	mchitale@ventanamicro.com, waylingii@gmail.com,
	greentime.hu@sifive.com, heiko@sntech.de, jszhang@kernel.org,
	shikemeng@huaweicloud.com, charlie@rivosinc.com,
	panqinglin2020@iscas.ac.cn, willy@infradead.org,
	vincent.chen@sifive.com, andy.chiu@sifive.com, gerg@kernel.org,
	jeeheng.sia@starfivetech.com, mason.huo@starfivetech.com,
	ancientmodern4@gmail.com, mathis.salmen@matsal.de,
	cuiyunhui@bytedance.com, bhe@redhat.com, ruscur@russell.cc,
	bgray@linux.ibm.com, alx@kernel.org, baruch@tkos.co.il,
	zhangqing@loongson.cn, catalin.marinas@arm.com, revest@chromium.org,
	josh@joshtriplett.org, joey.gouly@arm.com, shr@devkernel.io,
	omosnace@redhat.com, ojeda@kernel.org, jhubbard@nvidia.com,
	linux-doc@vger.kernel.org, linux-riscv@lists.infradead.org,
	linux-kernel@vger.kernel.org, linux-mm@kvack.org,
	linux-arch@vger.kernel.org, linux-kselftest@vger.kernel.org
Subject: Re: [RFC PATCH v1 09/28] mm: abstract shadow stack vma behind
 `arch_is_shadow_stack`
Message-ID: <ZdakSM7KcXGgOoU6@debug.ba.rivosinc.com>
References: <20240125062739.1339782-1-debug@rivosinc.com>
 <20240125062739.1339782-10-debug@rivosinc.com>
 <2f34f6aa-99fa-4545-b706-a1d50864f9e9@redhat.com>
 <ZbKVNm5ubV8yQtSE@debug.ba.rivosinc.com>
 <45d5b98c-bad8-471d-a285-47f47c5b50bb@redhat.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii; format=flowed
Content-Disposition: inline
In-Reply-To: <45d5b98c-bad8-471d-a285-47f47c5b50bb@redhat.com>

On Tue, Feb 13, 2024 at 11:34:59AM +0100, David Hildenbrand wrote:
>On 25.01.24 18:07, Deepak Gupta wrote:
>>On Thu, Jan 25, 2024 at 09:18:07AM +0100, David Hildenbrand wrote:
>>>On 25.01.24 07:21, debug@rivosinc.com wrote:
>>>>From: Deepak Gupta <debug@rivosinc.com>
>>>>
>>>>x86 has used VM_SHADOW_STACK (alias to VM_HIGH_ARCH_5) to encode shadow
>>>>stack VMA. VM_SHADOW_STACK is thus not possible on 32bit. Some arches may
>>>>need a way to encode shadow stack on 32bit and 64bit both and they may
>>>>encode this information differently in VMAs.
>>>>
>>>>This patch changes checks of VM_SHADOW_STACK flag in generic code to call
>>>>to a function `arch_is_shadow_stack` which will return true if arch
>>>>supports shadow stack and vma is shadow stack else stub returns false.
>>>>
>>>>There was a suggestion to name it as `vma_is_shadow_stack`. I preferred to
>>>>keep `arch` prefix in there because it's each arch specific.
>>>>
>>>>Signed-off-by: Deepak Gupta <debug@rivosinc.com>
>>>>---
>>>>  include/linux/mm.h | 18 +++++++++++++++++-
>>>>  mm/gup.c           |  5 +++--
>>>>  mm/internal.h      |  2 +-
>>>>  3 files changed, 21 insertions(+), 4 deletions(-)
>>>>
>>>>diff --git a/include/linux/mm.h b/include/linux/mm.h
>>>>index dfe0e8118669..15c70fc677a3 100644
>>>>--- a/include/linux/mm.h
>>>>+++ b/include/linux/mm.h
>>>>@@ -352,6 +352,10 @@ extern unsigned int kobjsize(const void *objp);
>>>>   * for more details on the guard size.
>>>>   */
>>>>  # define VM_SHADOW_STACK	VM_HIGH_ARCH_5
>>>>+static inline bool arch_is_shadow_stack(vm_flags_t vm_flags)
>>>>+{
>>>>+	return (vm_flags & VM_SHADOW_STACK);
>>>>+}
>>>>  #endif
>>>>  #ifdef CONFIG_RISCV_USER_CFI
>>>>@@ -362,10 +366,22 @@ extern unsigned int kobjsize(const void *objp);
>>>>   * with VM_SHARED.
>>>>   */
>>>>  #define VM_SHADOW_STACK	VM_WRITE
>>>>+
>>>>+static inline bool arch_is_shadow_stack(vm_flags_t vm_flags)
>>>>+{
>>>>+	return ((vm_flags & (VM_WRITE | VM_READ | VM_EXEC)) == VM_WRITE);
>>>>+}
>>>>+
>>>
>>>Please no such hacks just to work around the 32bit vmflags limitation.
>>
>>As I said in another response. Noted.
>>And if there're no takers for 32bit on riscv (which highly likely is the case)
>>This will go away in next version of patchsets.
>
>Sorry for the (unusually for me) late reply. Simplifying to riscv64 
>sounds great.
>
>Alternatively, maybe VM_SHADOW_STACK is not even required at all on 
>riscv if we can teach all code to only stare at arch_is_shadow_stack() 
>instead.

Sorry for late reply.

I think for risc-v this can be done, if done in following way

static inline bool arch_is_shadow_stack(struct vm_flags_t vm_flags)
{
		return (vm_get_page_prot(vm_flags) == PAGE_SHADOWSTACK);
}

But doing above will require following

- Inventing a new PROT_XXX type (let's call it PROT_SHADOWSTACK) that
   is only exposed to kernel. PROT_SHADOWSTACK protection flag would allow
   `do_mmap` to do right thing and setup appropriate protection perms.
   We wouldn't want to expose this protection type to user mode (because
   `map_shadow_stack` already exists for that).
   Current patch series uses PROT_SHADOWSTACK because VM_SHADOW_STACK was
   aliased to VM_WRITE.

- As you said teach all the generic code as well to use arch_is_shadow_stack
   which might become complicated (can't say for sure)


>
>... but, just using the same VM_SHADOW_STACK will it all much cleaner. 
>Eventually, we can just stop playing arch-specific games with 
>arch_is_shadow_stack and VM_SHADOW_STACK.

Yeah for now, looks like easier thing to do.

>
>-- 
>Cheers,
>
>David / dhildenb
>