Re: [PATCH mm-unstable 2/2] mm/mmap: fix NULL pointer dereference in dup_mmap() error handling

[Lorenzo Stoakes (Oracle)]

On Wed, Mar 04, 2026 at 03:00:57PM +0800, Hui Zhu wrote:
> From: Hui Zhu <zhuhui@xxxxxxxxxx>
>
> If dup_mmap() fails very early in its execution, it's possible that
> no VMAs have been inserted into the new mm's maple tree. When
> vma_next() is called in the cleanup block to retrieve the first
> VMA ('tmp'), it may return NULL.

Have you observed that or are you just guessing?

I mean given the below, you are just guessing :)

>
> The UNMAP_STATE macro and the subsequent call to tear_down_vmas()
> do not perform a NULL check on 'tmp' and directly attempt to
> access its fields (such as tmp->vm_end). This results in a
> NULL pointer dereference and a kernel panic.

Please don't suggest that something might happen if you can't demonstrate that
it might happen.

So there's this code just above that logic:

		/*
		 * The entire maple tree has already been duplicated, but
		 * replacing the vmas failed at mpnt (which could be NULL if
		 * all were allocated but the last vma was not fully set up).
		 * Use the start address of the failure point to clean up the
		 * partially initialized tree.
		 */
		if (!mm->map_count) {
			/* zero vmas were written to the new tree. */
			end = 0;
		} else if (mpnt) {
			/* partial tree failure */
			end = mpnt->vm_start;
		} else {
			/* All vmas were written to the new tree */
			end = ULONG_MAX;
		}

mm->map_count == number of VMAs, so we explicit check for this and the if (end)
{ ... } already covers this.

>
> This patch adds an explicit NULL check for 'tmp' before proceeding
> with the unmap and tear down logic in the failure path of dup_mmap().
>
> Signed-off-by: Hui Zhu <zhuhui@xxxxxxxxxx>

As a result of the above, this patch is completely unnecessary, so let's not
please.

Do try to prove to yourself that a suggested thing-that-might-happen might
actually happen in practice :)

The kernel does sometimes skip NULL checks when we set the code up such that a
pointer cannot be NULL, and this is one of those cases.

So it's important not to assume that a pointer _in theory_ being NULL means that
we need a NULL check somewhere, first check to see if _in practice_ a pointer
could be NULL first.

Thanks, Lorenzo

> ---
>  mm/mmap.c | 31 ++++++++++++++++++-------------
>  1 file changed, 18 insertions(+), 13 deletions(-)
>
> diff --git a/mm/mmap.c b/mm/mmap.c
> index 498c88a54a36..ca5645a2e456 100644
> --- a/mm/mmap.c
> +++ b/mm/mmap.c
> @@ -1879,19 +1879,24 @@ __latent_entropy int dup_mmap(struct mm_struct *mm, struct mm_struct *oldmm)
>  		if (end) {
>  			vma_iter_set(&vmi, 0);
>  			tmp = vma_next(&vmi);
> -			UNMAP_STATE(unmap, &vmi, /* first = */ tmp,
> -				    /* vma_start = */ 0, /* vma_end = */ end,
> -				    /* prev = */ NULL, /* next = */ NULL);
> -
> -			/*
> -			 * Don't iterate over vmas beyond the failure point for
> -			 * both unmap_vma() and free_pgtables().
> -			 */
> -			unmap.tree_end = end;
> -			flush_cache_mm(mm);
> -			unmap_region(&unmap);
> -			charge = tear_down_vmas(mm, &vmi, tmp, end);
> -			vm_unacct_memory(charge);
> +			if (tmp) {

This kind of hugely indented code is horrible. I know this function isn't great
but let's not make it worse.

> +				UNMAP_STATE(unmap, &vmi,
> +					    /* first = */ tmp,
> +					    /* vma_start = */ 0,
> +					    /* vma_end = */ end,
> +					    /* prev = */ NULL,
> +					    /* next = */ NULL);
> +
> +				/*
> +				 * Don't iterate over vmas beyond the failure point for
> +				 * both unmap_vma() and free_pgtables().
> +				 */
> +				unmap.tree_end = end;
> +				flush_cache_mm(mm);
> +				unmap_region(&unmap);
> +				charge = tear_down_vmas(mm, &vmi, tmp, end);
> +				vm_unacct_memory(charge);
> +			}
>  		}
>  		vma_iter_free(&vmi);
>  		__mt_destroy(&mm->mm_mt);
> --
> 2.43.0
>