linux-kernel - Re: [PATCH] mm/huge_memory: Fix to make vma_adjust_trans_huge() use find

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: <CAO9qdTE8WO100AJo_bgM+J5yCpTtv=tRniNV2Rq3YAwQjx3JrA@mail.gmail.com>
Date: Thu, 21 Nov 2024 23:18:12 +0900
From: Jeongjun Park <aha310510@...il.com>
To: David Hildenbrand <david@...hat.com>
Cc: akpm@...ux-foundation.org, dave@...olabs.net, willy@...radead.org, 
	Liam.Howlett@...cle.com, linux-mm@...ck.org, linux-kernel@...r.kernel.org, 
	stable@...r.kernel.org, Lorenzo Stoakes <lorenzo.stoakes@...cle.com>
Subject: Re: [PATCH] mm/huge_memory: Fix to make vma_adjust_trans_huge() use
 find_vma() correctly

David Hildenbrand <david@...hat.com> wrote:
>
> On 21.11.24 14:44, David Hildenbrand wrote:
> > On 21.11.24 13:41, Jeongjun Park wrote:
> >> vma_adjust_trans_huge() uses find_vma() to get the VMA, but find_vma() uses
> >> the returned pointer without any verification, even though it may return NULL.
> >> In this case, NULL pointer dereference may occur, so to prevent this,
> >> vma_adjust_trans_huge() should be fix to verify the return value of find_vma().
> >>
> >> Cc: <stable@...r.kernel.org>
> >> Fixes: 685405020b9f ("mm/khugepaged: stop using vma linked list")
> >
> > If that's an issue, wouldn't it have predated that commit?
> >
> > struct vm_area_struct *next = vma->vm_next;
> > unsigned long nstart = next->vm_start;
> >
> > Would have also assumed that there is a next VMA that can be
> > dereferenced, no?
> >
>
> And looking into the details, we only assume that there is a next VMA if
> we are explicitly told to by the caller of vma_adjust_trans_huge() using
> "adjust_next".
>
> There is only one such caller,
> vma_merge_existing_range()->commit_merge() where we set adj_start ->
> "adjust_next" where we seem to have a guarantee that there is a next VMA.

I also thought that it would not be a problem in general cases, but I think
that there may be a special case (for example, a race condition...?) that can
occur in certain conditions, although I have not found it yet.

In addition, most functions except this one unconditionally check the return
value of find_vma(), so I think it would be better to handle the return value
of find_vma() consistently in this function as well, rather than taking the
risk and leaving it alone just because it seems to be okay.

Regards,

Jeongjun Park

>
> So I don't think there is an issue here (although the code does look
> confusing ...).
>
> Not sure, though, if a
>
> if (WARN_ON_ONCE(!next))
>         return;
>
> would be reasonable.
>
> --
> Cheers,
>
> David / dhildenb
>