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: <CAJuCfpGzazWVYzc9XXh+xTP9R7cMffiP2P4G5OJTQ0-Ji4xFEQ@mail.gmail.com>
Date: Thu, 31 Jul 2025 07:23:54 -0700
From: Suren Baghdasaryan <surenb@...gle.com>
To: Lokesh Gidra <lokeshgidra@...gle.com>
Cc: Hillf Danton <hdanton@...a.com>, akpm@...ux-foundation.org, peterx@...hat.com, 
	linux-mm@...ck.org, linux-kernel@...r.kernel.org, 
	syzkaller-bugs@...glegroups.com, 
	syzbot <syzbot+b446dbe27035ef6bd6c2@...kaller.appspotmail.com>, 
	stable@...r.kernel.org
Subject: 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@...gle.com> wrote:
>
> On Wed, Jul 30, 2025 at 6:58 PM Hillf Danton <hdanton@...a.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@...kaller.appspotmail.com
> > Closes: https://lore.kernel.org/all/68794b5c.a70a0220.693ce.0050.GAE@google.com/
> > Signed-off-by: Suren Baghdasaryan <surenb@...gle.com>
> > Cc: stable@...r.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

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ