| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <CANpmjNML3wQjaxujkAxWiTVwgBaUtbCPEQES7duh0ktyT2ddTQ@mail.gmail.com>
Date: Tue, 18 Apr 2023 12:10:27 +0200
From: Marco Elver <elver@...gle.com>
To: Alexander Potapenko <glider@...gle.com>
Cc: urezki@...il.com, hch@...radead.org, linux-kernel@...r.kernel.org,
linux-mm@...ck.org, akpm@...ux-foundation.org, dvyukov@...gle.com,
kasan-dev@...glegroups.com,
Dipanjan Das <mail.dipanjan.das@...il.com>
Subject: Re: [PATCH v2 2/4] mm: kmsan: handle alloc failures in kmsan_ioremap_page_range()
On Thu, 13 Apr 2023 at 15:12, Alexander Potapenko <glider@...gle.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@...il.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@...gle.com>
Reviewed-by: Marco Elver <elver@...gle.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@...el.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
>
Powered by blists - more mailing lists