linux-kernel - Re: [PATCH 20/31] kasan, vmalloc: reset tags in vmalloc functions

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <YajVYNBDOyI3hTx1@elver.google.com>
Date:   Thu, 2 Dec 2021 15:17:04 +0100
From:   Marco Elver <elver@...gle.com>
To:     andrey.konovalov@...ux.dev
Cc:     Alexander Potapenko <glider@...gle.com>,
        Vincenzo Frascino <vincenzo.frascino@....com>,
        Catalin Marinas <catalin.marinas@....com>,
        Peter Collingbourne <pcc@...gle.com>,
        Andrey Konovalov <andreyknvl@...il.com>,
        Dmitry Vyukov <dvyukov@...gle.com>, kasan-dev@...glegroups.com,
        Andrew Morton <akpm@...ux-foundation.org>, linux-mm@...ck.org,
        Will Deacon <will@...nel.org>,
        linux-arm-kernel@...ts.infradead.org,
        Evgenii Stepanov <eugenis@...gle.com>,
        linux-kernel@...r.kernel.org,
        Andrey Konovalov <andreyknvl@...gle.com>
Subject: Re: [PATCH 20/31] kasan, vmalloc: reset tags in vmalloc functions

On Tue, Nov 30, 2021 at 11:07PM +0100, andrey.konovalov@...ux.dev wrote:
> From: Andrey Konovalov <andreyknvl@...gle.com>
> 
> In preparation for adding vmalloc support to SW/HW_TAGS KASAN,
> reset pointer tags in functions that use pointer values in
> range checks.
> 
> vread() is a special case here. Resetting the pointer tag in its
> prologue could technically lead to missing bad accesses to virtual
> mappings in its implementation. However, vread() doesn't access the
> virtual mappings cirectly. Instead, it recovers the physical address

s/cirectly/directly/

But this paragraph is a little confusing, because first you point out
that vread() might miss bad accesses, but then say that it does checked
accesses. I think to avoid confusing the reader, maybe just say that
vread() is checked, but hypothetically, should its implementation change
to directly access addr, invalid accesses might be missed.

Did I get this right? Or am I still confused?

> via page_address(vmalloc_to_page()) and acceses that. And as
> page_address() recovers the pointer tag, the accesses are checked.
> 
> Signed-off-by: Andrey Konovalov <andreyknvl@...gle.com>
> ---
>  mm/vmalloc.c | 12 +++++++++---
>  1 file changed, 9 insertions(+), 3 deletions(-)
> 
> diff --git a/mm/vmalloc.c b/mm/vmalloc.c
> index c5235e3e5857..a059b3100c0a 100644
> --- a/mm/vmalloc.c
> +++ b/mm/vmalloc.c
> @@ -72,7 +72,7 @@ static const bool vmap_allow_huge = false;
>  
>  bool is_vmalloc_addr(const void *x)
>  {
> -	unsigned long addr = (unsigned long)x;
> +	unsigned long addr = (unsigned long)kasan_reset_tag(x);
>  
>  	return addr >= VMALLOC_START && addr < VMALLOC_END;
>  }
> @@ -630,7 +630,7 @@ int is_vmalloc_or_module_addr(const void *x)
>  	 * just put it in the vmalloc space.
>  	 */
>  #if defined(CONFIG_MODULES) && defined(MODULES_VADDR)
> -	unsigned long addr = (unsigned long)x;
> +	unsigned long addr = (unsigned long)kasan_reset_tag(x);
>  	if (addr >= MODULES_VADDR && addr < MODULES_END)
>  		return 1;
>  #endif
> @@ -804,6 +804,8 @@ static struct vmap_area *find_vmap_area_exceed_addr(unsigned long addr)
>  	struct vmap_area *va = NULL;
>  	struct rb_node *n = vmap_area_root.rb_node;
>  
> +	addr = (unsigned long)kasan_reset_tag((void *)addr);
> +
>  	while (n) {
>  		struct vmap_area *tmp;
>  
> @@ -825,6 +827,8 @@ static struct vmap_area *__find_vmap_area(unsigned long addr)
>  {
>  	struct rb_node *n = vmap_area_root.rb_node;
>  
> +	addr = (unsigned long)kasan_reset_tag((void *)addr);
> +
>  	while (n) {
>  		struct vmap_area *va;
>  
> @@ -2143,7 +2147,7 @@ EXPORT_SYMBOL_GPL(vm_unmap_aliases);
>  void vm_unmap_ram(const void *mem, unsigned int count)
>  {
>  	unsigned long size = (unsigned long)count << PAGE_SHIFT;
> -	unsigned long addr = (unsigned long)mem;
> +	unsigned long addr = (unsigned long)kasan_reset_tag(mem);
>  	struct vmap_area *va;
>  
>  	might_sleep();
> @@ -3361,6 +3365,8 @@ long vread(char *buf, char *addr, unsigned long count)
>  	unsigned long buflen = count;
>  	unsigned long n;
>  
> +	addr = kasan_reset_tag(addr);
> +
>  	/* Don't allow overflow */
>  	if ((unsigned long) addr + count < count)
>  		count = -(unsigned long) addr;
> -- 
> 2.25.1
>