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

[David Hildenbrand (Arm)]

On 8/28/26 06:44, Matthew Brost wrote:
> 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@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 
>>
> 
> 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.

That's what I was thinking: because how should we partially migrate folios?
Doesn't make sense :)

If this cannot be triggered today, neither Fixes: nor CC: stable is appropriate.

Agreed that we should just warn.

-- 
Cheers,

David