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] [day] [month] [year] [list]

Message-Id: <D92WQWAUQYY4.2ED8JAFBDHGRN@ventanamicro.com>
Date: Thu, 10 Apr 2025 13:04:39 +0200
From: Radim Krčmář <rkrcmar@...tanamicro.com>
To: "Deepak Gupta" <debug@...osinc.com>, "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>
Cc: <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-03-14T14:39:24-07:00, Deepak Gupta <debug@...osinc.com>:
> diff --git a/arch/riscv/include/asm/thread_info.h b/arch/riscv/include/asm/thread_info.h
> @@ -62,6 +62,9 @@ struct thread_info {
>  	long			user_sp;	/* User stack pointer */
>  	int			cpu;
>  	unsigned long		syscall_work;	/* SYSCALL_WORK_ flags */
> +#ifdef CONFIG_RISCV_USER_CFI
> +	struct cfi_status	user_cfi_state;
> +#endif

I don't think it makes sense to put all the data in thread_info.
kernel_ssp and user_ssp is more than enough and the rest can comfortably
live elsewhere in task_struct.

thread_info is supposed to be as small as possible -- just spanning
multiple cache-lines could be noticeable.

> 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.)

>  	csrr s2, CSR_EPC
>  	csrr s3, CSR_TVAL
>  	csrr s4, CSR_CAUSE
> @@ -236,6 +250,18 @@ SYM_CODE_START_NOALIGN(ret_from_exception)
>  	csrw CSR_SCRATCH, tp
> +
> +	/*
> +	 * Going back to U mode, restore shadow stack pointer
> +	 */

Are we?  I think we can be just as well returning back to kernel-space.
Similar to how we can enter the exception handler from kernel-space.

> +	ALTERNATIVE("nop; nop",
> +				__stringify(					\
> +				REG_L s3, TASK_TI_USER_SSP(tp); \
> +				csrw CSR_SSP, s3),
> +				0,
> +				RISCV_ISA_EXT_ZICFISS,
> +				CONFIG_RISCV_USER_CFI)
> +

Thanks.