On Thu, 13 Apr 2023 at 15:12, Alexander Potapenko <glider@xxxxxxxxxx> wrote: > > Similarly to kmsan_vmap_pages_range_noflush(), > kmsan_ioremap_page_range() must also properly handle allocation/mapping > failures. In the case of such, it must clean up the already created > metadata mappings and return an error code, so that the error can be > propagated to ioremap_page_range(). Without doing so, KMSAN may silently > fail to bring the metadata for the page range into a consistent state, > which will result in user-visible crashes when trying to access them. > > Reported-by: Dipanjan Das <mail.dipanjan.das@xxxxxxxxx> > Link: https://lore.kernel.org/linux-mm/CANX2M5ZRrRA64k0hOif02TjmY9kbbO2aCBPyq79es34RXZ=cAw@xxxxxxxxxxxxxx/ > Fixes: b073d7f8aee4 ("mm: kmsan: maintain KMSAN metadata for page operations") > Signed-off-by: Alexander Potapenko <glider@xxxxxxxxxx> Reviewed-by: Marco Elver <elver@xxxxxxxxxx> > --- > v2: > -- updated patch description as requested by Andrew Morton > -- check the return value of __vmap_pages_range_noflush(), as suggested by Dipanjan Das > -- return 0 from the inline version of kmsan_ioremap_page_range() > (spotted by kernel test robot <lkp@xxxxxxxxx>) > --- > include/linux/kmsan.h | 19 ++++++++------- > mm/kmsan/hooks.c | 55 ++++++++++++++++++++++++++++++++++++------- > mm/vmalloc.c | 4 ++-- > 3 files changed, 59 insertions(+), 19 deletions(-) > > diff --git a/include/linux/kmsan.h b/include/linux/kmsan.h > index c7ff3aefc5a13..30b17647ce3c7 100644 > --- a/include/linux/kmsan.h > +++ b/include/linux/kmsan.h > @@ -160,11 +160,12 @@ void kmsan_vunmap_range_noflush(unsigned long start, unsigned long end); > * @page_shift: page_shift argument passed to vmap_range_noflush(). > * > * KMSAN creates new metadata pages for the physical pages mapped into the > - * virtual memory. > + * virtual memory. Returns 0 on success, callers must check for non-zero return > + * value. > */ > -void kmsan_ioremap_page_range(unsigned long addr, unsigned long end, > - phys_addr_t phys_addr, pgprot_t prot, > - unsigned int page_shift); > +int kmsan_ioremap_page_range(unsigned long addr, unsigned long end, > + phys_addr_t phys_addr, pgprot_t prot, > + unsigned int page_shift); > > /** > * kmsan_iounmap_page_range() - Notify KMSAN about a iounmap_page_range() call. > @@ -296,12 +297,12 @@ static inline void kmsan_vunmap_range_noflush(unsigned long start, > { > } > > -static inline void kmsan_ioremap_page_range(unsigned long start, > - unsigned long end, > - phys_addr_t phys_addr, > - pgprot_t prot, > - unsigned int page_shift) > +static inline int kmsan_ioremap_page_range(unsigned long start, > + unsigned long end, > + phys_addr_t phys_addr, pgprot_t prot, > + unsigned int page_shift) > { > + return 0; > } > > static inline void kmsan_iounmap_page_range(unsigned long start, > diff --git a/mm/kmsan/hooks.c b/mm/kmsan/hooks.c > index 3807502766a3e..ec0da72e65aa0 100644 > --- a/mm/kmsan/hooks.c > +++ b/mm/kmsan/hooks.c > @@ -148,35 +148,74 @@ void kmsan_vunmap_range_noflush(unsigned long start, unsigned long end) > * into the virtual memory. If those physical pages already had shadow/origin, > * those are ignored. > */ > -void kmsan_ioremap_page_range(unsigned long start, unsigned long end, > - phys_addr_t phys_addr, pgprot_t prot, > - unsigned int page_shift) > +int kmsan_ioremap_page_range(unsigned long start, unsigned long end, > + phys_addr_t phys_addr, pgprot_t prot, > + unsigned int page_shift) > { > gfp_t gfp_mask = GFP_KERNEL | __GFP_ZERO; > struct page *shadow, *origin; > unsigned long off = 0; > - int nr; > + int nr, err = 0, clean = 0, mapped; > > if (!kmsan_enabled || kmsan_in_runtime()) > - return; > + return 0; > > nr = (end - start) / PAGE_SIZE; > kmsan_enter_runtime(); > - for (int i = 0; i < nr; i++, off += PAGE_SIZE) { > + for (int i = 0; i < nr; i++, off += PAGE_SIZE, clean = i) { > shadow = alloc_pages(gfp_mask, 1); > origin = alloc_pages(gfp_mask, 1); > - __vmap_pages_range_noflush( > + if (!shadow || !origin) { > + err = -ENOMEM; > + goto ret; > + } > + mapped = __vmap_pages_range_noflush( > vmalloc_shadow(start + off), > vmalloc_shadow(start + off + PAGE_SIZE), prot, &shadow, > PAGE_SHIFT); > - __vmap_pages_range_noflush( > + if (mapped) { > + err = mapped; > + goto ret; > + } > + shadow = NULL; > + mapped = __vmap_pages_range_noflush( > vmalloc_origin(start + off), > vmalloc_origin(start + off + PAGE_SIZE), prot, &origin, > PAGE_SHIFT); > + if (mapped) { > + __vunmap_range_noflush( > + vmalloc_shadow(start + off), > + vmalloc_shadow(start + off + PAGE_SIZE)); > + err = mapped; > + goto ret; > + } > + origin = NULL; > + } > + /* Page mapping loop finished normally, nothing to clean up. */ > + clean = 0; > + > +ret: > + if (clean > 0) { > + /* > + * Something went wrong. Clean up shadow/origin pages allocated > + * on the last loop iteration, then delete mappings created > + * during the previous iterations. > + */ > + if (shadow) > + __free_pages(shadow, 1); > + if (origin) > + __free_pages(origin, 1); > + __vunmap_range_noflush( > + vmalloc_shadow(start), > + vmalloc_shadow(start + clean * PAGE_SIZE)); > + __vunmap_range_noflush( > + vmalloc_origin(start), > + vmalloc_origin(start + clean * PAGE_SIZE)); > } > flush_cache_vmap(vmalloc_shadow(start), vmalloc_shadow(end)); > flush_cache_vmap(vmalloc_origin(start), vmalloc_origin(end)); > kmsan_leave_runtime(); > + return err; > } > > void kmsan_iounmap_page_range(unsigned long start, unsigned long end) > diff --git a/mm/vmalloc.c b/mm/vmalloc.c > index 1355d95cce1ca..31ff782d368b0 100644 > --- a/mm/vmalloc.c > +++ b/mm/vmalloc.c > @@ -313,8 +313,8 @@ int ioremap_page_range(unsigned long addr, unsigned long end, > ioremap_max_page_shift); > flush_cache_vmap(addr, end); > if (!err) > - kmsan_ioremap_page_range(addr, end, phys_addr, prot, > - ioremap_max_page_shift); > + err = kmsan_ioremap_page_range(addr, end, phys_addr, prot, > + ioremap_max_page_shift); > return err; > } > > -- > 2.40.0.577.gac1e443424-goog >
