| Message ID | 20230413131223.4135168-2-glider@google.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [v2,1/4] mm: kmsan: handle alloc failures in kmsan_vmap_pages_range_noflush() | expand |
On Thu, 13 Apr 2023 at 15:12, Alexander Potapenko <glider@google.com> 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@gmail.com> > Link: https://lore.kernel.org/linux-mm/CANX2M5ZRrRA64k0hOif02TjmY9kbbO2aCBPyq79es34RXZ=cAw@mail.gmail.com/ > Fixes: b073d7f8aee4 ("mm: kmsan: maintain KMSAN metadata for page operations") > Signed-off-by: Alexander Potapenko <glider@google.com> Reviewed-by: Marco Elver <elver@google.com> > --- > 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@intel.com>) > --- > 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 >
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; }
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@gmail.com> Link: https://lore.kernel.org/linux-mm/CANX2M5ZRrRA64k0hOif02TjmY9kbbO2aCBPyq79es34RXZ=cAw@mail.gmail.com/ Fixes: b073d7f8aee4 ("mm: kmsan: maintain KMSAN metadata for page operations") Signed-off-by: Alexander Potapenko <glider@google.com> --- 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@intel.com>) --- include/linux/kmsan.h | 19 ++++++++------- mm/kmsan/hooks.c | 55 ++++++++++++++++++++++++++++++++++++------- mm/vmalloc.c | 4 ++-- 3 files changed, 59 insertions(+), 19 deletions(-)