linux-kernel - Re: [PATCH v12 05/28] riscv: usercfi state for task and save/restore of CSR

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-Id: <D9FOQ0C9HMIR.17FERF6F7C8LR@ventanamicro.com>
Date: Fri, 25 Apr 2025 13:32:00 +0200
From: Radim Krčmář <rkrcmar@...tanamicro.com>
To: "Deepak Gupta" <debug@...osinc.com>
Cc: "Thomas Gleixner" <tglx@...utronix.de>, "Ingo Molnar"
 <mingo@...hat.com>, "Borislav Petkov" <bp@...en8.de>, "Dave Hansen"
 <dave.hansen@...ux.intel.com>, <x86@...nel.org>, "H. Peter Anvin"
 <hpa@...or.com>, "Andrew Morton" <akpm@...ux-foundation.org>, "Liam R.
 Howlett" <Liam.Howlett@...cle.com>, "Vlastimil Babka" <vbabka@...e.cz>,
 "Lorenzo Stoakes" <lorenzo.stoakes@...cle.com>, "Paul Walmsley"
 <paul.walmsley@...ive.com>, "Palmer Dabbelt" <palmer@...belt.com>, "Albert
 Ou" <aou@...s.berkeley.edu>, "Conor Dooley" <conor@...nel.org>, "Rob
 Herring" <robh@...nel.org>, "Krzysztof Kozlowski" <krzk+dt@...nel.org>,
 "Arnd Bergmann" <arnd@...db.de>, "Christian Brauner" <brauner@...nel.org>,
 "Peter Zijlstra" <peterz@...radead.org>, "Oleg Nesterov" <oleg@...hat.com>,
 "Eric Biederman" <ebiederm@...ssion.com>, "Kees Cook" <kees@...nel.org>,
 "Jonathan Corbet" <corbet@....net>, "Shuah Khan" <shuah@...nel.org>, "Jann
 Horn" <jannh@...gle.com>, "Conor Dooley" <conor+dt@...nel.org>,
 <linux-kernel@...r.kernel.org>, <linux-fsdevel@...r.kernel.org>,
 <linux-mm@...ck.org>, <linux-riscv@...ts.infradead.org>,
 <devicetree@...r.kernel.org>, <linux-arch@...r.kernel.org>,
 <linux-doc@...r.kernel.org>, <linux-kselftest@...r.kernel.org>,
 <alistair.francis@....com>, <richard.henderson@...aro.org>,
 <jim.shu@...ive.com>, <andybnac@...il.com>, <kito.cheng@...ive.com>,
 <charlie@...osinc.com>, <atishp@...osinc.com>, <evan@...osinc.com>,
 <cleger@...osinc.com>, <alexghiti@...osinc.com>, <samitolvanen@...gle.com>,
 <broonie@...nel.org>, <rick.p.edgecombe@...el.com>, "Zong Li"
 <zong.li@...ive.com>, "linux-riscv"
 <linux-riscv-bounces@...ts.infradead.org>
Subject: Re: [PATCH v12 05/28] riscv: usercfi state for task and
 save/restore of CSR_SSP on trap entry/exit

2025-04-24T11:03:59-07:00, Deepak Gupta <debug@...osinc.com>:
> On Thu, Apr 24, 2025 at 02:16:32PM +0200, Radim Krčmář wrote:
>>2025-04-23T17:23:56-07:00, Deepak Gupta <debug@...osinc.com>:
>>> On Thu, Apr 10, 2025 at 01:04:39PM +0200, Radim Krčmář wrote:
>>>>2025-03-14T14:39:24-07:00, Deepak Gupta <debug@...osinc.com>:
>>>>> diff --git a/arch/riscv/kernel/entry.S b/arch/riscv/kernel/entry.S
>>>>> @@ -147,6 +147,20 @@ SYM_CODE_START(handle_exception)
>>>>>
>>>>>  	REG_L s0, TASK_TI_USER_SP(tp)
>>>>>  	csrrc s1, CSR_STATUS, t0
>>>>> +	/*
>>>>> +	 * If previous mode was U, capture shadow stack pointer and save it away
>>>>> +	 * Zero CSR_SSP at the same time for sanitization.
>>>>> +	 */
>>>>> +	ALTERNATIVE("nop; nop; nop; nop",
>>>>> +				__stringify(			\
>>>>> +				andi s2, s1, SR_SPP;	\
>>>>> +				bnez s2, skip_ssp_save;	\
>>>>> +				csrrw s2, CSR_SSP, x0;	\
>>>>> +				REG_S s2, TASK_TI_USER_SSP(tp); \
>>>>> +				skip_ssp_save:),
>>>>> +				0,
>>>>> +				RISCV_ISA_EXT_ZICFISS,
>>>>> +				CONFIG_RISCV_USER_CFI)
>>>>
>>>>(I'd prefer this closer to the user_sp and kernel_sp swap, it's breaking
>>>> the flow here.  We also already know if we've returned from userspace
>>>> or not even without SR_SPP, but reusing the information might tangle
>>>> the logic.)
>>>
>>> If CSR_SCRATCH was 0, then we would be coming from kernel else flow goes
>>> to `.Lsave_context`. If we were coming from kernel mode, then eventually
>>> flow merges to `.Lsave_context`.
>>>
>>> So we will be saving CSR_SSP on all kernel -- > kernel trap handling. That
>>> would be unnecessary. IIRC, this was one of the first review comments in
>>> early RFC series of these patch series (to not touch CSR_SSP un-necessarily)
>>>
>>> We can avoid that by ensuring when we branch by determining if we are coming
>>> from user to something like `.Lsave_ssp` which eventually merges into
>>> ".Lsave_context". And if we were coming from kernel then we would branch to
>>> `.Lsave_context` and thus skipping ssp save logic. But # of branches it
>>> introduces in early exception handling is equivalent to what current patches
>>> do. So I don't see any value in doing that.
>>>
>>> Let me know if I am missing something.
>>
>>Right, it's hard to avoid the extra branches.
>>
>>I think we could modify the entry point (STVEC), so we start at
>>different paths based on kernel/userspace trap and only jump once to the
>>common code, like:
>>
>>  SYM_CODE_START(handle_exception_kernel)
>>    /* kernel setup magic */
>>    j handle_exception_common
>>  SYM_CODE_START(handle_exception_user)
>>    /* userspace setup magic */
>>  handle_exception_common:
>
> Hmm... This can be done. But then it would require to constantly modify `stvec`
> When you're going back to user mode, you would have to write `stvec` with addr
> of `handle_exception_user`.

We'd just be writing STVEC instead of SSCRATCH, probably at the very
same places.
It's possible that some micro-architectures will be disturbed more by
writing STVEC than SSCRATCH, though, so it's not an easy change to make.

>                             But then you can easily get a NMI. It can become
> ugly. Needs much more thought and on first glance feels error prone.

Yeah, the M-mode Linux adds a lot of fun.  I don't see support for the
Smrnmi extension, so unlucky NMIs should be fatal even now.