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

[David Hildenbrand (Arm)]

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@xxxxxxxxxxxxxxx
> Signed-off-by: Hui Su <sh_def@xxxxxxx>
> ---
> 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@xxxxxxx/
> 
>  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@xxxxxxxxx 

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);
 
                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