Received-SPF: pass (google.com: domain of linux-kernel+bounces-141426-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Precedence: bulk
MIME-Version: 1.0
References: <20240409061043.3269676-1-debug@rivosinc.com> <20240409061043.3269676-9-debug@rivosinc.com>
 <CABCJKud622jAHZGiYed-2veynA6rd_=1mnTwnTT1WNQ6203q6A@mail.gmail.com> <ZhgeRA3XplDw7kmm@debug.ba.rivosinc.com>
In-Reply-To: <ZhgeRA3XplDw7kmm@debug.ba.rivosinc.com>
From: Sami Tolvanen <samitolvanen@google.com>
Date: Thu, 11 Apr 2024 17:47:10 +0000
Message-ID: <CABCJKufbEN-iO+nAQFw46Qe61HmfTfMALfz9o=EvmiAFY8+19Q@mail.gmail.com>
Subject: Re: [RFC PATCH 08/12] riscv: dynamic (zicfiss) shadow call stack support
To: Deepak Gupta <debug@rivosinc.com>
Cc: linux-riscv@lists.infradead.org, linux-kernel@vger.kernel.org, 
	llvm@lists.linux.dev, paul.walmsley@sifive.com, palmer@dabbelt.com, 
	aou@eecs.berkeley.edu, nathan@kernel.org, ndesaulniers@google.com, 
	morbo@google.com, justinstitt@google.com, andy.chiu@sifive.com, 
	hankuan.chen@sifive.com, guoren@kernel.org, greentime.hu@sifive.com, 
	cleger@rivosinc.com, apatel@ventanamicro.com, ajones@ventanamicro.com, 
	conor.dooley@microchip.com, mchitale@ventanamicro.com, 
	dbarboza@ventanamicro.com, waylingii@gmail.com, sameo@rivosinc.com, 
	alexghiti@rivosinc.com, akpm@linux-foundation.org, shikemeng@huaweicloud.com, 
	rppt@kernel.org, charlie@rivosinc.com, xiao.w.wang@intel.com, 
	willy@infradead.org, jszhang@kernel.org, leobras@redhat.com, 
	songshuaishuai@tinylab.org, haxel@fzi.de, samuel.holland@sifive.com, 
	namcaov@gmail.com, bjorn@rivosinc.com, cuiyunhui@bytedance.com, 
	wangkefeng.wang@huawei.com, falcon@tinylab.org, viro@zeniv.linux.org.uk, 
	bhe@redhat.com, chenjiahao16@huawei.com, hca@linux.ibm.com, arnd@arndb.de, 
	kent.overstreet@linux.dev, boqun.feng@gmail.com, oleg@redhat.com, 
	paulmck@kernel.org, broonie@kernel.org, rick.p.edgecombe@intel.com, 
	Ard Biesheuvel <ardb@kernel.org>, Will Deacon <will@kernel.org>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

On Thu, Apr 11, 2024 at 5:30=E2=80=AFPM Deepak Gupta <debug@rivosinc.com> w=
rote:
>
> On Thu, Apr 11, 2024 at 05:05:38PM +0000, Sami Tolvanen wrote:
> >Hi Deepak,
> >
> >Thanks for the patches!
> >
> >On Tue, Apr 9, 2024 at 6:12=E2=80=AFAM Deepak Gupta <debug@rivosinc.com>=
 wrote:
> >>
> >> Adding support for dynamic shadow call stack on riscv. zicfiss ISA ext=
n.
> >> enables protection for shadow stack against stray writes. This patch
> >> enables scs_* macros to use zicfiss shadow stack pointer (CSR_SSP) ins=
tead
> >> of relying on `gp`.
> >
> >CONFIG_DYNAMIC_SCS implies that runtime patching is used to select
> >between software SCS and an alternative hardware implementation (in
> >arm64's case, PAC instead of hardware shadow stacks). I understand
> >this series is still an RFC, but I didn't see runtime patching
> >support. Are you planning on implementing this later?
>
> Since I didn't see any example on selecting PAC when `CONFIG_DYNAMIC_SCS`
> is selected. So I had that confusion but wasn't sure. I thought of doing =
it
> but I don't know how to binary rewrite all the functions. It might be too=
 much.
> So I went ahead with using `CONFIG_DYNAMIC_SCS` in this RFC series.
>
> Question:
> If arm64 were to use PAC with CONFIG_DYNAMIC_SCS, how would it fixup the =
code
> sequences already setup by compiler for shadow stack push and pop in runt=
ime?
> You expect this to be some offline process using some object editing tool=
 or
> a runtime decision?

We use unwind tables for locating instructions to patch, look for
UNWIND_PATCH_PAC_INTO_SCS. The actual patching code is in
arch/arm64/kernel/pi/patch-scs.c. I suspect this is going to be a bit
trickier when patching between two shadow stack options though.

> >If there's no plan to actually patch between Zicfiss and SCS at
> >runtime, CONFIG_DYNAMIC_SCS doesn't seem like the appropriate choice
> >and we might need a separate config option that still allows you to
> >reuse most of the software SCS code.
>
> I wanted to avoid "#ifdef RISCV_SPECIFIC_HW_SHSTK" in arch agnostic scs c=
ode.
> And that's why went with CONFIG_DYNAMIC_SCS which sets dynamic static key=
 once.
> And then I use `is_dynamic` everywhere else in arch agnostic scs code.

We could define arch_ functions for any architecture-specific code
(with a weak default implementation), and maybe add a config option
for specifying which way the shadow stack grows?

Sami