Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
New issues:
- [Critical] Data corruption due to incorrect source page population during THP allocation fallback.
Pre-existing issues:
- [Critical] Out-of-bounds DMA read during partial THP migrations because full THP order is blindly encoded.
- [High] Unconditionally setting MIGRATE_PFN_COMPOUND on destination PFNs breaks PTE-level migration and leaks THPs.
--
--- Patch [3]: [PATCH 3/4] drm/pagemap: Fix folio allocation fallback and use-after-put ---
commit f92ba61577518de0d554e1323b18348b10e22b1a
Author: Matthew Brost <[email protected]>
Subject: drm/pagemap: Fix folio allocation fallback and use-after-put
This commit adds a fallback to order-0 allocations in
drm_pagemap_migrate_populate_ram_pfn() if higher-order folio allocation
fails without setting MIGRATE_PFN_COMPOUND. It also factors out source
page population into drm_pagemap_migrate_populate_src_pages() and fixes a
use-after-put when freeing pages.
> diff --git a/drivers/gpu/drm/drm_pagemap.c b/drivers/gpu/drm/drm_pagemap.c
> index 892b325fa99b8..7610e233d238c 100644
> --- a/drivers/gpu/drm/drm_pagemap.c
> +++ b/drivers/gpu/drm/drm_pagemap.c
[ ... ]
> @@ -383,6 +383,58 @@ drm_pagemap_migrate_map_system_pages(struct device *dev,
> return 0;
> }
>
> +/**
> + * drm_pagemap_migrate_populate_src_pages() - Populate the source page array
> + * @pages: Array of source pages to populate
> + * @src_mpfn: Source array of migrate PFNs
> + * @dst_mpfn: Destination array of migrate PFNs
> + * @npages: Number of pages in the arrays
> + *
> + * Populate @pages with the device pages the copy callback is to read from.
> + *
> + * Entries are normally only populated at the head of each source folio, with
> + * the copy callback deriving the rest of the folio from the order recorded in
> + * the corresponding drm_pagemap_addr. That does not work where
> + * drm_pagemap_migrate_populate_ram_pfn() had to demote a higher-order source
> + * folio to order-0 destination folios: the drm_pagemap_addr entries are then
> + * per-page, and the copy callback needs a source page for each of them.
> + * Populate every entry for those ranges.
> + *
> + * Note that the source folio itself is only split later, by
> + * migrate_vma_pages() / migrate_device_pages(), so its order cannot be used to
> + * detect the demotion - the destination has to be inspected instead.
> + */
> +static void drm_pagemap_migrate_populate_src_pages(struct page **pages,
> + unsigned long *src_mpfn,
> + unsigned long *dst_mpfn,
> + unsigned long npages)
> +{
> + unsigned long i;
> +
> + for (i = 0; i < npages;) {
> + struct page *page = migrate_pfn_to_page(src_mpfn[i]);
> + unsigned int order = 0;
> + unsigned long j, nr;
> +
> + if (!page) {
> + i++;
> + continue;
> + }
> +
> + order = folio_order(page_folio(page));
> + nr = NR_PAGES(order);
> +
> + if (order && !(dst_mpfn[i] & MIGRATE_PFN_COMPOUND)) {
> + for (j = 0; j < nr && i + j < npages; j++)
> + pages[i + j] = folio_page(page_folio(page), j);
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
[Severity: Critical]
Can this result in data corruption by incorrectly assigning the head page?
If migrate_vma_setup() is triggered by an unaligned VMA or partial migration,
it can collect a partial device-private THP and place a tail page in
migrate.src[0].
If the subsequent THP allocation fails and we fall back to order-0 allocations
without MIGRATE_PFN_COMPOUND, this loop handles the demotion. However, since
page_folio(page) returns the head page, j = 0 will assign the head page rather
than the correct subpage offset indicated by the original tail page.
Could this cause user memory at the destination to be populated with the wrong
data from the source?
> + } else {
> + pages[i] = page;
> + }
> +
> + i += nr;
> + }
> +}
[ ... ]
> @@ -875,6 +927,7 @@ static int drm_pagemap_migrate_populate_ram_pfn(struct vm_area_struct *vas,
> struct page *page = NULL, *src_page;
> struct folio *folio;
> unsigned int order = 0;
> + gfp_t gfp = GFP_HIGHUSER;
>
> if (!(src_mpfn[i] & MIGRATE_PFN_MIGRATE))
> goto next;
> @@ -890,12 +943,38 @@ static int drm_pagemap_migrate_populate_ram_pfn(struct vm_area_struct *vas,
> }
>
> order = folio_order(page_folio(src_page));
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
[Severity: Critical]
This is a pre-existing issue, but does this unconditionally skip ahead by the
full THP order, ignoring the actual bounds of a partial migration?
If migrate_vma_setup() processes an unaligned VMA range backed by a device THP,
it collects tail pages and npages will be smaller than the THP size. This code
unconditionally allocates a THP matching the source's full order, and then
later in this loop advances the index by the full order:
i += NR_PAGES(order);
When drm_pagemap_migrate_map_system_pages() subsequently encodes the full
compound order into the DMA address, the driver's copy_to_ram callback reads
the full THP size starting from the tail page.
Could this lead to an out-of-bounds physical memory read and potential memory
corruption if the copy exceeds the source physical allocation boundaries?
> + if (order)
> + gfp |= __GFP_NOWARN;
>
> - /* TODO: Support fallback to single pages if THP allocation fails */
> if (vas)
> - folio = vma_alloc_folio(GFP_HIGHUSER, order, vas, addr);
> + folio = vma_alloc_folio(gfp, order, vas, addr);
> else
> - folio = folio_alloc(GFP_HIGHUSER, order);
> + folio = folio_alloc(gfp, order);
> +
> + if (!folio && order) {
> + /*
> + * Higher-order allocation failed, fall back to
> + * order-0 allocations for the entire range covered
> + * by the original higher-order allocation, without
> + * setting MIGRATE_PFN_COMPOUND, until we move past
> + * that range.
> + */
> + unsigned long nr = NR_PAGES(order);
> + unsigned long j;
> +
> + gfp &= ~__GFP_NOWARN;
> + for (j = 0; j < nr && i < npages; j++, i++, addr += PAGE_SIZE) {
> + folio = vas ?
> + vma_alloc_folio(gfp, 0, vas, addr) :
> + folio_alloc(gfp, 0);
> + if (!folio)
> + goto free_pages;
> +
> + page = folio_page(folio, 0);
> + mpfn[i] = migrate_pfn(page_to_pfn(page));
> + }
> + continue;
> + }
>
> if (!folio)
> goto free_pages;
^^^^^^^^^^^^^^^^^^^^^^^^
[Severity: High]
This is a pre-existing issue, but does the code shortly after this point
unconditionally set MIGRATE_PFN_COMPOUND on the destination PFNs?
drm_pagemap_migrate_populate_ram_pfn() has the following sequence further down:
if (order)
mpfn[i] |= MIGRATE_PFN_COMPOUND;
When migrate_vma_setup() processes a PTE-mapped THP or an unaligned range,
it doesn't set MIGRATE_PFN_COMPOUND in the source PFN array. Because this
function unconditionally sets it on the destination and skips subsequent entries
by advancing the loop index by the full order, it creates a mismatch.
Will migrate_vma_pages() see mismatched flags, fall back to PTE-by-PTE
mapping, and skip the remaining entries because they are zeroed out, leaving
the VMA partially unmapped and leaking the newly allocated THPs?
--
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=3
lmpx.com only provides a reader for public news (NNTP) servers. It is not
affiliated with the servers or forums shown here and is not responsible for
the content of articles, which is written by their respective authors.