Re: [PATCH RFT v17 4/8] fork: Add shadow stack support to clone3()

[Catalin Marinas <catalin.marinas@arm.com>]

On Mon, Jun 09, 2025 at 01:54:05PM +0100, Mark Brown wrote:
> +int arch_shstk_validate_clone(struct task_struct *tsk,
> +			      struct vm_area_struct *vma,
> +			      struct page *page,
> +			      struct kernel_clone_args *args)
> +{
> +	unsigned long gcspr_el0;
> +	int ret = 0;
> +
> +	/* Ensure that a token written as a result of a pivot is visible */
> +	gcsb_dsync();
> +	gcspr_el0 = args->shadow_stack_token;
> +	if (!gcs_consume_token(vma, page, gcspr_el0))
> +		return -EINVAL;
> +
> +	tsk->thread.gcspr_el0 = gcspr_el0 + sizeof(u64);
> +
> +	/* Ensure that our token consumption visible */
> +	gcsb_dsync();
> +
> +	return ret;
> +}

What are the scenarios where we need the barriers? We have one via
map_shadow_stack() that would cover the first one. IIUC, GCSSS2 also
generates a GCSB event (or maybe I got it all wrong; I need to read the
spec).

> diff --git a/kernel/fork.c b/kernel/fork.c
> index 1ee8eb11f38b..89c19996235d 100644
> --- a/kernel/fork.c
> +++ b/kernel/fork.c
> @@ -1902,6 +1902,51 @@ static bool need_futex_hash_allocate_default(u64 clone_flags)
>  	return true;
>  }
>  
> +static int shstk_validate_clone(struct task_struct *p,
> +				struct kernel_clone_args *args)
> +{
> +	struct mm_struct *mm;
> +	struct vm_area_struct *vma;
> +	struct page *page;
> +	unsigned long addr;
> +	int ret;
> +
> +	if (!IS_ENABLED(CONFIG_ARCH_HAS_USER_SHADOW_STACK))
> +		return 0;
> +
> +	if (!args->shadow_stack_token)
> +		return 0;
> +
> +	mm = get_task_mm(p);
> +	if (!mm)
> +		return -EFAULT;
> +
> +	mmap_read_lock(mm);
> +
> +	addr = untagged_addr_remote(mm, args->shadow_stack_token);

I think down the line, get_user_page_vma_remote() already does an
untagged_addr_remote(). But it does it after the vma look-up, so we
still need the untagging early.

That said, would we ever allowed a tagged pointer for the shadow stack?

> @@ -2840,6 +2891,27 @@ static inline bool clone3_stack_valid(struct kernel_clone_args *kargs)
>  	return true;
>  }
>  
> +/**
> + * clone3_shadow_stack_valid - check and prepare shadow stack
> + * @kargs: kernel clone args
> + *
> + * Verify that shadow stacks are only enabled if supported.
> + */
> +static inline bool clone3_shadow_stack_valid(struct kernel_clone_args *kargs)
> +{
> +	if (!kargs->shadow_stack_token)
> +		return true;
> +
> +	if (!IS_ALIGNED(kargs->shadow_stack_token, sizeof(void *)))
> +		return false;
> +
> +	/*
> +	 * The architecture must check support on the specific
> +	 * machine.
> +	 */
> +	return IS_ENABLED(CONFIG_ARCH_HAS_USER_SHADOW_STACK);

I don't understand the comment here. It implies some kind of fallback
for further arch checks but it's just a return.

BTW, clone3_stack_valid() has an access_ok() check as well. Shall we add
it here? That's where the size would have come in handy but IIUC the
decision was to drop it (fine by me, just validate that the token is
accessible).

-- 
Catalin