Re: [PATCH v8 24/29] riscv: enable kernel access to shadow stack memory via FWFT sbi call

From: Deepak Gupta <debug@rivosinc.com>
To: Nick Hu <nick.hu@sifive.com>
Cc: Thomas Gleixner <tglx@linutronix.de>,
	Ingo Molnar <mingo@redhat.com>, Borislav Petkov <bp@alien8.de>,
	Dave Hansen <dave.hansen@linux.intel.com>,
	x86@kernel.org, "H. Peter Anvin" <hpa@zytor.com>,
	Andrew Morton <akpm@linux-foundation.org>,
	"Liam R. Howlett" <Liam.Howlett@oracle.com>,
	Vlastimil Babka <vbabka@suse.cz>,
	Lorenzo Stoakes <lorenzo.stoakes@oracle.com>,
	Paul Walmsley <paul.walmsley@sifive.com>,
	Palmer Dabbelt <palmer@dabbelt.com>,
	Albert Ou <aou@eecs.berkeley.edu>,
	Conor Dooley <conor@kernel.org>, Rob Herring <robh@kernel.org>,
	Krzysztof Kozlowski <krzk+dt@kernel.org>,
	Arnd Bergmann <arnd@arndb.de>,
	Christian Brauner <brauner@kernel.org>,
	Peter Zijlstra <peterz@infradead.org>,
	Oleg Nesterov <oleg@redhat.com>,
	Eric Biederman <ebiederm@xmission.com>,
	Kees Cook <kees@kernel.org>, Jonathan Corbet <corbet@lwn.net>,
	Shuah Khan <shuah@kernel.org>,
	linux-kernel@vger.kernel.org, linux-fsdevel@vger.kernel.org,
	linux-mm@kvack.org, linux-riscv@lists.infradead.org,
	devicetree@vger.kernel.org, linux-arch@vger.kernel.org,
	linux-doc@vger.kernel.org, linux-kselftest@vger.kernel.org,
	alistair.francis@wdc.com, richard.henderson@linaro.org,
	jim.shu@sifive.com, andybnac@gmail.com, kito.cheng@sifive.com,
	charlie@rivosinc.com, atishp@rivosinc.com, evan@rivosinc.com,
	cleger@rivosinc.com, alexghiti@rivosinc.com,
	samitolvanen@google.com, broonie@kernel.org,
	rick.p.edgecombe@intel.com
Subject: Re: [PATCH v8 24/29] riscv: enable kernel access to shadow stack memory via FWFT sbi call
Date: Thu, 14 Nov 2024 07:50:45 -0800	[thread overview]
Message-ID: <ZzYcVW/4M0jab1T4@debug.ba.rivosinc.com> (raw)
In-Reply-To: <CAKddAkBCByf570PXfz798FtBbeGQWe2LJpdzxkE+jv3Zd3ZV1w@mail.gmail.com>

Hi Nick,

Thanks for reviewing and helping.

On Thu, Nov 14, 2024 at 02:17:30PM +0800, Nick Hu wrote:
>Hi Deepak
>
>On Thu, Nov 14, 2024 at 9:25 AM Deepak Gupta <debug@rivosinc.com> wrote:
>>
>> On Thu, Nov 14, 2024 at 09:20:14AM +0800, Nick Hu wrote:
>> >Hi Deepak
>> >
>> >On Thu, Nov 14, 2024 at 9:06 AM Deepak Gupta <debug@rivosinc.com> wrote:
>> >> >> diff --git a/arch/riscv/kernel/head.S b/arch/riscv/kernel/head.S
>> >> >> index 356d5397b2a2..6244408ca917 100644
>> >> >> --- a/arch/riscv/kernel/head.S
>> >> >> +++ b/arch/riscv/kernel/head.S
>> >> >> @@ -164,6 +164,12 @@ secondary_start_sbi:
>> >> >>         call relocate_enable_mmu
>> >> >>  #endif
>> >> >>         call .Lsetup_trap_vector
>> >> >> +       li a7, SBI_EXT_FWFT
>> >> >> +       li a6, SBI_EXT_FWFT_SET
>> >> >> +       li a0, SBI_FWFT_SHADOW_STACK
>> >> >> +       li a1, 1 /* enable supervisor to access shadow stack access */
>> >> >> +       li a2, SBI_FWFT_SET_FLAG_LOCK
>> >> >> +       ecall
>> >> >>         scs_load_current
>> >> >>         call smp_callin
>> >> >>  #endif /* CONFIG_SMP */
>> >> >> @@ -320,6 +326,12 @@ SYM_CODE_START(_start_kernel)
>> >> >>         la tp, init_task
>> >> >>         la sp, init_thread_union + THREAD_SIZE
>> >> >>         addi sp, sp, -PT_SIZE_ON_STACK
>> >> >> +       li a7, SBI_EXT_FWFT
>> >> >> +       li a6, SBI_EXT_FWFT_SET
>> >> >> +       li a0, SBI_FWFT_SHADOW_STACK
>> >> >> +       li a1, 1 /* enable supervisor to access shadow stack access */
>> >> >> +       li a2, SBI_FWFT_SET_FLAG_LOCK
>> >> >> +       ecall
>> >> >>         scs_load_current
>> >> >>
>> >> >>  #ifdef CONFIG_KASAN
>> >> >>
>> >> >> --
>> >> >> 2.45.0
>> >> >>
>> >> >Should we clear the SBI_FWFT_SET_FLAG_LOCK before the cpu hotplug
>> >> >otherwise the menvcfg.sse won't be set by the fwft set sbi call when
>> >> >the hotplug cpu back to kernel?
>> >>
>> >> Hmm...
>> >>
>> >> An incoming hotplug CPU has no features setup on it.
>> >> I see that `sbi_cpu_start` will supply `secondary_start_sbi` as start
>> >> up code for incoming CPU. `secondary_start_sbi` is in head.S which converges
>> >> in `.Lsecondary_start_common`. And thus hotplugged CPU should be
>> >> issuing shadow stack set FWFT sbi as well.
>> >>
>> >> Am I missing something ?
>> >>
>> >This is the correct flow. However the opensbi will deny it due to the
>> >SBI_FWFT_SET_FLAG_LOCK already being set.
>> >So the menvcfg.sse will not set by this flow.
>> >
>> >if (conf->flags & SBI_FWFT_SET_FLAG_LOCK)
>> >                return SBI_EDENIED;
>> >
>>
>> hmm... Why?
>>
>> `conf` is pointing to per-hart state in firmware.
>>
>> On this incoming cpu, opensbi (or equivalent) firmware must have
>> ensured that this per-hart state doesn't have lock set.
>>
>> Am I missing something?
>>
>Current OpenSBI doesn't clear the lock in the warm init of the hotplug path.
>It seems like we need a patch to address it.

Got it thanks.
Since you already know what's the problem, can you send a patch to opensbi.
If you want rather have me do it, let me know. Thanks.

>
>Regards,
>Nick
>> >Regards,
>> >Nick
>> >> >
>> >> >Regards,
>> >> >Nick
>> >> >>
>> >> >> _______________________________________________
>> >> >> linux-riscv mailing list
>> >> >> linux-riscv@lists.infradead.org
>> >> >> http://lists.infradead.org/mailman/listinfo/linux-riscv