linux-kernel - Re: [RFC PATCH] mm/mmap: Fix uprobe anon page be overwritten when expanding vma during mremap

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 for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <172df994-7f24-4fbb-876c-4fab22937177@redhat.com>
Date: Fri, 30 May 2025 10:50:25 +0200
From: David Hildenbrand <david@...hat.com>
To: Lorenzo Stoakes <lorenzo.stoakes@...cle.com>
Cc: Pu Lehui <pulehui@...weicloud.com>, Oleg Nesterov <oleg@...hat.com>,
 mhiramat@...nel.org, peterz@...radead.org, Liam.Howlett@...cle.com,
 akpm@...ux-foundation.org, vbabka@...e.cz, jannh@...gle.com,
 pfalcato@...e.de, linux-mm@...ck.org, linux-kernel@...r.kernel.org,
 pulehui@...wei.com
Subject: Re: [RFC PATCH] mm/mmap: Fix uprobe anon page be overwritten when
 expanding vma during mremap

On 30.05.25 10:41, Lorenzo Stoakes wrote:
> On Fri, May 30, 2025 at 10:33:16AM +0200, David Hildenbrand wrote:
>> On 29.05.25 18:07, Pu Lehui wrote:
>>>
>>> On 2025/5/28 17:03, David Hildenbrand wrote:
>>>> On 27.05.25 15:38, Pu Lehui wrote:
>>>>> Hi David,
>>>>>
>>>>> On 2025/5/27 2:46, David Hildenbrand wrote:
>>>>>> On 26.05.25 17:48, Oleg Nesterov wrote:
>>>>>>> Hi Lehui,
>>>>>>>
>>>>>>> As I said, I don't understand mm/, so can't comment, but...
>>>>>>>
>>>>>>> On 05/26, Pu Lehui wrote:
>>>>>>>>
>>>>>>>> To make things simpler, perhaps we could try post-processing, that is:
>>>>>>>>
>>>>>>>> diff --git a/mm/mremap.c b/mm/mremap.c
>>>>>>>> index 83e359754961..46a757fd26dc 100644
>>>>>>>> --- a/mm/mremap.c
>>>>>>>> +++ b/mm/mremap.c
>>>>>>>> @@ -240,6 +240,11 @@ static int move_ptes(struct
>>>>>>>> pagetable_move_control
>>>>>>>> *pmc,
>>>>>>>>                     if (pte_none(ptep_get(old_pte)))
>>>>>>>>                             continue;
>>>>>>>>
>>>>>>>> +               /* skip move pte when expanded range has uprobe */
>>>>>>>> +               if (unlikely(pte_present(*new_pte) &&
>>>>>>>> +                            vma_has_uprobes(pmc->new, new_addr,
>>>>>>>> new_addr +
>>>>>>>> PAGE_SIZE)))
>>>>>>>> +                       continue;
>>>>>>>> +
>>>>>>>
>>>>>>> I was thinking about
>>>>>>>
>>>>>>>        WARN_ON(!pte_none(*new_pte))
>>>>>>>
>>>>>>> at the start of the main loop.
>>>>>>>
>>>>>>> Obviously not to fix the problem, but rather to make it more explicit.
>>>>>>
>>>>>> Yeah, WARN_ON_ONCE().
>>>>>>
>>>>>> We really should fix the code to not install uprobes into the area we
>>>>>> are moving.
>>>>> Alright, so let's try this direction.
>>>>>
>>>>>>
>>>>>> Likely, the correct fix will be to pass the range as well to
>>>>>> uprobe_mmap(), and passing that range to build_probe_list().
>>>>>
>>>>> It will be great. But IIUC, the range we expand to is already included
>>>>> when entering uprobe_mmap and also build_probe_list.
>>>>
>>>> Right, you'd have to communicate that information through all layers
>>>> (expanded range).
>>>>
>>>> As an alternative, maybe we can really call handle_vma_uprobe() after
>>>> moving the pages.
>>>
>>> Hi David,
>>>
>>> Not sure if this is possible, but I think it would be appropriate to not
>>> handle this uprobe_mmap at the source, and maybe we should make it clear
>>> that new_pte must be NULL when move_ptes, otherwise it should be an
>>> exception?
>>
>> Yeah, we should ay least document that if we find any non-none pte in the
>> range we are moving to, we have a big problem.
>>
>> I think the main issue is that vma_complete() calls uprobe_mmap() before
>> moving the page tables over.
> 
> Well vma_complete() is not _normally_ invoked before moving page tables,
> it's mremap that's making things strange :)
> 
> That's why I think my suggested approach of specifically indicating that we
> want different behaviour for mremap is a reasonable one here, as it special
> cases things for this case.
> 
> However...
> 
>>
>> If we could defer the uprobe_mmap() call, we might be good.
>>
>> The entry point is copy_vma_and_data(), where we call copy_vma() before
>> move_page_tables().
>>
>> copy_vma() should trigger the uprobe_mmap() through vma_merge_new_range().
>>
>> I wonder if there might be a clean way to move the uprobe_mmap() out of
>> vma_complete(). (or at least specify to skip it because it will be done
>> manually).
> 
> ...I would also love to see some means of not having to invoke
> uprobe_mmap() in the VMA code, but I mean _at all_.
> 
> But that leads into my desire to not do:
> 
> if (blah blah)
> 	some_specific_hardcoded_case();
> 
> I wish we had a better means of hooking stuff like this.
> 
> However I don't think currently we can reasonably do so, as in all other
> merge cases we _do_ want to invoke it.

"all other" -- not so sure.

Why would we invoke uprobe when merging VMAs after mprotect, mremap, 
madvise, ordinary mremap where we are not mapping anything new but just 
... merging VMAs?

Really, we need to invoke uprobe only when adding new VMAs or extending 
existing VMAs -- mapping new file ranges some way.

Or am I missing something important?

-- 
Cheers,

David / dhildenb