[Devel] [PATCH RFC PATCH hci-8.1 2/4] exec: protect preserved memory against surviving shared-mm tasks

[no-reply at sashiko.virtuozzo.com]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [Medium] exec: reserve_preserved_mem() fails execve() with -EBUSY for vfork/CLONE_VM parents
- [Medium] fork: CLONE_VM clones, including sibling pthread_create(), fail with -EBUSY during reservation
- [Medium] exec: stale VM_EXEC_KEEP snapshot in reserve_preserved_mem() drops later-marked VMAs
- [Medium] exec: preserved-mm reservation misses non-task mm users like kthread_use_mm()
--

commit 0203d4ae0ee568e8d416b284444f0842edfb59ae
Author: Andrey Drobyshev <andrey.drobyshev at virtuozzo.com>

exec: protect preserved memory against surviving shared-mm tasks

This patch reserves the mm holding VM_EXEC_KEEP VMAs before exec becomes
irreversible. It rejects tasks outside the execing thread group that share
the mm, and tracks in-flight CLONE_VM clones through a counter in
mm_struct. New shared-mm clones are refused while the reservation is held.

Link: https://virtuozzo.atlassian.net/browse/VSTOR-103860

> diff --git a/fs/exec.c b/fs/exec.c
> --- a/fs/exec.c
> +++ b/fs/exec.c

[ ... ]

> +static int reserve_preserved_mem(struct linux_binprm *bprm)
> +{

[ ... ]

> +	mmap_read_unlock(mm);
> +	if (!preserve)
> +		return 0;

[Severity: Medium]
Can a VMA marked after this scan be silently dropped?

Sibling threads of the execing group are exempt from admission and keep
running until de_thread().  If one of them calls madvise(MADV_DOEXEC) after
this scan returned 0 with preserving_mem unset, exec_mmap() no longer
re-evaluates the VMAs, so vma_dup_some() is skipped and the newly marked
VMA is not preserved.

Before this patch, vma_dup_some() ran after de_thread() under
mmap_read_lock, so the set of marked VMAs was stable at transfer time.
Should the decision be re-checked after de_thread()?

> +	/* A clone is either counted here or already visible in the task list. */
> +	spin_lock(&mm->page_table_lock);
> +	if (mm->exec_keep_active || mm->exec_keep_clones)
> +		ret = -EBUSY;

[Severity: Medium]
Can an in-flight pthread_create() from a sibling thread make the whole
execve() fail here?

If another thread of the execing group is inside copy_mm() between
exec_keep_clone_begin() and exec_keep_clone_end(), exec_keep_clones is
non-zero and reserve_preserved_mem() returns -EBUSY with no retry.  Such
siblings are about to be removed by de_thread() anyway.

> +	else
> +		mm->exec_keep_active = true;
> +	spin_unlock(&mm->page_table_lock);
> +	if (ret)
> +		return ret;
> +	bprm->preserving_mem = true;
> +
> +	read_lock(&tasklist_lock);
> +	for_each_process_thread(p, t) {
> +		if (same_thread_group(current, t))
> +			continue;
> +		task_lock(t);
> +		if (t->mm == mm)
> +			ret = -EBUSY;

[Severity: Medium]
Does this turn a valid vfork() or posix_spawn() exec into a failure?

If a process has a VM_EXEC_KEEP VMA and calls vfork() or
clone(CLONE_VM|CLONE_VFORK), the child shares the parent's mm.  When the
child calls execve() on a binary with accepts_preserved_mem set, the
suspended parent is in a different thread group and has t->mm == mm, so
reserve_preserved_mem() returns -EBUSY.  Before this patch that exec
succeeded.

Is failing the entire execve() with an -EBUSY that userspace does not expect
the intended behavior here, rather than exec without preservation?

Separately, this check is a point-in-time scan of t->mm.  Can it miss other
users of the mm that are not in a task's ->mm field, such as a kthread
holding the mm via kthread_use_mm() on a stored mm, or any holder of an
mmget() reference?  exec_keep_active only gates CLONE_VM in copy_mm(), and
mm_users is not examined.

> +		task_unlock(t);
> +		if (ret)
> +			goto out;
> +	}
> +out:
> +	read_unlock(&tasklist_lock);
> +	if (ret)
> +		release_preserved_mem(bprm, mm);
> +	return ret;
> +}

[ ... ]

> @@ -1067,6 +1140,7 @@ static int exec_mmap(struct linux_binprm *bprm)
>  		BUG_ON(active_mm != old_mm);
>  		setmax_mm_hiwater_rss(&tsk->signal->maxrss, old_mm);
>  		mm_update_next_owner(old_mm);
> +		release_preserved_mem(bprm, old_mm);
>  		mmput(old_mm);

[Severity: Medium]
Can an mm user that the reservation never saw keep old_mm alive after this
point?

By the time the reservation is released here, the VM_EXEC_KEEP VMAs have
already been moved to the new mm by vma_dup_some().  A holder such as one
using kthread_use_mm() on a stored mm would then see empty, newly faulted
pages in the old VMAs instead of the preserved data.

The commit message says other tasks using the old mm are quiesced, but this
appears to be enforced only for tasks on the task list.  Is there a check
that mm_users is no longer greater than one after de_thread()?

[ ... ]

> diff --git a/kernel/fork.c b/kernel/fork.c
> --- a/kernel/fork.c
> +++ b/kernel/fork.c

[ ... ]

> +static int exec_keep_clone_begin(struct mm_struct *mm)
> +{
> +	int ret = 0;
> +
> +	spin_lock(&mm->page_table_lock);
> +	if (mm->exec_keep_active)
> +		ret = -EBUSY;

[Severity: Medium]
Is it intended that every CLONE_VM clone is rejected while exec_keep_active
is set, including CLONE_THREAD clones from sibling threads of the execing
group?

The reservation window runs from reserve_preserved_mem() until exec_mmap()
or free_bprm(), and de_thread() can wait for siblings to exit inside that
window.  During that time a sibling's pthread_create() fails with -EBUSY
even though that sibling is about to be killed.

Also, exec_keep_active is checked only in copy_mm(), so other paths that
attach an existing mm to a task or kthread are not gated.

> +	else
> +		mm->exec_keep_clones++;
> +	spin_unlock(&mm->page_table_lock);
> +	return ret;
> +}

[ ... ]

-- 
Sashiko AI review · https://sashiko.vzint.dev/#/patchset/20261009-vfio_v1-v1-0-1901cd6c3c74@virtuozzo.com?part=2