Re: [PATCH v2] mm/migrate_device: avoid out-of-bounds writes for compound folios

[Matthew Brost <matthew.brost@intel.com>]

On Thu, Aug 27, 2026 at 05:14:40PM +0200, David Hildenbrand (Arm) wrote:
> On 8/17/26 14:08, Hui Su wrote:
> > migrate_device_range() and migrate_device_pfns() clear the entries
> > following a compound folio so that the PFN arrays retain their
> > page-granular representation.
> > 
> > If a compound folio extends beyond the end of the caller-provided range,
> > the loops clear all following folio entries without limiting them to the
> > number of slots remaining in the npages-sized array, causing an
> > out-of-bounds write.
> > 
> > Do not proceed with a compound folio if its page-granular representation
> > does not fit entirely in the remaining PFN array. If this happens, drop
> > any reference and lock acquired for the folio, clear the remaining
> > entries, and stop collecting.
> > 
> > Observed with a KASAN x86 QEMU kernel using the HMM
> > migrate_anon_huge_zero selftest. Closing /dev/hmm_dmirror0 after
> > migrating an anonymous huge page to device memory exercises:
> > 
> >   dmirror_fops_release()
> >     -> dmirror_device_evict_chunk()
> >       -> migrate_device_range()
> > 
> > Fixes: a30b48bf1b24 ("mm/migrate_device: implement THP migration of zone device pages")
> > Cc: stable@vger.kernel.org
> > Signed-off-by: Hui Su <sh_def@163.com>
> > ---
> > Changes in v2:
> > - Do not partially represent a compound folio when it does not fit in
> >   the remaining PFN array.
> > - Drop any reference and lock acquired for that folio, clear the
> >   remaining entries, and stop collecting, as suggested by Balbir Singh.
> > 
> > v1: https://lore.kernel.org/lkml/20260817074350.442493-2-sh_def@163.com/
> > 
> >  mm/migrate_device.c | 18 ++++++++++++++++++
> >  1 file changed, 18 insertions(+)
> > 
> > diff --git a/mm/migrate_device.c b/mm/migrate_device.c
> > index 908d2d4ec43a..69b8d0660bab 100644
> > --- a/mm/migrate_device.c
> > +++ b/mm/migrate_device.c
> > @@ -1400,6 +1400,15 @@ int migrate_device_range(unsigned long *src_pfns, unsigned long start,
> >  
> >  		src_pfns[i] = migrate_device_pfn_lock(pfn);
> >  		nr = folio_nr_pages(folio);
> > +		if (nr > npages - i) {
> > +			if (src_pfns[i] & MIGRATE_PFN_MIGRATE) {
> > +				folio_unlock(folio);
> > +				folio_put(folio);
> > +			}
> > +			memset(&src_pfns[i], 0,
> > +			       (npages - i) * sizeof(*src_pfns));
> > +			break;
> > +		}
> >  		if (nr > 1) {
> >  			src_pfns[i] |= MIGRATE_PFN_COMPOUND;
> >  			for (j = 1; j < nr; j++)
> > @@ -1434,6 +1443,15 @@ int migrate_device_pfns(unsigned long *src_pfns, unsigned long npages)
> >  
> >  		src_pfns[i] = migrate_device_pfn_lock(src_pfns[i]);
> >  		nr = folio_nr_pages(folio);
> > +		if (nr > npages - i) {
> > +			if (src_pfns[i] & MIGRATE_PFN_MIGRATE) {
> > +				folio_unlock(folio);
> > +				folio_put(folio);
> > +			}
> > +			memset(&src_pfns[i], 0,
> > +			       (npages - i) * sizeof(*src_pfns));
> > +			break;
> > +		}
> >  		if (nr > 1) {
> >  			src_pfns[i] |= MIGRATE_PFN_COMPOUND;
> >  			for (j = 1; j < nr; j++)
> 
> The code duplication here makes me angry. :) And using a memset on one branch but
> not on the other is weird.
> 
> But is this the right fix or rather what
> 
> 	https://lore.kernel.org/r/20260805231041.3791771-3-matthew.brost@intel.com 
> 

I'm not sure my patch is right now that I think about, or if this patch
is right, or your suggestion either :).

> tried? That fix would also need a cleanup but seems code-wise simpler. A cleanup could look like:
> 
> diff --git a/mm/migrate_device.c b/mm/migrate_device.c
> index 762c5cee8fecc..480bf59bc2425 100644
> --- a/mm/migrate_device.c
> +++ b/mm/migrate_device.c
> @@ -1419,10 +1419,9 @@ int migrate_device_range(unsigned long *src_pfns, unsigned long start,
>         for (pfn = start, i = 0; i < npages; pfn++, i++) {
>                 struct page *page = pfn_to_page(pfn);
>                 struct folio *folio = page_folio(page);
> -               unsigned int nr = 1;
> +               unsigned int nr = min(folio_nr_pages(folio), npages - i);
>  
>                 src_pfns[i] = migrate_device_pfn_lock(pfn);
> -               nr = folio_nr_pages(folio);
>                 if (nr > 1) {
>                         src_pfns[i] |= MIGRATE_PFN_COMPOUND;
>                         for (j = 1; j < nr; j++)
> @@ -1453,10 +1452,9 @@ int migrate_device_pfns(unsigned long *src_pfns, unsigned long npages)
>         for (i = 0; i < npages; i++) {
>                 struct page *page = pfn_to_page(src_pfns[i]);
>                 struct folio *folio = page_folio(page);
> -               unsigned int nr = 1;
> +               unsigned int nr = min(folio_nr_pages(folio), npages - i);

I think the semantics of both these function should be that if a user
passes in a PFN array and one of the PFNs exceeds the `npages` limit,
that is a misuse of this function and we should unwind and return an
error. The caller is providing the PFNs and should therefore know the
backing page alignment, so `npages` should always end exactly on a page
boundary. The sole caller of migrate_device_pfns function is
`drm_gpusvm/pagemap`, which certainly adheres to this requirement.

The two callers of migrate_device_range, also seemly adheres to this
requirement.

Matt

>  
>                 src_pfns[i] = migrate_device_pfn_lock(src_pfns[i]);
> -               nr = folio_nr_pages(folio);
>                 if (nr > 1) {
>                         src_pfns[i] |= MIGRATE_PFN_COMPOUND;
>                         for (j = 1; j < nr; j++)
> 
> 
> And as a further cleanup, we'd better de-duplicate that code and possibly use a
> memset for clearing, getting rid of j entirely.
> 
> 
> 
> -- 
> Cheers,
> 
> David