linux-kernel - Re: [PATCH v6 16/33] riscv/shstk: If needed allocate a new shadow stack on clone

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: <ZwW9m6pqcTFBovuG@debug.ba.rivosinc.com>
Date: Tue, 8 Oct 2024 16:17:47 -0700
From: Deepak Gupta <debug@...osinc.com>
To: "Edgecombe, Rick P" <rick.p.edgecombe@...el.com>
Cc: "corbet@....net" <corbet@....net>, "robh@...nel.org" <robh@...nel.org>,
	"lorenzo.stoakes@...cle.com" <lorenzo.stoakes@...cle.com>,
	"dave.hansen@...ux.intel.com" <dave.hansen@...ux.intel.com>,
	"vbabka@...e.cz" <vbabka@...e.cz>,
	"brauner@...nel.org" <brauner@...nel.org>,
	"akpm@...ux-foundation.org" <akpm@...ux-foundation.org>,
	"palmer@...belt.com" <palmer@...belt.com>,
	"mingo@...hat.com" <mingo@...hat.com>,
	"paul.walmsley@...ive.com" <paul.walmsley@...ive.com>,
	"Liam.Howlett@...cle.com" <Liam.Howlett@...cle.com>,
	"tglx@...utronix.de" <tglx@...utronix.de>,
	"aou@...s.berkeley.edu" <aou@...s.berkeley.edu>,
	"oleg@...hat.com" <oleg@...hat.com>,
	"krzk+dt@...nel.org" <krzk+dt@...nel.org>,
	"conor@...nel.org" <conor@...nel.org>,
	"ebiederm@...ssion.com" <ebiederm@...ssion.com>,
	"hpa@...or.com" <hpa@...or.com>,
	"peterz@...radead.org" <peterz@...radead.org>,
	"arnd@...db.de" <arnd@...db.de>, "bp@...en8.de" <bp@...en8.de>,
	"kees@...nel.org" <kees@...nel.org>,
	"x86@...nel.org" <x86@...nel.org>,
	"shuah@...nel.org" <shuah@...nel.org>,
	"broonie@...nel.org" <broonie@...nel.org>,
	"jim.shu@...ive.com" <jim.shu@...ive.com>,
	"alistair.francis@....com" <alistair.francis@....com>,
	"cleger@...osinc.com" <cleger@...osinc.com>,
	"kito.cheng@...ive.com" <kito.cheng@...ive.com>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"samitolvanen@...gle.com" <samitolvanen@...gle.com>,
	"evan@...osinc.com" <evan@...osinc.com>,
	"linux-mm@...ck.org" <linux-mm@...ck.org>,
	"linux-arch@...r.kernel.org" <linux-arch@...r.kernel.org>,
	"atishp@...osinc.com" <atishp@...osinc.com>,
	"andybnac@...il.com" <andybnac@...il.com>,
	"devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
	"charlie@...osinc.com" <charlie@...osinc.com>,
	"linux-doc@...r.kernel.org" <linux-doc@...r.kernel.org>,
	"linux-fsdevel@...r.kernel.org" <linux-fsdevel@...r.kernel.org>,
	"linux-kselftest@...r.kernel.org" <linux-kselftest@...r.kernel.org>,
	"richard.henderson@...aro.org" <richard.henderson@...aro.org>,
	"linux-riscv@...ts.infradead.org" <linux-riscv@...ts.infradead.org>,
	"alexghiti@...osinc.com" <alexghiti@...osinc.com>
Subject: Re: [PATCH v6 16/33] riscv/shstk: If needed allocate a new shadow
 stack on clone

On Tue, Oct 08, 2024 at 10:55:29PM +0000, Edgecombe, Rick P wrote:
>On Tue, 2024-10-08 at 15:36 -0700, Deepak Gupta wrote:
>> +unsigned long shstk_alloc_thread_stack(struct task_struct *tsk,
>> +					   const struct kernel_clone_args *args)
>> +{
>> +	unsigned long addr, size;
>> +
>> +	/* If shadow stack is not supported, return 0 */
>> +	if (!cpu_supports_shadow_stack())
>> +		return 0;
>> +
>> +	/*
>> +	 * If shadow stack is not enabled on the new thread, skip any
>> +	 * switch to a new shadow stack.
>> +	 */
>> +	if (!is_shstk_enabled(tsk))
>> +		return 0;
>> +
>> +	/*
>> +	 * For CLONE_VFORK the child will share the parents shadow stack.
>> +	 * Set base = 0 and size = 0, this is special means to track this state
>> +	 * so the freeing logic run for child knows to leave it alone.
>> +	 */
>> +	if (args->flags & CLONE_VFORK) {
>> +		set_shstk_base(tsk, 0, 0);
>> +		return 0;
>> +	}
>> +
>> +	/*
>> +	 * For !CLONE_VM the child will use a copy of the parents shadow
>> +	 * stack.
>> +	 */
>> +	if (!(args->flags & CLONE_VM))
>> +		return 0;
>> +
>> +	/*
>> +	 * reaching here means, CLONE_VM was specified and thus a separate shadow
>> +	 * stack is needed for new cloned thread. Note: below allocation is happening
>> +	 * using current mm.
>> +	 */
>> +	size = calc_shstk_size(args->stack_size);
>> +	addr = allocate_shadow_stack(0, size, 0, false);
>> +	if (IS_ERR_VALUE(addr))
>> +		return addr;
>> +
>> +	set_shstk_base(tsk, addr, size);
>> +
>> +	return addr + size;
>> +}
>
>A lot of this patch and the previous one is similar to x86's and arm's. It great
>that we can have consistency around this behavior.
>
>There might be enough consistency to refactor some of the arch code into a
>kernel/shstk.c.
>
>Should we try?

Yeah you're right. Honestly, I've been shameless in adapting most of the flows
from x86 `shstk.c` for risc-v. So thank you for that.

Now that we've `ARCH_HAS_USER_SHADOW_STACK` part of multiple patch series (riscv
shadowstack, clone3 and I think arm64 gcs series as well). It's probably the
appropriate time to find common grounds.

This is what I suggest

- move most of the common/arch agnostic shadow stack stuff in kernel/shstk.c
   This gets part of compile if `ARCH_HAS_USER_SHADOW_STACK` is enabled/selected.

- allow arch specific branch out guard checks for "if cpu supports", "is shadow stack
   enabled on the task_struct" (I expect each arch layout of task_struct will be
   different, no point finding common ground there), etc.

I think it's worth a try. 
If you already don't have patches, I'll spend some time to see what it takes to
converge in my next version. If I end up into some roadblock, will use this thread
for further discussion.