[PATCH v2] mmap: Fix do_brk_flags() modifying obviously incorrect VMAs

[PATCH v2] mmap: Fix do_brk_flags() modifying obviously incorrect VMAs

[Liam Howlett]

Add more sanity checks to the VMA that do_brk_flags() will expand.
Ensure the VMA matches basic merge requirements within the function
before calling can_vma_merge_after().

Drop the duplicate checks from vm_brk_flags() since they will be
enforced later.

Fixes: 2e7ce7d354f2 ("mm/mmap: change do_brk_flags() to expand existing VMA and add do_brk_munmap()")
Suggested-by: Jann Horn <jannh@google.com>
Signed-off-by: Liam R. Howlett <Liam.Howlett@oracle.com>
---
 mm/mmap.c | 11 +++--------
 1 file changed, 3 insertions(+), 8 deletions(-)

diff --git a/mm/mmap.c b/mm/mmap.c
index a5eb2f175da0..5d48170fc2b2 100644
--- a/mm/mmap.c
+++ b/mm/mmap.c
@@ -2946,9 +2946,9 @@ static int do_brk_flags(struct ma_state *mas, struct vm_area_struct *vma,
 	 * Expand the existing vma if possible; Note that singular lists do not
 	 * occur after forking, so the expand will only happen on new VMAs.
 	 */
-	if (vma &&
-	    (!vma->anon_vma || list_is_singular(&vma->anon_vma_chain)) &&
-	    ((vma->vm_flags & ~VM_SOFTDIRTY) == flags)) {
+	if (vma && vma->vm_end == addr && !vma_policy(vma) &&
+	    can_vma_merge_after(vma, flags, NULL, NULL,
+				addr >> PAGE_SHIFT, NULL_VM_UFFD_CTX, NULL)) {
 		mas_set_range(mas, vma->vm_start, addr + len - 1);
 		if (mas_preallocate(mas, vma, GFP_KERNEL))
 			return -ENOMEM;
@@ -3035,11 +3035,6 @@ int vm_brk_flags(unsigned long addr, unsigned long request, unsigned long flags)
 		goto munmap_failed;
 
 	vma = mas_prev(&mas, 0);
-	if (!vma || vma->vm_end != addr || vma_policy(vma) ||
-	    !can_vma_merge_after(vma, flags, NULL, NULL,
-				 addr >> PAGE_SHIFT, NULL_VM_UFFD_CTX, NULL))
-		vma = NULL;
-
 	ret = do_brk_flags(&mas, vma, addr, len, flags);
 	populate = ((mm->def_flags & VM_LOCKED) != 0);
 	mmap_write_unlock(mm);
-- 
2.35.1

Re: [PATCH v2] mmap: Fix do_brk_flags() modifying obviously incorrect VMAs

[Andrew Morton]

On Mon, 5 Dec 2022 19:23:17 +0000 Liam Howlett <liam.howlett@oracle.com> wrote:

> Add more sanity checks to the VMA that do_brk_flags() will expand.
> Ensure the VMA matches basic merge requirements within the function
> before calling can_vma_merge_after().

I't unclear what's actually being fixed here.

Why do you feel we need the above changes?

> Drop the duplicate checks from vm_brk_flags() since they will be
> enforced later.
> 
> Fixes: 2e7ce7d354f2 ("mm/mmap: change do_brk_flags() to expand existing VMA and add do_brk_munmap()")

Fixes in what way?  Removing the duplicate checks?

Re: [PATCH v2] mmap: Fix do_brk_flags() modifying obviously incorrect VMAs

[Jann Horn]

On Mon, Dec 5, 2022 at 9:32 PM Andrew Morton <akpm@linux-foundation.org> wrote:
> On Mon, 5 Dec 2022 19:23:17 +0000 Liam Howlett <liam.howlett@oracle.com> wrote:
> > Add more sanity checks to the VMA that do_brk_flags() will expand.
> > Ensure the VMA matches basic merge requirements within the function
> > before calling can_vma_merge_after().
>
> I't unclear what's actually being fixed here.
>
> Why do you feel we need the above changes?
>
> > Drop the duplicate checks from vm_brk_flags() since they will be
> > enforced later.
> >
> > Fixes: 2e7ce7d354f2 ("mm/mmap: change do_brk_flags() to expand existing VMA and add do_brk_munmap()")
>
> Fixes in what way?  Removing the duplicate checks?

The old code would expand file VMAs on brk(), which is functionally
wrong and also dangerous in terms of locking because the brk() path
isn't designed for file VMAs and therefore doesn't lock the file
mapping. Checking can_vma_merge_after() ensures that new anonymous
VMAs can't be merged into file VMAs.

See https://lore.kernel.org/linux-mm/CAG48ez1tJZTOjS_FjRZhvtDA-STFmdw8PEizPDwMGFd_ui0Nrw@mail.gmail.com/
.

Re: [PATCH v2] mmap: Fix do_brk_flags() modifying obviously incorrect VMAs

[Vlastimil Babka]

On 12/5/22 22:55, Jann Horn wrote:
> On Mon, Dec 5, 2022 at 9:32 PM Andrew Morton <akpm@linux-foundation.org> wrote:
>> On Mon, 5 Dec 2022 19:23:17 +0000 Liam Howlett <liam.howlett@oracle.com> wrote:
>> > Add more sanity checks to the VMA that do_brk_flags() will expand.
>> > Ensure the VMA matches basic merge requirements within the function
>> > before calling can_vma_merge_after().
>>
>> I't unclear what's actually being fixed here.
>>
>> Why do you feel we need the above changes?
>>
>> > Drop the duplicate checks from vm_brk_flags() since they will be
>> > enforced later.
>> >
>> > Fixes: 2e7ce7d354f2 ("mm/mmap: change do_brk_flags() to expand existing VMA and add do_brk_munmap()")
>>
>> Fixes in what way?  Removing the duplicate checks?
> 
> The old code would expand file VMAs on brk(), which is functionally
> wrong and also dangerous in terms of locking because the brk() path
> isn't designed for file VMAs and therefore doesn't lock the file
> mapping. Checking can_vma_merge_after() ensures that new anonymous
> VMAs can't be merged into file VMAs.
> 
> See https://lore.kernel.org/linux-mm/CAG48ez1tJZTOjS_FjRZhvtDA-STFmdw8PEizPDwMGFd_ui0Nrw@mail.gmail.com/
> .

I guess the point is that if we fix it still within 6.1, we don't have to
devise how exactly this is exploitable, but due to the insufficient locking
it most likely is, right?

Re: [PATCH v2] mmap: Fix do_brk_flags() modifying obviously incorrect VMAs

[Jann Horn]

On Mon, Dec 5, 2022 at 11:13 PM Vlastimil Babka <vbabka@suse.cz> wrote:
> On 12/5/22 22:55, Jann Horn wrote:
> > On Mon, Dec 5, 2022 at 9:32 PM Andrew Morton <akpm@linux-foundation.org> wrote:
> >> On Mon, 5 Dec 2022 19:23:17 +0000 Liam Howlett <liam.howlett@oracle.com> wrote:
> >> > Add more sanity checks to the VMA that do_brk_flags() will expand.
> >> > Ensure the VMA matches basic merge requirements within the function
> >> > before calling can_vma_merge_after().
> >>
> >> I't unclear what's actually being fixed here.
> >>
> >> Why do you feel we need the above changes?
> >>
> >> > Drop the duplicate checks from vm_brk_flags() since they will be
> >> > enforced later.
> >> >
> >> > Fixes: 2e7ce7d354f2 ("mm/mmap: change do_brk_flags() to expand existing VMA and add do_brk_munmap()")
> >>
> >> Fixes in what way?  Removing the duplicate checks?
> >
> > The old code would expand file VMAs on brk(), which is functionally
> > wrong and also dangerous in terms of locking because the brk() path
> > isn't designed for file VMAs and therefore doesn't lock the file
> > mapping. Checking can_vma_merge_after() ensures that new anonymous
> > VMAs can't be merged into file VMAs.
> >
> > See https://lore.kernel.org/linux-mm/CAG48ez1tJZTOjS_FjRZhvtDA-STFmdw8PEizPDwMGFd_ui0Nrw@mail.gmail.com/
> > .
>
> I guess the point is that if we fix it still within 6.1, we don't have to
> devise how exactly this is exploitable,

Yeah, that was sort of my thinking.

> but due to the insufficient locking
> it most likely is, right?

To be honest, I don't really know how bad this is - pretty much the
only thing we're doing here is to change the VMA end. I don't know if
that messes up the address_space's interval tree or something?
I have no clue how that data structure looks.

Re: [PATCH v2] mmap: Fix do_brk_flags() modifying obviously incorrect VMAs

[Vlastimil Babka]

On 12/5/22 23:13, Vlastimil Babka wrote:
> On 12/5/22 22:55, Jann Horn wrote:
>> On Mon, Dec 5, 2022 at 9:32 PM Andrew Morton <akpm@linux-foundation.org> wrote:
>>> On Mon, 5 Dec 2022 19:23:17 +0000 Liam Howlett <liam.howlett@oracle.com> wrote:
>>> > Add more sanity checks to the VMA that do_brk_flags() will expand.
>>> > Ensure the VMA matches basic merge requirements within the function
>>> > before calling can_vma_merge_after().
>>>
>>> I't unclear what's actually being fixed here.
>>>
>>> Why do you feel we need the above changes?
>>>
>>> > Drop the duplicate checks from vm_brk_flags() since they will be
>>> > enforced later.
>>> >
>>> > Fixes: 2e7ce7d354f2 ("mm/mmap: change do_brk_flags() to expand existing VMA and add do_brk_munmap()")
>>>
>>> Fixes in what way?  Removing the duplicate checks?
>> 
>> The old code would expand file VMAs on brk(), which is functionally
>> wrong and also dangerous in terms of locking because the brk() path
>> isn't designed for file VMAs and therefore doesn't lock the file
>> mapping. Checking can_vma_merge_after() ensures that new anonymous
>> VMAs can't be merged into file VMAs.
>> 
>> See https://lore.kernel.org/linux-mm/CAG48ez1tJZTOjS_FjRZhvtDA-STFmdw8PEizPDwMGFd_ui0Nrw@mail.gmail.com/

And yeah, that URL should have been a Link: in the patch. And the scenario
it's fixing described in a bit more detail?

> I guess the point is that if we fix it still within 6.1, we don't have to
> devise how exactly this is exploitable, but due to the insufficient locking
> it most likely is, right?

Re: [PATCH v2] mmap: Fix do_brk_flags() modifying obviously incorrect VMAs

[Liam Howlett]

* Vlastimil Babka <vbabka@suse.cz> [221205 17:26]:
> On 12/5/22 23:13, Vlastimil Babka wrote:
> > On 12/5/22 22:55, Jann Horn wrote:
> >> On Mon, Dec 5, 2022 at 9:32 PM Andrew Morton <akpm@linux-foundation.org> wrote:
> >>> On Mon, 5 Dec 2022 19:23:17 +0000 Liam Howlett <liam.howlett@oracle.com> wrote:
> >>> > Add more sanity checks to the VMA that do_brk_flags() will expand.
> >>> > Ensure the VMA matches basic merge requirements within the function
> >>> > before calling can_vma_merge_after().
> >>>
> >>> I't unclear what's actually being fixed here.
> >>>
> >>> Why do you feel we need the above changes?
> >>>
> >>> > Drop the duplicate checks from vm_brk_flags() since they will be
> >>> > enforced later.
> >>> >
> >>> > Fixes: 2e7ce7d354f2 ("mm/mmap: change do_brk_flags() to expand existing VMA and add do_brk_munmap()")
> >>>
> >>> Fixes in what way?  Removing the duplicate checks?
> >> 
> >> The old code would expand file VMAs on brk(), which is functionally
> >> wrong and also dangerous in terms of locking because the brk() path
> >> isn't designed for file VMAs and therefore doesn't lock the file
> >> mapping. Checking can_vma_merge_after() ensures that new anonymous
> >> VMAs can't be merged into file VMAs.
> >> 
> >> See https://lore.kernel.org/linux-mm/CAG48ez1tJZTOjS_FjRZhvtDA-STFmdw8PEizPDwMGFd_ui0Nrw@mail.gmail.com/
> 
> And yeah, that URL should have been a Link: in the patch. And the scenario
> it's fixing described in a bit more detail?

Yes, sorry.  I should have made a better effort in describing what I was
fixing.  It seems I understated what was happening.

> 
> > I guess the point is that if we fix it still within 6.1, we don't have to
> > devise how exactly this is exploitable, but due to the insufficient locking
> > it most likely is, right?
>