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

[Suren Baghdasaryan <surenb@google.com>]

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