linux-kernel - Re: [PATCH 1/7] mm/mremap: correctly handle partial mremap() of VMA starting at 0

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: <2b28da79-8f2f-49bc-805f-452f91958eac@lucifer.local>
Date: Wed, 5 Mar 2025 19:46:07 +0000
From: Lorenzo Stoakes <lorenzo.stoakes@...cle.com>
To: Vlastimil Babka <vbabka@...e.cz>
Cc: Andrew Morton <akpm@...ux-foundation.org>,
        "Liam R . Howlett" <Liam.Howlett@...cle.com>,
        Jann Horn <jannh@...gle.com>, linux-mm@...ck.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH 1/7] mm/mremap: correctly handle partial mremap() of VMA
 starting at 0

On Wed, Mar 05, 2025 at 12:50:25PM +0100, Vlastimil Babka wrote:
> On 3/3/25 12:08, Lorenzo Stoakes wrote:
> > Consider the case of a a partial mremap() (that results in a VMA split) of
> > an accountable VMA (i.e. which has the VM_ACCOUNT flag set) whose start
> > address is zero, with the MREMAP_MAYMOVE flag specified and a scenario
> > where a move does in fact occur:
> >
> >        addr  end
> >         |     |
> >         v     v
> >     |-------------|
> >     |     vma     |
> >     |-------------|
> >     0
> >
> > This move is affected by unmapping the range [addr, end). In order to
> > prevent an incorrect decrement of accounted memory which has already been
> > determined, the mremap() code in move_vma() clears VM_ACCOUNT from the VMA
> > prior to doing so, before reestablishing it in each of the VMAs post-split:
> >
> >     addr  end
> >      |     |
> >      v     v
> >  |---|     |---|
> >  | A |     | B |
> >  |---|     |---|
> >
> > Commit 6b73cff239e5 ("mm: change munmap splitting order and move_vma()")
> > changed this logic such as to determine whether there is a need to do so by
> > establishing account_start and account_end and, in the instance where such
> > an operation is required, assigning them to vma->vm_start and vma->vm_end.
> >
> > Later the code checks if the operation is required for 'A' referenced above
> > thusly:
> >
> > 	if (account_start) {
> > 		...
> > 	}
> >
> > However, if the VMA described above has vma->vm_start == 0, which is now
> > assigned to account_start, this branch will not be executed.
> >
> > As a result, the VMA 'A' above will remain stripped of its VM_ACCOUNT flag,
> > incorrectly.
> >
> > The fix is to simply convert these variables to booleans and set them as
> > required.
> >
> > Signed-off-by: Lorenzo Stoakes <lorenzo.stoakes@...cle.com>
> > Fixes: 6b73cff239e5 ("mm: change munmap splitting order and move_vma()")
> > Cc: stable@...r.kernel.org
>
> Looks like the variables would be more accurately named account_prev and
> account_next? But maybe it's refactored later anyway. Keeps the diff to
> stable backport smaller.

Yeah indeed, I think it's better to keep it relatively smaller.

>
> Reviewed-by: Vlastimil Babka <vbabka@...e.cz>
>

Thanks!