linux-kernel - Re: [PATCH] mm/vma: reset VMA iterator on commit

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] [day] [month] [year] [list]

Message-ID: <u36foizqfsdur4mvy5y4fcsahxtcxljlhrouqkmetycihm2rns@ebjd5jrljgny>
Date: Tue, 10 Jun 2025 14:40:30 -0400
From: "Liam R. Howlett" <Liam.Howlett@...cle.com>
To: Lorenzo Stoakes <lorenzo.stoakes@...cle.com>
Cc: Andrew Morton <akpm@...ux-foundation.org>,
        Vlastimil Babka <vbabka@...e.cz>, Jann Horn <jannh@...gle.com>,
        Pedro Falcato <pfalcato@...e.de>, linux-mm@...ck.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH] mm/vma: reset VMA iterator on commit_merge() OOM failure

* Lorenzo Stoakes <lorenzo.stoakes@...cle.com> [250606 08:51]:
> While an OOM failure in commit_merge() isn't really feasible due to the
> allocation which might fail (a maple tree pre-allocation) being 'too small
> to fail', we do need to handle this case correctly regardless.
> 
> In vma_merge_existing_range(), we can theoretically encounter failures
> which result in an OOM error in two ways - firstly dup_anon_vma() might
> fail with an OOM error, and secondly commit_merge() failing, ultimately, to
> pre-allocate a maple tree node.
> 
> The abort logic for dup_anon_vma() resets the VMA iterator to the initial
> range, ensuring that any logic looping on this iterator will correctly
> proceed to the next VMA.
> 
> However the commit_merge() abort logic does not do the same thing. This
> resulted in a syzbot report occurring because mlockall() iterates through
> VMAs, is tolerant of errors, but ended up with an incorrect previous VMA
> being specified due to incorrect iterator state.
> 
> While making this change, it became apparent we are duplicating logic - the
> logic introduced in commit 41e6ddcaa0f1 ("mm/vma: add give_up_on_oom option
> on modify/merge, use in uffd release") duplicates the vmg->give_up_on_oom
> check in both abort branches.
> 
> Additionally, we observe that we can perform the anon_dup check safely on
> dup_anon_vma() failure, as this will not be modified should this call fail.
> 
> Finally, we need to reset the iterator in both cases, so now we can simply
> use the exact same code to abort for both.
> 
> We remove the VM_WARN_ON(err != -ENOMEM) as it would be silly for this to
> be otherwise and it allows us to implement the abort check more neatly.

Nice clean up.

Reviewed-by: Liam R. Howlett <Liam.Howlett@...cle.com>

> 
> Reported-by: syzbot+d16409ea9ecc16ed261a@...kaller.appspotmail.com
> Closes: https://lore.kernel.org/linux-mm/6842cc67.a00a0220.29ac89.003b.GAE@google.com/
> Fixes: 47b16d0462a4 ("mm: abort vma_modify() on merge out of memory failure")
> Cc: stable@...r.kernel.org
> Signed-off-by: Lorenzo Stoakes <lorenzo.stoakes@...cle.com>
> ---
>  mm/vma.c | 22 ++++------------------
>  1 file changed, 4 insertions(+), 18 deletions(-)
> 
> diff --git a/mm/vma.c b/mm/vma.c
> index 1497d7d28096..01b1d26d87b4 100644
> --- a/mm/vma.c
> +++ b/mm/vma.c
> @@ -967,26 +967,9 @@ static __must_check struct vm_area_struct *vma_merge_existing_range(
>  		err = dup_anon_vma(next, middle, &anon_dup);
>  	}
> 
> -	if (err)
> +	if (err || commit_merge(vmg))
>  		goto abort;
> 
> -	err = commit_merge(vmg);
> -	if (err) {
> -		VM_WARN_ON(err != -ENOMEM);
> -
> -		if (anon_dup)
> -			unlink_anon_vmas(anon_dup);
> -
> -		/*
> -		 * We've cleaned up any cloned anon_vma's, no VMAs have been
> -		 * modified, no harm no foul if the user requests that we not
> -		 * report this and just give up, leaving the VMAs unmerged.
> -		 */
> -		if (!vmg->give_up_on_oom)
> -			vmg->state = VMA_MERGE_ERROR_NOMEM;
> -		return NULL;
> -	}
> -
>  	khugepaged_enter_vma(vmg->target, vmg->flags);
>  	vmg->state = VMA_MERGE_SUCCESS;
>  	return vmg->target;
> @@ -995,6 +978,9 @@ static __must_check struct vm_area_struct *vma_merge_existing_range(
>  	vma_iter_set(vmg->vmi, start);
>  	vma_iter_load(vmg->vmi);
> 
> +	if (anon_dup)
> +		unlink_anon_vmas(anon_dup);
> +

Nit, I would have put this earlier in the abort path in case other
issues need to be addressed later.  Hardly worth a respin though, I
don't really see this ever needing to be done anyways.

>  	/*
>  	 * This means we have failed to clone anon_vma's correctly, but no
>  	 * actual changes to VMAs have occurred, so no harm no foul - if the
> --
> 2.49.0