Re: [PATCH 1/1] userfaultfd: fix a crash when UFFDIO_MOVE handles a THP hole

On Thu, Jul 31, 2025 at 12:35 AM Lokesh Gidra lokeshgidra@google.com wrote:

On Wed, Jul 30, 2025 at 6:58 PM Hillf Danton hdanton@sina.com wrote:

...

#syz test

When UFFDIO_MOVE is used with UFFDIO_MOVE_MODE_ALLOW_SRC_HOLES and it encounters a non-present THP, it fails to properly recognize an unmapped hole and tries to access a non-existent folio, resulting in a crash. Add a check to skip non-present THPs.

Thanks Suren for promptly addressing this issue.

...

Fixes: adef440691ba ("userfaultfd: UFFDIO_MOVE uABI") Reported-by: syzbot+b446dbe27035ef6bd6c2@syzkaller.appspotmail.com Closes: https://lore.kernel.org/all/68794b5c.a70a0220.693ce.0050.GAE@google.com/ Signed-off-by: Suren Baghdasaryan surenb@google.com Cc: stable@vger.kernel.org

---

mm/userfaultfd.c | 38 +++++++++++++++++++++++--------------- 1 file changed, 23 insertions(+), 15 deletions(-)

diff --git a/mm/userfaultfd.c b/mm/userfaultfd.c index cbed91b09640..60be8080ddd0 100644 --- a/mm/userfaultfd.c +++ b/mm/userfaultfd.c @@ -1818,27 +1818,35 @@ ssize_t move_pages(struct userfaultfd_ctx *ctx, unsigned long dst_start,

            ptl = pmd_trans_huge_lock(src_pmd, src_vma);
            if (ptl) {

•                   /* Check if we can move the pmd without splitting it. */
  

•                   if (move_splits_huge_pmd(dst_addr, src_addr, src_start + len) ||
  

•                       !pmd_none(dst_pmdval)) {
  

•                           struct folio *folio = pmd_folio(*src_pmd);
  

•                   if (pmd_present(*src_pmd) || is_pmd_migration_entry(*src_pmd)) {
  

•                           /* Check if we can move the pmd without splitting it. */
  

•                           if (move_splits_huge_pmd(dst_addr, src_addr, src_start + len) ||
  

•                               !pmd_none(dst_pmdval)) {
  

•                                   if (pmd_present(*src_pmd)) {
  

•                                           struct folio *folio = pmd_folio(*src_pmd);
  

•                                           if (!folio || (!is_huge_zero_folio(folio) &&
  

•                                                          !PageAnonExclusive(&folio->page))) {
  

•                                                   spin_unlock(ptl);
  

•                                                   err = -EBUSY;
  

•                                                   break;
  

•                                           }
  

•                                   }
  
  

•                           if (!folio || (!is_huge_zero_folio(folio) &&
  

•                                          !PageAnonExclusive(&folio->page))) {
                                    spin_unlock(ptl);
  

•                                   err = -EBUSY;
  

•                                   break;
  

•                                   split_huge_pmd(src_vma, src_pmd, src_addr);
  

•                                   /* The folio will be split by move_pages_pte() */
  

•                                   continue;
                            }
  
  

•                           err = move_pages_huge_pmd(mm, dst_pmd, src_pmd,
  

•                                                     dst_pmdval, dst_vma, src_vma,
  

•                                                     dst_addr, src_addr);
  

•                   } else {
  

•                           /* nothing to do to move a hole */
                            spin_unlock(ptl);
  

•                           split_huge_pmd(src_vma, src_pmd, src_addr);
  

•                           /* The folio will be split by move_pages_pte() */
  

•                           continue;
  

•                           err = 0;
  

I think we need to act here depending on whether UFFDIO_MOVE_MODE_ALLOW_SRC_HOLES is set or not.

Hmm, yes, I think you are right. I thought we would bail out earlier if !UFFDIO_MOVE_MODE_ALLOW_SRC_HOLES but I think it's possible to get here if the PMD was established earlier but then unmapped.

       err = (mode & UFFDIO_MOVE_MODE_ALLOW_SRC_HOLES) ? 0 : -ENOENT;

Also, IMO, the step_size in this case should be the minimum of remaining length and HPAGE_PMD_SIZE.

Ah, ok. I think it matters only for incrementing "moved" correctly because otherwise the functionality is the same.

...

                    }

•                   err = move_pages_huge_pmd(mm, dst_pmd, src_pmd,
  

•                                             dst_pmdval, dst_vma, src_vma,
  

•                                             dst_addr, src_addr);
                    step_size = HPAGE_PMD_SIZE;
            } else {
                    if (pmd_none(*src_pmd)) {
  

I have a related question/doubt: why do we populate the page-table hierarchy on the src side [1] (and then also at line 1857) when a hole is found? IMHO, it shouldn't be needed. Depending on whether UFFDIO_MOVE_MODE_ALLOW_SRC_HOLES is set or not, it should either return -ENOENT, or continue past the hole. Please correct me if I'm wrong.

I thought about that too. I think it's done to simplify the logic. This way we can treat the cases when PMD was never allocated and when PMD was allocated, mapped and then unmapped the same way.

[1] https://elixir.bootlin.com/linux/v6.16/source/mm/userfaultfd.c#L1797

...

base-commit: 01da54f10fddf3b01c5a3b80f6b16bbad390c302

2.50.1.552.g942d659e1b-goog