linux-kernel - Re: [PATCH] kernel/fork: Be more careful about dup

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: <7a16bf18-e193-4e0a-9e3b-83f609cb9e72@lucifer.local>
Date: Fri, 24 Jan 2025 11:15:38 +0000
From: Lorenzo Stoakes <lorenzo.stoakes@...cle.com>
To: "Liam R. Howlett" <Liam.Howlett@...cle.com>
Cc: Andrew Morton <akpm@...ux-foundation.org>, maple-tree@...ts.infradead.org,
        linux-mm@...ck.org, linux-kernel@...r.kernel.org,
        Jann Horn <jannh@...gle.com>, Peter Zijlstra <peterz@...radead.org>,
        Michal Hocko <mhocko@...e.com>,
        Peng Zhang <zhangpeng.00@...edance.com>
Subject: Re: [PATCH] kernel/fork: Be more careful about dup_mmap() failures

On Thu, Jan 23, 2025 at 03:58:49PM -0500, Liam R. Howlett wrote:
> From: "Liam R. Howlett" <Liam.Howlett@...cle.com>
>
> In the even that there is a failure during dup_mmap(), the maple tree
> can be left in an unsafe state for other iterators besides the exit
> path.
>
> The unsafe state is created after the tree is cloned, but before the
> vmas are replaced; if a vma allocation fails (for instance), then the
> tree will have a marker (XA_ZERO_ENTRY) to denote where to stop
> destroying vmas on the exit path.  This marker replaces a vma in the
> tree and may be treated as a pointer to a vma in iterators besides the
> special case exit_mmap() iterator.

Thanks for addresing this.

In the original issue, the problem arose in vm_area_dup() which I tracked
down to vma_iter_load() -> mas_walk() (see [0]).

I'm not sure if your change would actually have addressed this? Would the
MMF_UNSTABLE flag prevent this code path from being taken (if so where?),
if not don't we need to add a check for this somewhere?

[0]:https://lore.kernel.org/all/aa2c1930-becc-4bc5-adfb-96e88290acc7@lucifer.local/

>
> All the locks are dropped before the exit_mmap() call, but the
> incomplete mm_struct can be reached through (at least) the rmap finding
> the vmas which have a pointer back to the mm_struct.
>
> Up to this point, there have been no issues with being able to find an
> mm_sturct that was only partially initialised.  Syzbot was able to make
> the incomplete mm_struct fail with recent forking changes, so it has
> been proven unsafe to use the mm_sturct that hasn't been initialised, as
> referenced in the link below.
>
> Although 8ac662f5da19f ("fork: avoid inappropriate uprobe access to
> invalid mm") fixed the uprobe access, it does not completely remove the
> race.
>
> This patch sets the MMF_OOM_SKIP to avoid the iteration of the vmas on
> the oom side (even though this is extremely unlikely to be selected as
> an oom victim in the race window), and sets MMF_UNSTABLE to avoid other
> potential users from using a partially initialised mm_struct.

Good to have it set also I think.

We were concerned that _somehow_ MMF_UNSTABLE might cause some issue in
OOM, which was why when I first thought of this we decided maybe not just
to be safe, but I am confident this will not be an issue as indicating that
something is about to be torn down and is unstable when it is in fact
unstable and about to be torn down is correct, plus I checked all the paths
where this impacted and found no issues.

And in any case, setting this flag avoids the problem...!

>
> Link: https://lore.kernel.org/all/6756d273.050a0220.2477f.003d.GAE@google.com/
> Fixes: d240629148377 ("fork: use __mt_dup() to duplicate maple tree in dup_mmap()")
> Cc: Jann Horn <jannh@...gle.com>
> Cc: Lorenzo Stoakes <lorenzo.stoakes@...cle.com>
> Cc: Peter Zijlstra <peterz@...radead.org>
> Cc: Michal Hocko <mhocko@...e.com>
> Cc: Peng Zhang <zhangpeng.00@...edance.com>
> Signed-off-by: Liam R. Howlett <Liam.Howlett@...cle.com>
> ---
>  kernel/fork.c | 17 ++++++++++++++---
>  1 file changed, 14 insertions(+), 3 deletions(-)
>
> diff --git a/kernel/fork.c b/kernel/fork.c
> index ded49f18cd95c..20b2120f019ca 100644
> --- a/kernel/fork.c
> +++ b/kernel/fork.c
> @@ -760,7 +760,8 @@ static __latent_entropy int dup_mmap(struct mm_struct *mm,
>  		mt_set_in_rcu(vmi.mas.tree);
>  		ksm_fork(mm, oldmm);
>  		khugepaged_fork(mm, oldmm);
> -	} else if (mpnt) {
> +	} else {
> +
>  		/*
>  		 * The entire maple tree has already been duplicated. If the
>  		 * mmap duplication fails, mark the failure point with
> @@ -768,8 +769,18 @@ static __latent_entropy int dup_mmap(struct mm_struct *mm,
>  		 * stop releasing VMAs that have not been duplicated after this
>  		 * point.
>  		 */
> -		mas_set_range(&vmi.mas, mpnt->vm_start, mpnt->vm_end - 1);
> -		mas_store(&vmi.mas, XA_ZERO_ENTRY);
> +		if (mpnt) {
> +			mas_set_range(&vmi.mas, mpnt->vm_start, mpnt->vm_end - 1);
> +			mas_store(&vmi.mas, XA_ZERO_ENTRY);
> +			/* Avoid OOM iterating a broken tree */
> +			set_bit(MMF_OOM_SKIP, &mm->flags);
> +		}
> +		/*
> +		 * The mm_struct is going to exit, but the locks will be dropped
> +		 * first.  Set the mm_struct as unstable is advisable as it is
> +		 * not fully initialised.
> +		 */
> +		set_bit(MMF_UNSTABLE, &mm->flags);

Do we still need to do this if !mpnt?

Also 'mpnt' is in the running for the worst variable name in mm...

>  	}
>  out:
>  	mmap_write_unlock(mm);
> --
> 2.43.0
>