Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider: - [High] mm: BUG_ON() triggered in __vunmap_range_noflush() on first chunk failure - [High] mm: stale TLB entries left on mapping rollback cause UAF --
commit 8d2eaa273026d4be467acd8256954d343a66fc4c Author: Hao Ge <[email protected]> mm/vmalloc: undo partial mappings inside the mapping functions This patch modifies __vmap_pages_range_noflush() and friends to undo the PTEs they installed themselves on failure. Callers no longer need to clean up partial mappings. > diff --git a/mm/vmalloc.c b/mm/vmalloc.c > index a9fce7efe4f49..436393aba7eb7 100644 > --- a/mm/vmalloc.c > +++ b/mm/vmalloc.c > @@ -349,6 +349,10 @@ static int vmap_range_noflush(unsigned long addr, > unsigned long end, > if (mask & ARCH_PAGE_TABLE_SYNC_MASK) > arch_sync_kernel_mappings(start, end); > > + /* Undo the PTEs installed before the failure. */ > + if (err) > + __vunmap_range_noflush(start, end); > + > return err; > } [Severity: High] Is it safe to omit a TLB flush on rollback here? By clearing the PTEs using __vunmap_range_noflush() without flushing the TLB, the CPU might retain stale entries. If a caller like vm_module_tags_populate() in mm/alloc_tag.c frees the physical pages immediately after the mapping attempt fails: if (vmap_pages_range(...) < 0) { ... release_pages(arg, nr); } could speculative or adjacent accesses hit the freed memory, leading to a use-after-free and state corruption? [ ... ] > @@ -683,6 +694,7 @@ int __vmap_pages_range_noflush(unsigned long addr, > unsigned long end, > pgprot_t prot, struct page **pages, unsigned int page_shift) > { > unsigned int i, nr = (end - addr) >> PAGE_SHIFT; > + unsigned long start = addr; > > WARN_ON(page_shift < PAGE_SHIFT); > > @@ -696,8 +708,14 @@ int __vmap_pages_range_noflush(unsigned long addr, > unsigned long end, > err = vmap_range_noflush(addr, addr + (1UL << page_shift), > page_to_phys(pages[i]), prot, > page_shift); > - if (err) > + if (err) { > + /* > + * vmap_range_noflush() undoes its own partial work, > + * discard the chunks that already mapped. > + */ > + __vunmap_range_noflush(start, addr); > return err; > + } [Severity: High] What happens if vmap_range_noflush() fails on the very first iteration of this loop? In that case, the variable addr is still equal to start, so this calls __vunmap_range_noflush() with a zero-length range. Because __vunmap_range_noflush() contains: BUG_ON(addr >= end); will this immediately panic the kernel on an early allocation failure? > > addr += 1UL << page_shift; > } > @@ -714,7 +732,16 @@ int vmap_pages_range_noflush(unsigned long addr, > unsigned long end, > > if (ret) > return ret; > - return __vmap_pages_range_noflush(addr, end, prot, pages, page_shift); > + > + ret = __vmap_pages_range_noflush(addr, end, prot, pages, page_shift); > + /* > + * The page tables undo themselves on failure. Tear down the > + * metadata that was fully set up before the mapping failed. > + */ > + if (ret) > + kmsan_vunmap_range_noflush(addr, end); [Severity: High] Are we also leaving behind stale TLB entries for the KMSAN metadata here? kmsan_vmap_pages_range_noflush() successfully maps the metadata and explicitly flushes the TLB for shadow/origin mappings. If the subsequent main mapping fails, this rollback clears the KMSAN PTEs but skips the TLB flush, which might leave stale TLB entries pointing to freed KMSAN metadata. > + > + return ret; > } -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=2
