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

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

On Tue, Sep 02, 2025 at 11:21:48AM +0100, Mark Brown wrote:
> diff --git a/kernel/fork.c b/kernel/fork.c
> index af673856499d..d484ebeded33 100644
> --- a/kernel/fork.c
> +++ b/kernel/fork.c
> @@ -1907,6 +1907,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;

In theory, I don't think we need the get_task_mm() -> mmget() since
copy_mm() early on already did this and the task can't disappear from
underneath while we are creating it.

> +
> +	mmap_read_lock(mm);
> +
> +	addr = untagged_addr_remote(mm, args->shadow_stack_token);
> +	page = get_user_page_vma_remote(mm, addr, FOLL_FORCE | FOLL_WRITE,
> +					&vma);

However, I wonder whether it makes sense to use the remote mm access
here at all. Does this code ever run without CLONE_VM? If not, this is
all done within the current mm context.

I can see the x86 shstk_alloc_thread_stack() returns early if !CLONE_VM.
Similarly on arm64. I think the behaviour is preserved with this series
but I'm not entirely sure from the contextual diff (I need to apply the
patches locally).

Otherwise the patch looks fine (well, even the above wouldn't fail, I
just find it strange that we pretend it's a remote mm but on the default
allocation path like alloc_gcs() we go for current->mm).


BTW, if you repost, it might be worth cross-posting to linux-arm-kernel
for wider exposure as not everyone reads LKML (and you can drop
Szabolcs, his arm address is no longer valid).

-- 
Catalin