Re: [PATCH v2 1/4] mm: kmsan: handle alloc failures in kmsan_vmap_pages_range_noflush()
From: Marco Elver
Date: Tue Apr 18 2023 - 06:11:13 EST
On Thu, 13 Apr 2023 at 15:12, Alexander Potapenko <glider@xxxxxxxxxx> wrote:
>
> As reported by Dipanjan Das, when KMSAN is used together with kernel
> fault injection (or, generally, even without the latter), calls to
> kcalloc() or __vmap_pages_range_noflush() may fail, leaving the
> metadata mappings for the virtual mapping in an inconsistent state.
> When these metadata mappings are accessed later, the kernel crashes.
>
> To address the problem, we return a non-zero error code from
> kmsan_vmap_pages_range_noflush() in the case of any allocation/mapping
> failure inside it, and make vmap_pages_range_noflush() return an error
> if KMSAN fails to allocate the metadata.
>
> This patch also removes KMSAN_WARN_ON() from vmap_pages_range_noflush(),
> as these allocation failures are not fatal anymore.
>
> 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>
Looks reasonable, thanks.
> ---
> v2:
> -- return 0 from the inline version of kmsan_vmap_pages_range_noflush()
> (spotted by kernel test robot <lkp@xxxxxxxxx>)
> ---
> include/linux/kmsan.h | 20 +++++++++++---------
> mm/kmsan/shadow.c | 27 ++++++++++++++++++---------
> mm/vmalloc.c | 6 +++++-
> 3 files changed, 34 insertions(+), 19 deletions(-)
>
> diff --git a/include/linux/kmsan.h b/include/linux/kmsan.h
> index e38ae3c346184..c7ff3aefc5a13 100644
> --- a/include/linux/kmsan.h
> +++ b/include/linux/kmsan.h
> @@ -134,11 +134,12 @@ void kmsan_kfree_large(const void *ptr);
> * @page_shift: page_shift passed to vmap_range_noflush().
> *
> * KMSAN maps shadow and origin pages of @pages into contiguous ranges in
> - * vmalloc metadata address range.
> + * vmalloc metadata address range. Returns 0 on success, callers must check
> + * for non-zero return value.
> */
> -void kmsan_vmap_pages_range_noflush(unsigned long start, unsigned long end,
> - pgprot_t prot, struct page **pages,
> - unsigned int page_shift);
> +int kmsan_vmap_pages_range_noflush(unsigned long start, unsigned long end,
> + pgprot_t prot, struct page **pages,
> + unsigned int page_shift);
>
> /**
> * kmsan_vunmap_kernel_range_noflush() - Notify KMSAN about a vunmap.
> @@ -281,12 +282,13 @@ static inline void kmsan_kfree_large(const void *ptr)
> {
> }
>
> -static inline void kmsan_vmap_pages_range_noflush(unsigned long start,
> - unsigned long end,
> - pgprot_t prot,
> - struct page **pages,
> - unsigned int page_shift)
> +static inline int kmsan_vmap_pages_range_noflush(unsigned long start,
> + unsigned long end,
> + pgprot_t prot,
> + struct page **pages,
> + unsigned int page_shift)
> {
> + return 0;
> }
>
> static inline void kmsan_vunmap_range_noflush(unsigned long start,
> diff --git a/mm/kmsan/shadow.c b/mm/kmsan/shadow.c
> index a787c04e9583c..b8bb95eea5e3d 100644
> --- a/mm/kmsan/shadow.c
> +++ b/mm/kmsan/shadow.c
> @@ -216,27 +216,29 @@ void kmsan_free_page(struct page *page, unsigned int order)
> kmsan_leave_runtime();
> }
>
> -void kmsan_vmap_pages_range_noflush(unsigned long start, unsigned long end,
> - pgprot_t prot, struct page **pages,
> - unsigned int page_shift)
> +int kmsan_vmap_pages_range_noflush(unsigned long start, unsigned long end,
> + pgprot_t prot, struct page **pages,
> + unsigned int page_shift)
> {
> unsigned long shadow_start, origin_start, shadow_end, origin_end;
> struct page **s_pages, **o_pages;
> - int nr, mapped;
> + int nr, mapped, err = 0;
>
> if (!kmsan_enabled)
> - return;
> + return 0;
>
> shadow_start = vmalloc_meta((void *)start, KMSAN_META_SHADOW);
> shadow_end = vmalloc_meta((void *)end, KMSAN_META_SHADOW);
> if (!shadow_start)
> - return;
> + return 0;
>
> nr = (end - start) / PAGE_SIZE;
> s_pages = kcalloc(nr, sizeof(*s_pages), GFP_KERNEL);
> o_pages = kcalloc(nr, sizeof(*o_pages), GFP_KERNEL);
> - if (!s_pages || !o_pages)
> + if (!s_pages || !o_pages) {
> + err = -ENOMEM;
> goto ret;
> + }
> for (int i = 0; i < nr; i++) {
> s_pages[i] = shadow_page_for(pages[i]);
> o_pages[i] = origin_page_for(pages[i]);
> @@ -249,10 +251,16 @@ void kmsan_vmap_pages_range_noflush(unsigned long start, unsigned long end,
> kmsan_enter_runtime();
> mapped = __vmap_pages_range_noflush(shadow_start, shadow_end, prot,
> s_pages, page_shift);
> - KMSAN_WARN_ON(mapped);
> + if (mapped) {
> + err = mapped;
> + goto ret;
> + }
> mapped = __vmap_pages_range_noflush(origin_start, origin_end, prot,
> o_pages, page_shift);
> - KMSAN_WARN_ON(mapped);
> + if (mapped) {
> + err = mapped;
> + goto ret;
> + }
> kmsan_leave_runtime();
> flush_tlb_kernel_range(shadow_start, shadow_end);
> flush_tlb_kernel_range(origin_start, origin_end);
> @@ -262,6 +270,7 @@ void kmsan_vmap_pages_range_noflush(unsigned long start, unsigned long end,
> ret:
> kfree(s_pages);
> kfree(o_pages);
> + return err;
> }
>
> /* Allocate metadata for pages allocated at boot time. */
> diff --git a/mm/vmalloc.c b/mm/vmalloc.c
> index a50072066221a..1355d95cce1ca 100644
> --- a/mm/vmalloc.c
> +++ b/mm/vmalloc.c
> @@ -605,7 +605,11 @@ int __vmap_pages_range_noflush(unsigned long addr, unsigned long end,
> int vmap_pages_range_noflush(unsigned long addr, unsigned long end,
> pgprot_t prot, struct page **pages, unsigned int page_shift)
> {
> - kmsan_vmap_pages_range_noflush(addr, end, prot, pages, page_shift);
> + int ret = kmsan_vmap_pages_range_noflush(addr, end, prot, pages,
> + page_shift);
> +
> + if (ret)
> + return ret;
> return __vmap_pages_range_noflush(addr, end, prot, pages, page_shift);
> }
>
> --
> 2.40.0.577.gac1e443424-goog
>