From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from smtp.kernel.org (aws-us-west-2-korg-mail-1.web.codeaurora.org [10.30.226.201]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id BFD653A8748 for ; Wed, 4 Mar 2026 17:25:29 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=10.30.226.201 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1772645129; cv=none; b=KLPnGES4ZvTtuC7kBOGcH2PT3jOatBtOVCNiMVW+19avacEDm5TzQWxvtRqMby2CjDeZ3pnKHNDXxpaBjDT+eA0yEXHut4y+I6lcwXcuPQwfk3yTtmQ17hXre6AJd3w8zOSKAWuSM3iP9eajlKAim9K/vVsYiI1do/VdBLPS2Us= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1772645129; c=relaxed/simple; bh=WOppeuleJuW3iXckdFInG5Ten0AsL0JScXhelOhuklM=; h=Date:From:To:Cc:Subject:Message-ID:References:MIME-Version: Content-Type:Content-Disposition:In-Reply-To; b=YrwZfB8UWbyW7yFZV2BqlopafC6jeWWG9/eYjcHDQFQbxBlTqH1poz31c74TAl+Bk1ObXdk1Usob0LZeSETTnVOcYK4uCFJWMdnC9V9wyp85sLUg2Hd0qSIuhRe7MAfIxeazr1636nug01Lo7WYhARJXoDuwiNx50q5TdCOOZQ0= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=kernel.org header.i=@kernel.org header.b=HRlnvrM9; arc=none smtp.client-ip=10.30.226.201 Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=kernel.org header.i=@kernel.org header.b="HRlnvrM9" Received: by smtp.kernel.org (Postfix) with ESMTPSA id BEF35C4CEF7; Wed, 4 Mar 2026 17:25:28 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1772645129; bh=WOppeuleJuW3iXckdFInG5Ten0AsL0JScXhelOhuklM=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=HRlnvrM9rb9xng2RSidhax7C3dbOLj0hyVI5KgaRz0PcTDf2SJmaUH3y4BOvewGWp ktp3l1dfYk3TPZBdYpY/SO80mCpO22l67FEaDBqxLWGO2vnr+trnIFa1FpLvf3YcbM ZLKg37FTy8YP8PoPhT4XYu2aSFKbLSiZO7nCTTAZG4H7kQDplSxPkI7JmrSLkyHIis 1nE8pgZK6+2SECNYzjgwl0chZqPZJOMvYgJ+2QGuHqoVdKP9QZE6rQu5vB5DPvauBE xx+UYJioqxPOzbJlHSRJyV6a51RM3hyyvpmFn2ogfN/PApb07Z9/OLVeFUhhaD7WWS BbLEz5FFvDwBw== Date: Wed, 4 Mar 2026 17:25:26 +0000 From: "Lorenzo Stoakes (Oracle)" To: Hui Zhu Cc: Andrew Morton , "Liam R . Howlett" , Lorenzo Stoakes , Vlastimil Babka , Jann Horn , Pedro Falcato , linux-mm@kvack.org, linux-kernel@vger.kernel.org, Hui Zhu Subject: Re: [PATCH mm-unstable 2/2] mm/mmap: fix NULL pointer dereference in dup_mmap() error handling Message-ID: <49b1baaa-3cdc-416f-991b-bf2bb013341b@lucifer.local> References: <6dc840b8dc7da9f56787e7a353c633b3c12eda6a.1772607155.git.zhuhui@kylinos.cn> Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <6dc840b8dc7da9f56787e7a353c633b3c12eda6a.1772607155.git.zhuhui@kylinos.cn> On Wed, Mar 04, 2026 at 03:00:57PM +0800, Hui Zhu wrote: > From: Hui Zhu > > 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 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 >